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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=83827f9a59abe148bff4
> > Fixes: 991d9fa02da0 ("dm: add thin provisioning target")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Rahul Rao <shreeramrahul94@xxxxxxxxx>
> > ---
> >  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
>
>