Re: [PATCH] dm thin: reject metadata and data devices that resolve to the same device

[sashiko-bot@kernel.org]

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 <mohamadpishdar@gmail.com>

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/20261103193956.63354-1-mohamadpishdar@gmail.com?part=1