Re: [PATCH] dm thin: prevent use-after-free when metadata and data devices alias

[Benjamin Marzinski <bmarzins@redhat.com>]

On Mon, Oct 26, 2026 at 04:34:02PM +0530, Rahul Rao wrote:
> pool_ctr() takes separate specification strings for the metadata and data
> devices. However, different strings (or format representations such as
> /dev/ram0 and 1:0) can resolve to the same underlying block device.
> 
> When the read_only feature is requested, the metadata device is opened
> read-only before the data device is opened read-write. If the devices
> alias, the second dm_get_device() call upgrades the existing device's
> mode. upgrade_mode() replaces the dm_dev and drops the old table device,
> leaving pool_ctr() with a stale metadata_dev pointer. The subsequent
> metadata_dev->bdev access triggers a KASAN use-after-free report.
> 
> Acquire the read-write data device first so that acquiring the metadata
> device cannot trigger this mode upgrade. Then reject matching block
> devices, including those specified through different names, and release
> the devices in reverse acquisition order on error.

What this patch fixes looks good to me, but there is still an issue
here. There is a similar problem in thin_ctr() if the pool and origin
device name strings are different but they alias. Unfortunately, this
style of fix doesn't work as well there. It is only a valid fix for a
thin-pool target because that is a DM_TARGET_SINGLETON. That means there
can't be another target that's part of the same table. This isn't the
case with the thin target. There can be other targets in the table, and
you can't force correct dm_get_device() ordering between them. Like
Mikulas said about a similar issue with dm-snapshot, this is probably
something that is better fixed in dm_get_device() than in its callers.

-Ben

> 
> Reported-by: syzbot+83827f9a59abe148bff4@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=83827f9a59abe148bff4
> Fixes: 991d9fa02da0 ("dm: add thin provisioning target")
> Cc: stable@vger.kernel.org
> Signed-off-by: Rahul Rao <shreeramrahul94@gmail.com>
> ---
>  drivers/md/dm-thin.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index 59392de7a477..52e47874048d 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -3326,18 +3326,24 @@ static int pool_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  
>  	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);
> +	r = dm_get_device(ti, argv[1], BLK_OPEN_READ | BLK_OPEN_WRITE, &data_dev);
>  	if (r) {
> -		ti->error = "Error opening metadata block device";
> +		ti->error = "Error getting data 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);
> +	r = dm_get_device(ti, argv[0], metadata_mode, &metadata_dev);
>  	if (r) {
> -		ti->error = "Error getting data device";
> -		goto out_metadata;
> +		ti->error = "Error opening metadata block device";
> +		goto out_data;
> +	}
> +
> +	if (metadata_dev->bdev == data_dev->bdev) {
> +		ti->error = "Metadata and data devices must be different";
> +		r = -EINVAL;
> +		goto out;
>  	}
> +	warn_if_metadata_device_too_big(metadata_dev->bdev);
>  
>  	if (kstrtoul(argv[2], 10, &block_size) || !block_size ||
>  	    block_size < DATA_DEV_BLOCK_SIZE_MIN_SECTORS ||
> @@ -3426,9 +3432,9 @@ static int pool_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  out_free_pt:
>  	kfree(pt);
>  out:
> -	dm_put_device(ti, data_dev);
> -out_metadata:
>  	dm_put_device(ti, metadata_dev);
> +out_data:
> +	dm_put_device(ti, data_dev);
>  out_unlock:
>  	mutex_unlock(&dm_thin_pool_table.mutex);
>  
> -- 
> 2.43.0