Re: [PATCH] dm: reject aliased metadata/data and pool/origin devices

[sashiko-bot@kernel.org]

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

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