On Wed, 2 Aug 2023, Mike Snitzer wrote:
> On Thu, May 18 2023 at 8:11P -0400,
> lilingfeng (A) <lilingfeng3@xxxxxxxxxx> wrote:
>
> >
> > 在 2022/10/18 3:56, Mike Snitzer 写道:
> > > On Mon, Oct 10 2022 at 10:41P -0400,
> > > Luo Meng <luomeng@xxxxxxxxxxxxxxx> wrote:
> > >
> > > > From: Luo Meng <luomeng12@xxxxxxxxxx>
> > > >
> > > > If dm_get_device() create dd in multipath_message(),
> > > > and then call table_deps() after dm_put_table_device(),
> > > > it will lead to concurrency UAF bugs.
> > > >
> > > > One of the concurrency UAF can be shown as below:
> > > >
> > > > (USE) | (FREE)
> > > > | target_message
> > > > | multipath_message
> > > > | dm_put_device
> > > > | dm_put_table_device #
> > > > | kfree(td) # table_device *td
> > > > ioctl # DM_TABLE_DEPS_CMD | ...
> > > > table_deps | ...
> > > > dm_get_live_or_inactive_table | ...
> > > > retrieve_dep | ...
> > > > list_for_each_entry | ...
> > > > deps->dev[count++] = | ...
> > > > huge_encode_dev | ...
> > > > (dd->dm_dev->bdev->bd_dev) | list_del(&dd->list)
> > > > | kfree(dd) # dm_dev_internal
> > > >
> > > > The root cause of UAF bugs is that find_device() failed in
> > > > dm_get_device() and will create dd and refcount set 1, kfree()
> > > > in dm_put_table() is not protected. When td, which there are
> > > > still pointers point to, is released, the concurrency UAF bug
> > > > will happen.
> > > >
> > > > This patch add a flag to determine whether to create a new dd.
> > > >
> > > > Fixes: 8215d6ec5fee(dm table: remove unused dm_get_device range parameters)
> > > > Signed-off-by: Luo Meng <luomeng12@xxxxxxxxxx>
> > > > ---
> > > > drivers/md/dm-mpath.c | 2 +-
> > > > drivers/md/dm-table.c | 43 +++++++++++++++++++++--------------
> > > > include/linux/device-mapper.h | 2 ++
> > > > 3 files changed, 29 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > > > index 0e325469a252..1f51059520ac 100644
> > > > --- a/drivers/md/dm-mpath.c
> > > > +++ b/drivers/md/dm-mpath.c
> > > > @@ -2001,7 +2001,7 @@ static int multipath_message(struct dm_target *ti, unsigned argc, char **argv,
> > > > goto out;
> > > > }
> > > > - r = dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev);
> > > > + r = __dm_get_device(ti, argv[1], dm_table_get_mode(ti->table), &dev, false);
> > > > if (r) {
> > > > DMWARN("message: error getting device %s",
> > > > argv[1]);
> > > > diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
> > > > index d8034ff0cb24..ad18a41f1608 100644
> > > > --- a/drivers/md/dm-table.c
> > > > +++ b/drivers/md/dm-table.c
> > > > @@ -338,12 +338,8 @@ dev_t dm_get_dev_t(const char *path)
> > > > }
> > > > EXPORT_SYMBOL_GPL(dm_get_dev_t);
> > > > -/*
> > > > - * Add a device to the list, or just increment the usage count if
> > > > - * it's already present.
> > > > - */
> > > > -int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > - struct dm_dev **result)
> > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > + struct dm_dev **result, bool create_dd)
> > > > {
> > > > int r;
> > > > dev_t dev;
> > > > @@ -367,19 +363,21 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > dd = find_device(&t->devices, dev);
> > > > if (!dd) {
> > > > - dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > > - if (!dd)
> > > > - return -ENOMEM;
> > > > -
> > > > - if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> > > > - kfree(dd);
> > > > - return r;
> > > > - }
> > > > + if (create_dd) {
> > > > + dd = kmalloc(sizeof(*dd), GFP_KERNEL);
> > > > + if (!dd)
> > > > + return -ENOMEM;
> > > > - refcount_set(&dd->count, 1);
> > > > - list_add(&dd->list, &t->devices);
> > > > - goto out;
> > > > + if ((r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev))) {
> > > > + kfree(dd);
> > > > + return r;
> > > > + }
> > > > + refcount_set(&dd->count, 1);
> > > > + list_add(&dd->list, &t->devices);
> > > > + goto out;
> > > > + } else
> > > > + return -ENODEV;
> > > > } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) {
> > > > r = upgrade_mode(dd, mode, t->md);
> > > > if (r)
> > > > @@ -390,6 +388,17 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > *result = dd->dm_dev;
> > > > return 0;
> > > > }
> > > > +EXPORT_SYMBOL(__dm_get_device);
> > > > +
> > > > +/*
> > > > + * Add a device to the list, or just increment the usage count if
> > > > + * it's already present.
> > > > + */
> > > > +int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > + struct dm_dev **result)
> > > > +{
> > > > + return __dm_get_device(ti, path, mode, result, true);
> > > > +}
> > > > EXPORT_SYMBOL(dm_get_device);
> > > > static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev,
> > > > diff --git a/include/linux/device-mapper.h b/include/linux/device-mapper.h
> > > > index 04c6acf7faaa..ef4031a844b6 100644
> > > > --- a/include/linux/device-mapper.h
> > > > +++ b/include/linux/device-mapper.h
> > > > @@ -178,6 +178,8 @@ dev_t dm_get_dev_t(const char *path);
> > > > int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > struct dm_dev **result);
> > > > void dm_put_device(struct dm_target *ti, struct dm_dev *d);
> > > > +int __dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
> > > > + struct dm_dev **result, bool create_dd);
> > > > /*
> > > > * Information about a target type
> > > > --
> > > > 2.31.1
> > > >
> > > This patch is treating the one multipath case like it is only consumer
> > > of dm_get_device() that might have this issue. It feels too focused on
> > > an instance where dm_get_device()'s side-effect of creating the device
> > > is unwelcome. Feels like you're treating the symptom rather than the
> > > problem (e.g. that the "kfree() in dm_put_table() is not protected"?)
> > >
> > > Mike
> >
> > In other cases, kfree() in dm_put_table() is protected by srcu.
> > For example:
> > USE FREE
> > table_deps dev_remove
> > dm_get_live_or_inactive_table dm_sync_table
> > // lock
> > srcu_read_lock(&md->io_barrier)
> > // wait for unlock
> > synchronize_srcu(&md->io_barrier)
> > retrieve_deps
> > dm_put_live_table
> > // unlock
> > srcu_read_unlock(&md->io_barrier...)
> > dm_table_destroy
> > linear_dtr
> > dm_put_device
> > dm_put_table_device
> >
> > However, in multipath_message(), the dm_dev is created and destroyed
> > under srcu_read_lock, which means destroying dm_dev in this process
> > and using dm_dev in other process will happen at same time since both
> > of them hold srcu_read_lock.
> >
> > target_message
> > dm_get_live_table // lock
> > multipath_message
> > dm_get_device // created
> > // may be got by other processes
> > dm_put_device // destroyed
> > // may be used by other processes
> > dm_put_live_table // unlock
> >
> > We figured out some other solutions:
> > 1) Judge refcount of dd under some lock before using dm_dev;
> > 2) Get dd from list and use dm_dev under rcu;
> > 3) Implement dm_get_device_xxx() with reference to dm_get_device()
> > for dm-mpath to avoid creating dd when find_device() failed.
> >
> > Compared to solutions above, Luo Meng's patch may be more appropriate.
> > Any suggestions will be appreciated.
> >
> > Li
>
> [Cc'ing Mikulas so he can take a look at this too.]
Hi
I suggest this patch (but it's only compile-tested, so please run some
testsuite on it).
Mikulas
From: Mikulas Patocka <mpatocka@xxxxxxxxxx>
Subject: [PATCH] dm: fix a race condition in retrieve_deps
There's a race condition in the multipath target when retrieve_deps races
with multipath_message. multipath_message opens and closes a device using
dm_get_device and dm_put_device; retrieve_deps walks the list of open
devices without holding any lock. The end result may be memory corruption
or use-after-free memory access.
multipath_message already holds a mutex that prevents two
multipath_messages from running concurrently - in order to fix this race
condition, we modify retrieve_deps, so that it grabs and frees this mutex
too.
We add an entry "devices_mutex" to "struct dm_table" and we introduce a
function "dm_table_set_devices_mutex" that sets it. The mutex is set in
the multipath target contructor.
Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
drivers/md/dm-core.h | 5 +++++
drivers/md/dm-ioctl.c | 9 ++++++++-
drivers/md/dm-mpath.c | 2 ++
drivers/md/dm-table.c | 6 ++++++
include/linux/device-mapper.h | 5 +++++
5 files changed, 26 insertions(+), 1 deletion(-)
Index: linux-2.6/drivers/md/dm-core.h
===================================================================
--- linux-2.6.orig/drivers/md/dm-core.h
+++ linux-2.6/drivers/md/dm-core.h
@@ -214,6 +214,11 @@ struct dm_table {
/* a list of devices used by this table */
struct list_head devices;
+ /*
+ * The mutex should be set if the target modifies the "devices" list
+ * outside of the constructor and destructor.
+ */
+ struct mutex *devices_mutex;
/* events get handed up using this callback */
void (*event_fn)(void *data);
Index: linux-2.6/drivers/md/dm-table.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-table.c
+++ linux-2.6/drivers/md/dm-table.c
@@ -2034,6 +2034,12 @@ struct list_head *dm_table_get_devices(s
return &t->devices;
}
+void dm_table_set_devices_mutex(struct dm_table *t, struct mutex *m)
+{
+ t->devices_mutex = m;
+}
+EXPORT_SYMBOL(dm_table_set_devices_mutex);
+
blk_mode_t dm_table_get_mode(struct dm_table *t)
{
return t->mode;
Index: linux-2.6/drivers/md/dm-ioctl.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-ioctl.c
+++ linux-2.6/drivers/md/dm-ioctl.c
@@ -1630,6 +1630,9 @@ static void retrieve_deps(struct dm_tabl
struct dm_dev_internal *dd;
struct dm_target_deps *deps;
+ if (table->devices_mutex)
+ mutex_lock(table->devices_mutex);
+
deps = get_result_buffer(param, param_size, &len);
/*
@@ -1644,7 +1647,7 @@ static void retrieve_deps(struct dm_tabl
needed = struct_size(deps, dev, count);
if (len < needed) {
param->flags |= DM_BUFFER_FULL_FLAG;
- return;
+ goto ret;
}
/*
@@ -1656,6 +1659,10 @@ static void retrieve_deps(struct dm_tabl
deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev);
param->data_size = param->data_start + needed;
+
+ret:
+ if (table->devices_mutex)
+ mutex_unlock(table->devices_mutex);
}
static int table_deps(struct file *filp, struct dm_ioctl *param, size_t param_size)
Index: linux-2.6/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-mpath.c
+++ linux-2.6/drivers/md/dm-mpath.c
@@ -1268,6 +1268,8 @@ static int multipath_ctr(struct dm_targe
else
ti->per_io_data_size = sizeof(struct dm_mpath_io);
+ dm_table_set_devices_mutex(ti->table, &m->work_mutex);
+
return 0;
bad:
Index: linux-2.6/include/linux/device-mapper.h
===================================================================
--- linux-2.6.orig/include/linux/device-mapper.h
+++ linux-2.6/include/linux/device-mapper.h
@@ -177,6 +177,11 @@ struct dm_dev {
int dm_get_device(struct dm_target *ti, const char *path, blk_mode_t mode,
struct dm_dev **result);
void dm_put_device(struct dm_target *ti, struct dm_dev *d);
+/*
+ * The mutex must be set if we use dm_get_device or dm_put_device outside
+ * of the constructor and destructor.
+ */
+void dm_table_set_devices_mutex(struct dm_table *t, struct mutex *m);
/*
* Information about a target type--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
