From: Ming-Hung Tsai <[email protected]> There's an UAF bug in dm_get_device(): when a table construction calls dm_get_device() on the same device twice with widen access mode (e.g., read-only first then read-write), the second dm_get_device() calls upgrade_mode(), which reopens the device in upgraded mode, making previously returned dm_dev* a stale pointer.
Fixing approaches: - Add per-mode views (dm_dev_ref) from a per-device dm_dev_internal to multiple table_devices. - Keep reusing the dm_dev_ref when the requested mode is included, i.e., a read-only reference can reuse the read-write opened device within a table. - Maintain a canonical block_device pointer in dm_dev_internal for handling the DM_TABLE_DEPS ioctl. Also tweak the way of mode sharing check in find_ref() to "(x & mask) == mask". The result stays the same to the previous upgrade_mode() guard "(x | mask) == x". Pros: keep the relationship between dm_dev_internal and table_device: one dm_dev_internal per device, regardless the mode, so the output of DMM_TABLE_DEPS ioctl unchanged. Cons: extra dm_dev_ref allocation. Note that now the table_device's count not necessarily mean the number tables referecing this <dev, mode> pair, since we're allowing multiple references within a table. Signed-off-by: Ming Hung Tsai <[email protected]> Signed-off-by: Mikulas Patocka <[email protected]> Co-Authored-By: Claude Opus 5 <[email protected]> Cc: [email protected] --- drivers/md/dm-ioctl.c | 2 +- drivers/md/dm-table.c | 142 +++++++++++++++++++++++------------------- drivers/md/dm.c | 2 +- drivers/md/dm.h | 18 +++++- 4 files changed, 98 insertions(+), 66 deletions(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 8f945f0aeda5..4a7cb0926c17 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -1794,7 +1794,7 @@ static void retrieve_deps(struct dm_table *table, deps->count = count; count = 0; list_for_each_entry(dd, dm_table_get_devices(table), list) - deps->dev[count++] = huge_encode_dev(dd->dm_dev->bdev->bd_dev); + deps->dev[count++] = huge_encode_dev(dd->bdev->bd_dev); param->data_size = param->data_start + needed; } diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c index a483f49dd4ce..db1ec4ae8e56 100644 --- a/drivers/md/dm-table.c +++ b/drivers/md/dm-table.c @@ -165,14 +165,16 @@ int dm_table_create(struct dm_table **result, blk_mode_t mode, static void free_devices(struct list_head *devices, struct mapped_device *md) { - struct list_head *tmp, *next; - - list_for_each_safe(tmp, next, devices) { - struct dm_dev_internal *dd = - list_entry(tmp, struct dm_dev_internal, list); - DMWARN("%s: dm_table_destroy: dm_put_device call missing for %s", - dm_device_name(md), dd->dm_dev->name); - dm_put_table_device(md, dd->dm_dev); + struct dm_dev_internal *dd, *dd_next; + struct dm_dev_ref *ref, *ref_next; + + list_for_each_entry_safe(dd, dd_next, devices, list) { + list_for_each_entry_safe(ref, ref_next, &dd->refs, list) { + DMWARN("%s: dm_table_destroy: dm_put_device call missing for %s", + dm_device_name(md), ref->dm_dev->name); + dm_put_table_device(md, ref->dm_dev); + kfree(ref); + } kfree(dd); } } @@ -218,12 +220,27 @@ static struct dm_dev_internal *find_device(struct list_head *l, dev_t dev) struct dm_dev_internal *dd; list_for_each_entry(dd, l, list) - if (dd->dm_dev->bdev->bd_dev == dev) + if (dd->bdev->bd_dev == dev) return dd; return NULL; } +/* + * See if the device is already open with a mode that covers the + * requested one. + */ +static struct dm_dev_ref *find_ref(struct dm_dev_internal *dd, blk_mode_t mode) +{ + struct dm_dev_ref *ref; + + list_for_each_entry(ref, &dd->refs, list) + if ((ref->dm_dev->mode & mode) == mode) + return ref; + + return NULL; +} + /* * If possible, this checks an area of a destination device is invalid. */ @@ -303,31 +320,6 @@ static int device_area_is_invalid(struct dm_target *ti, struct dm_dev *dev, return 0; } -/* - * This upgrades the mode on an already open dm_dev, being - * careful to leave things as they were if we fail to reopen the - * device and not to touch the existing bdev field in case - * it is accessed concurrently. - */ -static int upgrade_mode(struct dm_dev_internal *dd, blk_mode_t new_mode, - struct mapped_device *md) -{ - int r; - struct dm_dev *old_dev, *new_dev; - - old_dev = dd->dm_dev; - - r = dm_get_table_device(md, dd->dm_dev->bdev->bd_dev, - dd->dm_dev->mode | new_mode, &new_dev); - if (r) - return r; - - dd->dm_dev = new_dev; - dm_put_table_device(md, old_dev); - - return 0; -} - /* * Note: the __ref annotation is because this function can call the __init * marked early_lookup_bdev when called during early boot code from dm-init.c. @@ -360,7 +352,9 @@ EXPORT_SYMBOL(dm_devt_from_path); /* * Add a device to the list, or just increment the usage count if - * it's already present. + * it's already present with a mode that covers the requested one. + * Otherwise open the device again with the requested mode, so the + * dm_devs returned earlier stay valid. */ int dm_get_device(struct dm_target *ti, const char *path, blk_mode_t mode, struct dm_dev **result) @@ -368,6 +362,7 @@ int dm_get_device(struct dm_target *ti, const char *path, blk_mode_t mode, int r; dev_t dev; struct dm_dev_internal *dd; + struct dm_dev_ref *ref; struct dm_table *t = ti->table; BUG_ON(!t); @@ -380,30 +375,46 @@ int dm_get_device(struct dm_target *ti, const char *path, blk_mode_t mode, return -EINVAL; dd = find_device(&t->devices, dev); - if (!dd) { + if (dd) { + ref = find_ref(dd, mode); + if (ref) { + refcount_inc(&ref->count); + goto out; + } + } else { dd = kmalloc_obj(*dd); if (!dd) return -ENOMEM; + INIT_LIST_HEAD(&dd->refs); + } - r = dm_get_table_device(t->md, dev, mode, &dd->dm_dev); - if (r) { - kfree(dd); - return r; - } + ref = kmalloc_obj(*ref); + if (!ref) { + r = -ENOMEM; + goto bad; + } - refcount_set(&dd->count, 1); - list_add(&dd->list, &t->devices); - goto out; + r = dm_get_table_device(t->md, dev, mode, &ref->dm_dev); + if (r) { + kfree(ref); + goto bad; + } + refcount_set(&ref->count, 1); - } else if (dd->dm_dev->mode != (mode | dd->dm_dev->mode)) { - r = upgrade_mode(dd, mode, t->md); - if (r) - return r; + /* A device in the list always has at least one ref */ + if (list_empty(&dd->refs)) { + dd->bdev = ref->dm_dev->bdev; + list_add(&dd->list, &t->devices); } - refcount_inc(&dd->count); + list_add_tail(&ref->list, &dd->refs); out: - *result = dd->dm_dev; + *result = ref->dm_dev; return 0; + +bad: + if (list_empty(&dd->refs)) + kfree(dd); + return r; } EXPORT_SYMBOL(dm_get_device); @@ -453,23 +464,28 @@ static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, */ void dm_put_device(struct dm_target *ti, struct dm_dev *d) { - int found = 0; struct list_head *devices = &ti->table->devices; struct dm_dev_internal *dd; + struct dm_dev_ref *ref; - list_for_each_entry(dd, devices, list) { - if (dd->dm_dev == d) { - found = 1; - break; - } - } - if (!found) { - DMERR("%s: device %s not in table devices list", - dm_device_name(ti->table->md), d->name); + list_for_each_entry(dd, devices, list) + list_for_each_entry(ref, &dd->refs, list) + if (ref->dm_dev == d) + goto found; + + DMERR("%s: device %s not in table devices list", + dm_device_name(ti->table->md), d->name); + return; + +found: + if (!refcount_dec_and_test(&ref->count)) return; - } - if (refcount_dec_and_test(&dd->count)) { - dm_put_table_device(ti->table->md, d); + + dm_put_table_device(ti->table->md, d); + list_del(&ref->list); + kfree(ref); + + if (list_empty(&dd->refs)) { list_del(&dd->list); kfree(dd); } diff --git a/drivers/md/dm.c b/drivers/md/dm.c index eb9e32995b79..1fff84b3f869 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1588,7 +1588,7 @@ static void __send_empty_flush(struct clone_info *ci) */ clone = alloc_tio(ci, NULL, 0, &len, GFP_NOIO); atomic_add(1, &ci->io->io_count); - bio_set_dev(clone, dd->dm_dev->bdev); + bio_set_dev(clone, dd->bdev); clone->bi_end_io = clone_endio; dm_submit_bio_remap(clone, NULL); } diff --git a/drivers/md/dm.h b/drivers/md/dm.h index 7a795979ec72..34f58060243f 100644 --- a/drivers/md/dm.h +++ b/drivers/md/dm.h @@ -37,10 +37,26 @@ /* * List of devices that a metadevice uses and should open/close. + * There is one dm_dev_internal per device. It holds one dm_dev_ref + * for each mode that the targets opened the device with. */ struct dm_dev_internal { struct list_head list; - refcount_t count; + struct block_device *bdev; + struct list_head refs; +}; + +/* + * One open of a device with one mode, shared by the targets of a table. + * dm_get_device() reuses a dm_dev_ref whose mode covers the requested + * mode, and adds a new one only when none does. So the dm_dev_refs of + * a device have different modes, and a table holds each table_device + * at most once. table_device->count is still the number of tables that + * use the table_device. + */ +struct dm_dev_ref { + struct list_head list; + refcount_t count; /* dm_get_device() calls that returned dm_dev */ struct dm_dev *dm_dev; }; -- 2.49.0
