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

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

[Drif Abdelmalek Mohamed Said]

dm_get_device() identifies devices by their resolved dev_t, but pool_ctr()
and thin_ctr() only reject identical argument strings.  Different paths
such as "/dev/ram0" and "1:0" therefore bypass the check and select the
same dm_dev_internal.

syzbot reported a slab-use-after-free in pool_ctr() when a read-only pool
used such aliased metadata and data devices.  Opening the metadata device
read-only first and the data device read-write second made upgrade_mode()
replace dd->dm_dev and free the old table_device, leaving metadata_dev
dangling before __pool_find() dereferenced metadata_dev->bdev.

syzbot reported:

BUG: KASAN: slab-use-after-free in pool_ctr+0x874/0xd60 drivers/md/dm-thin.c:3363
Read of size 8 at addr ffff8880340cc698 by task syz.0.17/5507

Call Trace:
 <TASK>
 dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
 print_address_description+0x55/0x1e0 mm/kasan/report.c:378
 print_report+0x58/0x70 mm/kasan/report.c:482
 kasan_report+0x117/0x150 mm/kasan/report.c:595
 pool_ctr+0x874/0xd60 drivers/md/dm-thin.c:3363
 dm_table_add_target+0x58d/0xc70 drivers/md/dm-table.c:749
 populate_table drivers/md/dm-ioctl.c:1602 [inline]
 table_load+0x325/0x8a0 drivers/md/dm-ioctl.c:1645
 ctl_ioctl drivers/md/dm-ioctl.c:2221 [inline]
 dm_ctl_ioctl+0x9ec/0xe10 drivers/md/dm-ioctl.c:2243
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:597 [inline]
 __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583
 do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
 do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

 Allocated by task 5507:
 kasan_save_stack mm/kasan/common.c:57 [inline]
 kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
 poison_kmalloc_redzone mm/kasan/common.c:409 [inline]
 __kasan_kmalloc+0x93/0xb0 mm/kasan/common.c:426
 kasan_kmalloc include/linux/kasan.h:263 [inline]
 __kmalloc_cache_node_noprof+0x3cc/0x620 mm/slub.c:5582
 _kmalloc_node_noprof include/linux/slab.h:1196 [inline]
 open_table_device drivers/md/dm.c:733 [inline]
 dm_get_table_device+0x23e/0x730 drivers/md/dm.c:821
 dm_get_device+0x327/0x760 drivers/md/dm-table.c:388
 pool_ctr+0x4ae/0xd60 drivers/md/dm-thin.c:3329
 dm_table_add_target+0x58d/0xc70 drivers/md/dm-table.c:749
 populate_table drivers/md/dm-ioctl.c:1602 [inline]
 table_load+0x325/0x8a0 drivers/md/dm-ioctl.c:1645
 ctl_ioctl drivers/md/dm-ioctl.c:2221 [inline]
 dm_ctl_ioctl+0x9ec/0xe10 drivers/md/dm-ioctl.c:2243
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:597 [inline]
 __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583
 do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
 do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Freed by task 5507:
 kasan_save_stack mm/kasan/common.c:57 [inline]
 kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
 kasan_save_free_info+0x40/0x50 mm/kasan/generic.c:584
 poison_slab_object mm/kasan/common.c:264 [inline]
 __kasan_slab_free+0x5c/0x80 mm/kasan/common.c:296
 kasan_slab_free include/linux/kasan.h:235 [inline]
 slab_free_hook mm/slub.c:2748 [inline]
 slab_free mm/slub.c:6509 [inline]
 kfree+0x1c5/0x650 mm/slub.c:6802
 close_table_device drivers/md/dm.c:798 [inline]
 dm_put_table_device+0x255/0x2a0 drivers/md/dm.c:841
 upgrade_mode drivers/md/dm-table.c:326 [inline]
 dm_get_device+0x56f/0x760 drivers/md/dm-table.c:399
 pool_ctr+0x585/0xd60 drivers/md/dm-thin.c:3336
 dm_table_add_target+0x58d/0xc70 drivers/md/dm-table.c:749
 populate_table drivers/md/dm-ioctl.c:1602 [inline]
 table_load+0x325/0x8a0 drivers/md/dm-ioctl.c:1645
 ctl_ioctl drivers/md/dm-ioctl.c:2221 [inline]
 dm_ctl_ioctl+0x9ec/0xe10 drivers/md/dm-ioctl.c:2243
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:597 [inline]
 __se_sys_ioctl+0xfc/0x170 fs/ioctl.c:583
 do_syscall_x64 arch/x86/entry/syscall_64.c:61 [inline]
 do_syscall_64+0x166/0x520 arch/x86/entry/syscall_64.c:84
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

The buggy address belongs to the object at ffff8880340cc680
 which belongs to the cache kmalloc-96 of size 96
The buggy address is located 24 bytes inside of
 freed 96-byte region [ffff8880340cc680, ffff8880340cc6e0)

Validate the resolved device numbers with dm_devt_from_path() before either
device is opened, keeping the existing string comparison.  Do this in both
pool_ctr() and thin_ctr(), which has the same problem for the pool and
optional origin devices.  Checking the device identity up front avoids the
mode upgrade entirely and is preferable to switching the dm_get_device()
call order, which would rely on the data mode being a superset of the
metadata mode and would only cover pool_ctr().

Reported-by: syzbot+83827f9a59abe148bff4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=83827f9a59abe148bff4
Fixes: 70de2cbda8a5 ("dm thin: add sanity checks to thin-pool and external snapshot creation")
Cc: stable@vger.kernel.org
Signed-off-by: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>
---
 drivers/md/dm-thin.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 358ed77ffb2b..281b2e1745c3 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;
+	}
+
 	/*
 	 * Set default pool features.
 	 */
@@ -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";
+			r = -EINVAL;
+			goto out_unlock;
+		}
+
 		r = dm_get_device(ti, argv[2], BLK_OPEN_READ, &origin_dev);
 		if (r) {
 			ti->error = "Error opening origin device";
-- 
2.43.0

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

[sashiko-bot]

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