Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] dm thin: TOCTOU race in pool_ctr() device check still allows UAF --
commit 5ead52d14677a9859b05ed7a484ff22ce5ff6bca Author: Mohammad Pishdar <[email protected]> dm thin: reject metadata and data devices that resolve to the same device This commit updates pool_ctr() in drivers/md/dm-thin.c to resolve the metadata and data device paths via dm_devt_from_path() and reject the table if both paths resolve to the same dev_t, aiming to prevent dm_get_device() from calling upgrade_mode() and freeing metadata_dev while it is still held. > diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c > index 358ed77ffb2be..6285bbc8c746d 100644 > --- a/drivers/md/dm-thin.c > +++ b/drivers/md/dm-thin.c > @@ -3292,6 +3292,7 @@ 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, data_devt; > > /* > * FIXME Remove validation from scope of lock. > @@ -3307,8 +3308,17 @@ static int pool_ctr(struct dm_target *ti, unsigned int > argc, char **argv) > as.argc = argc; > as.argv = argv; > > - /* make sure metadata and data are different devices */ > - if (!strcmp(argv[0], argv[1])) { > + /* > + * Make sure metadata and data are different devices. Comparing the > + * strings is not enough: "/dev/foo" and "major:minor" can name the > + * same device, which would make dm_get_device() upgrade the open > + * mode of the already acquired metadata device and free the > + * struct dm_dev we are still holding. > + */ > + if (!strcmp(argv[0], argv[1]) || > + (!dm_devt_from_path(argv[0], &metadata_devt) && > + !dm_devt_from_path(argv[1], &data_devt) && > + metadata_devt == data_devt)) { > ti->error = "Error setting metadata or data device"; > r = -EINVAL; > goto out_unlock; [Severity: High] Can a concurrent symlink or device node change between this pre-check and the subsequent dm_get_device() calls still trigger the use-after-free in dm_put_device()? For filesystem paths, dm_devt_from_path() calls lookup_bdev(), which resolves the path via kern_path(), reads inode->i_rdev, and drops the path reference via path_put() without pinning the dentry or block device. In addition, if either dm_devt_from_path() call returns an error here (for instance -ENOENT if a symlink or device node does not exist yet), the condition evaluates to false and execution continues. When pool_ctr() later calls dm_get_device() for metadata_dev and data_dev: drivers/md/dm-thin.c:pool_ctr() { ... metadata_mode = BLK_OPEN_READ | ((pf.mode == PM_READ_ONLY) ? 0 : BLK_OPEN_WRITE); r = dm_get_device(ti, argv[0], metadata_mode, &metadata_dev); if (r) { ti->error = "Error opening metadata block device"; goto out_unlock; } warn_if_metadata_device_too_big(metadata_dev->bdev); r = dm_get_device(ti, argv[1], BLK_OPEN_READ | BLK_OPEN_WRITE, &data_dev); ... } each dm_get_device() call invokes dm_devt_from_path() a second time. If argv[0] or argv[1] is created or modified in that window so that both now resolve to the same dev_t: - When pf.mode is PM_READ_ONLY, metadata_dev is opened with BLK_OPEN_READ and data_dev is opened with BLK_OPEN_READ | BLK_OPEN_WRITE, causing the second dm_get_device() call to invoke upgrade_mode() and free the struct dm_dev still held in metadata_dev, followed by the same slab use-after-free on metadata_dev->name in dm_put_device(ti, metadata_dev) when pool_ctr() fails. - When pf.mode is not PM_READ_ONLY, pool_ctr() has no check after opening that metadata_dev->bdev != data_dev->bdev, allowing a thin-pool to be created with metadata_dev and data_dev aliasing the same block device. Would opening data_dev (which always requests BLK_OPEN_READ | BLK_OPEN_WRITE) before metadata_dev and checking metadata_dev->bdev == data_dev->bdev after both dm_get_device() calls avoid both upgrade_mode() and the double path lookup race? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
