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

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

[Mohammad Pishdar]

pool_ctr() checks that the metadata and data devices differ by comparing
the two argument strings. Two different strings can name the same block
device, for example "/dev/ram1" and "1:1". The check is then bypassed.

With the "read_only" feature the metadata device is opened with
BLK_OPEN_READ only, while the data device is requested with
BLK_OPEN_READ | BLK_OPEN_WRITE. dm_get_device() finds the already
present entry by dev_t, sees that the mode differs and calls
upgrade_mode(). That opens a new table device, points dd->dm_dev at it
and releases the old one. pool_ctr() however still holds the old
struct dm_dev in metadata_dev. When pool_ctr() later fails,
dm_put_device(ti, metadata_dev) does not find the stale pointer in the
table's device list and reads d->name from the freed table_device to
print an error:

  BUG: KASAN: slab-use-after-free in string_nocheck lib/vsprintf.c:650
  Read of size 1
  dm_put_device+... drivers/md/dm-table.c:467
  pool_ctr+... drivers/md/dm-thin.c:3431
  Freed by:  close_table_device <- dm_put_table_device <- upgrade_mode
             <- dm_get_device <- pool_ctr
  Allocated by: open_table_device <- dm_get_table_device

Resolve both paths with dm_devt_from_path() and reject the table if they
refer to the same device. If either path cannot be resolved, keep the
existing behaviour and let dm_get_device() report the error.

This only fixes pool_ctr(). Any target that calls dm_get_device() twice
for the same device with different modes while keeping the first result
can hit the same stale pointer through upgrade_mode(); that would need a
change in the dm core and is not attempted here.

Testing: on v7.4-rc1 built with KASAN, in QEMU (software emulation), the
syzbot C reproducer triggers the slab-use-after-free before this patch.
After the patch the table load is rejected with -EINVAL ("Error setting
metadata or data device") and there is no KASAN report. A regular
thin-pool with two different devices still loads, and both a
"/dev/ramN" + "major:minor" spelling of the same device and two identical
strings are rejected with -EINVAL. The kernel config was generated from
x86_64_defconfig plus KASAN and dm-thin, not taken from syzbot.

Fixes: 86f1152b117a ("dm: allow active and inactive tables to share dm_devs")
Reported-by: syzbot+9519233e6d144970bbc2@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9519233e6d144970bbc2

Tooling: the root cause analysis, this patch and the test scripts were
drafted with an AI assistant (Claude), based on the syzbot report and the
reproducer. The tests described above were run by the submitter.

Signed-off-by: Mohammad Pishdar <mohamadpishdar@gmail.com>
---
 drivers/md/dm-thin.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
index 358ed77ff..6285bbc8c 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;
-- 
2.34.1

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

[sashiko-bot]

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