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

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

[Rahul Rao]

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.

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

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

[Benjamin Marzinski]

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

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

[Ming Hung Tsai]

On Wed, Oct 28, 2026 at 1:10 PM Benjamin Marzinski <bmarzins@redhat.com> wrote:
>
> 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

Agreed. Fixing in dm_get_device() looks like the right direction. A
possible approach is to avoid upgrade_mode() and allow a
dm_dev_internal to hold multiple references to table_devices of
different modes. This way we can fix dm-thin by simply comparing the
block_device pointers in pool_ctr() and thin_ctr() after
dm_get_device() returns, and reject the table if they alias.

dm-snapshot already has the block_device comparison, so once
dm_get_device() is fixed it is covered too.

I have a rough WIP that seems to confirm the idea, though the approach
still needs refinement.

>
> >
> > 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
>
>

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

[Benjamin Marzinski]

On Sat, Oct 31, 2026 at 02:47:44AM +0800, Ming Hung Tsai wrote:
> On Wed, Oct 28, 2026 at 1:10 PM Benjamin Marzinski <bmarzins@redhat.com> wrote:
> >
> > 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
> 
> Agreed. Fixing in dm_get_device() looks like the right direction. A
> possible approach is to avoid upgrade_mode() and allow a
> dm_dev_internal to hold multiple references to table_devices of
> different modes. This way we can fix dm-thin by simply comparing the
> block_device pointers in pool_ctr() and thin_ctr() after
> dm_get_device() returns, and reject the table if they alias.
> 
> dm-snapshot already has the block_device comparison, so once
> dm_get_device() is fixed it is covered too.
> 
> I have a rough WIP that seems to confirm the idea, though the approach
> still needs refinement.

The big issue I see is the dm_table_get_devices() callers. They
currently assume that they are getting a list of devices with no
duplicates. I'm pretty sure that we'll need to add a flag to
dm_dev_internal so that we can mark duplicates, and we probably need to
always mark the read-only device as the duplicate. Then all the callers
of dm_table_get_devices() need to skip the duplicate devices.

-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
> >
> >