Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] dm thin: TOCTOU race between dm_devt_from_path() and dm_get_device() - [Medium] dm thin: memory leak of struct thin_c on error paths in thin_ctr() - [Low] dm thin: wrong ti->error messages in thin_ctr() and pool_ctr() --
commit 1eca47222f62970abc013ee1decbccbb30099b93 Author: Drif Abdelmalek Mohamed Said <[email protected]> dm: reject aliased metadata/data and pool/origin devices This commit validates resolved device numbers with dm_devt_from_path() in pool_ctr() and thin_ctr() before opening the devices so that aliased metadata/data or pool/origin paths pointing to the same block device are rejected before upgrade_mode() can free an already-opened table_device. > diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c > index 358ed77ffb2be..281b2e1745c31 100644 > --- a/drivers/md/dm-thin.c > +++ b/drivers/md/dm-thin.c > @@ -3292,6 +3292,8 @@ static int pool_ctr(struct dm_target *ti, unsigned int > argc, char **argv) > dm_block_t low_water_blocks; > struct dm_dev *metadata_dev; > blk_mode_t metadata_mode; > + dev_t metadata_devt; > + dev_t data_devt; > > /* > * FIXME Remove validation from scope of lock. > @@ -3314,6 +3316,22 @@ static int pool_ctr(struct dm_target *ti, unsigned int > argc, char **argv) > goto out_unlock; > } > > + r = dm_devt_from_path(argv[0], &metadata_devt); > + if (r) { > + ti->error = "Error opening metadata block device"; > + goto out_unlock; > + } > + r = dm_devt_from_path(argv[1], &data_devt); > + if (r) { > + ti->error = "Error opening metadata data device"; > + goto out_unlock; > + } > + if (metadata_devt == data_devt) { > + ti->error = "Error setting metadata or data device"; > + r = -EINVAL; > + goto out_unlock; > + } [Severity: High] Can a concurrent path modification between dm_devt_from_path() and dm_get_device() still trigger the same slab-use-after-free in pool_ctr() and thin_ctr()? When given a filesystem path rather than major:minor, dm_devt_from_path() calls lookup_bdev(), which resolves the path via kern_path(), reads inode->i_rdev, and immediately drops the path reference via path_put() without pinning the dentry or opening the block device: drivers/md/dm-table.c:dm_devt_from_path() { if (sscanf(path, "%u:%u%c", &major, &minor, &dummy) == 2) { ... } else { r = lookup_bdev(path, &dev); ... } *dev_p = dev; return 0; } Later in pool_ctr() (and similarly in thin_ctr()), dm_get_device() calls dm_devt_from_path() -> lookup_bdev() a second time on each path string: drivers/md/dm-thin.c:pool_ctr() { ... r = dm_get_device(ti, argv[0], metadata_mode, &metadata_dev); ... r = dm_get_device(ti, argv[1], BLK_OPEN_READ | BLK_OPEN_WRITE, &data_dev); } Because no VFS lock or device reference is held across the pre-check and the dm_get_device() calls during dm_ctl_ioctl() -> table_load() -> populate_table() -> dm_table_add_target(), a concurrent symlink flip or block device node rename after the metadata_devt == data_devt (or pool_devt == origin_devt) check passes can cause both dm_get_device() calls to resolve to the same dev_t. When the first dm_get_device() opens the device with BLK_OPEN_READ (metadata_dev in a read-only pool in pool_ctr(), or origin_dev in thin_ctr()) and the second dm_get_device() opens the same dev_t with BLK_OPEN_READ | BLK_OPEN_WRITE, upgrade_mode() in drivers/md/dm-table.c replaces dd->dm_dev and frees old_dev via dm_put_table_device() -> close_table_device() -> kfree(): drivers/md/dm-table.c:upgrade_mode() { 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; } Wouldn't that leave metadata_dev in pool_ctr() or tc->origin_dev in thin_ctr() dangling before __pool_find(), thin target operations, or dm_put_device() dereference it? [ ... ] > @@ -4204,12 +4222,30 @@ static int thin_ctr(struct dm_target *ti, unsigned > int argc, char **argv) > tc->sort_bio_list = RB_ROOT; > > if (argc == 3) { > + dev_t pool_devt; > + dev_t origin_devt; > if (!strcmp(argv[0], argv[2])) { > ti->error = "Error setting origin device"; > r = -EINVAL; > goto bad_origin_dev; > } > > + r = dm_devt_from_path(argv[0], &pool_devt); > + if (r) { > + ti->error = "Error opening metadata block device"; > + goto out_unlock; > + } > + r = dm_devt_from_path(argv[2], &origin_devt); > + if (r) { > + ti->error = "Error opening metadata data device"; > + goto out_unlock; > + } > + if (pool_devt == origin_devt) { > + ti->error = "Error setting metadata or data device"; [Severity: Low] Are these ti->error strings in thin_ctr() and pool_ctr() copy-pasted from the metadata check? In thin_ctr() when argc == 3, argv[0] is the pool device and argv[2] is the origin device (where the surrounding checks use "Error setting origin device", "Error opening origin device", and "Error opening pool device"), so reporting metadata and data device errors when dm_devt_from_path(argv[0], &pool_devt) fails, dm_devt_from_path(argv[2], &origin_devt) fails, or pool_devt == origin_devt looks mismatched. Similarly, in pool_ctr() when dm_devt_from_path(argv[1], &data_devt) fails on the data device argv[1], ti->error is set to "Error opening metadata data device". > + r = -EINVAL; > + goto out_unlock; > + } [Severity: Medium] Does jumping to out_unlock in these three new error paths in thin_ctr() leak tc? In drivers/md/dm-thin.c, thin_ctr() allocates tc before entering the argc == 3 block: drivers/md/dm-thin.c:thin_ctr() { ... tc = ti->private = kzalloc_obj(*tc); if (!tc) { ti->error = "Out of memory"; r = -ENOMEM; goto out_unlock; } ... if (argc == 3) { ... if (!strcmp(argv[0], argv[2])) { ti->error = "Error setting origin device"; r = -EINVAL; goto bad_origin_dev; } ... } ... bad_origin_dev: kfree(tc); out_unlock: mutex_unlock(&dm_thin_pool_table.mutex); return r; } When dm_ctl_ioctl() -> table_load() -> populate_table() -> dm_table_add_target() -> thin_ctr() is called with argc == 3 and dm_devt_from_path(argv[0], &pool_devt) fails, dm_devt_from_path(argv[2], &origin_devt) fails, or pool_devt == origin_devt, jumping to out_unlock bypasses bad_origin_dev where kfree(tc) is called, and dm_table_add_target() does not invoke thin_dtr() on constructor failure. Should these three error paths jump to bad_origin_dev instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
