[dm-devel] [PATCH 1/2] Fail I/O to thin pool devices

[Joe Thornber]

Nack.

I don't see the security issue; how is this any different from running the
thin tools on any incorrect device?  Or even the data device that the pool
is mirroring.  In general the thin tools don't modify the metadata they're
running on.  If you know of a security issue with the thin tools please let
me know.


On Tue, Feb 7, 2023 at 7:56 AM Demi Marie Obenour <
demi at invisiblethingslab.com> wrote:

> A thin pool device currently just passes all I/O to its origin device,
> but this is a footgun: the user might not realize that tools that
> operate on thin pool metadata must operate on the metadata volume.  This
> could have security implications.
>
> Fix this by failing all I/O to thin pool devices.
>
> Signed-off-by: Demi Marie Obenour <demi at invisiblethingslab.com>
> ---
>  drivers/md/dm-thin.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index
> 64cfcf46881dc5d87d5dfdb5650ba9babd32cd31..d85fdbd782ae5426003c99a4b4bf53818cc85efa
> 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -3405,19 +3405,14 @@ static int pool_ctr(struct dm_target *ti, unsigned
> argc, char **argv)
>
>  static int pool_map(struct dm_target *ti, struct bio *bio)
>  {
> -       int r;
> -       struct pool_c *pt = ti->private;
> -       struct pool *pool = pt->pool;
> -
>         /*
> -        * As this is a singleton target, ti->begin is always zero.
> +        * Previously, access to the pool was passed down to the origin
> device.
> +        * However, this turns out to be error-prone: if the user runs any
> of
> +        * the thin tools on the pool device, the tools could wind up
> parsing
> +        * potentially attacker-controlled data.  This mistake has actually
> +        * happened in practice.  Therefore, fail all I/O on the pool
> device.
>          */
> -       spin_lock_irq(&pool->lock);
> -       bio_set_dev(bio, pt->data_dev->bdev);
> -       r = DM_MAPIO_REMAPPED;
> -       spin_unlock_irq(&pool->lock);
> -
> -       return r;
> +       return -EIO;
>  }
>
>  static int maybe_resize_data_dev(struct dm_target *ti, bool *need_commit)
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
>
> --
> dm-devel mailing list
> dm-devel at redhat.com
> https://listman.redhat.com/mailman/listinfo/dm-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20230207/0db600e7/attachment.htm>