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.] The proposed patch papers over the issue, leaving a landmine for some future DM target. In addition, the patch header is very muddled about relation between table_device and dm_dev_internal. And also needs clarification like: "kfree(td) in dm_put_table_device() is not interlocked with dm_dev_internal list (table->devices) management" Also, the commit referenced with the "Fixes:" is bogus. Wouldn't this change address the UAF (albeit, list_for_each_entry_safe likely needed in methods like retrieve_deps())? diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index 7d208b2b1a19..39e4c9ee8f16 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -434,9 +434,9 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d) return; } if (refcount_dec_and_test(&dd->count)) { - dm_put_table_device(ti->table->md, d); list_del(&dd->list); kfree(dd); + dm_put_table_device(ti->table->md, d); } } EXPORT_SYMBOL(dm_put_device); -- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel