linux-kernel - Re: [PATCH] rbd: avoid use-after-free in do_rbd_add() when rbd_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAOi1vP9S_y_vWk8jt=cV-TExEwa+vfM3tSPhYdULZhjrFVY3tw@mail.gmail.com>
Date:   Sun, 26 Feb 2023 21:04:13 +0100
From:   Ilya Dryomov <idryomov@...il.com>
To:     ceph-devel@...r.kernel.org
Cc:     Natalia Petrova <n.petrova@...tech.ru>,
        Dongsheng Yang <dongsheng.yang@...ystack.cn>,
        Alexey Khoroshilov <khoroshilov@...ras.ru>,
        Jens Axboe <axboe@...nel.dk>, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org,
        Nikita Zhandarovich <n.zhandarovich@...tech.ru>
Subject: Re: [PATCH] rbd: avoid use-after-free in do_rbd_add() when
 rbd_dev_create() fails

On Sun, Feb 26, 2023 at 8:52 PM Ilya Dryomov <idryomov@...il.com> wrote:
>
> If getting an ID or setting up a work queue in rbd_dev_create() fails,
> use-after-free on rbd_dev->rbd_client, rbd_dev->spec and rbd_dev->opts
> is triggered in do_rbd_add().  The root cause is that the ownership of
> these structures is transfered to rbd_dev prematurely and they all end
> up getting freed when rbd_dev_create() calls rbd_dev_free() prior to
> returning to do_rbd_add().
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE, an
> incomplete patch submitted by Natalia Petrova <n.petrova@...tech.ru>.
>
> Cc: stable@...r.kernel.org
> Fixes: 1643dfa4c2c8 ("rbd: introduce a per-device ordered workqueue")
> Signed-off-by: Ilya Dryomov <idryomov@...il.com>
> ---
>  drivers/block/rbd.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c
> index 04453f4a319c..60aed196a2e5 100644
> --- a/drivers/block/rbd.c
> +++ b/drivers/block/rbd.c
> @@ -5292,8 +5292,7 @@ static void rbd_dev_release(struct device *dev)
>                 module_put(THIS_MODULE);
>  }
>
> -static struct rbd_device *__rbd_dev_create(struct rbd_client *rbdc,
> -                                          struct rbd_spec *spec)
> +static struct rbd_device *__rbd_dev_create(struct rbd_spec *spec)
>  {
>         struct rbd_device *rbd_dev;
>
> @@ -5338,9 +5337,6 @@ static struct rbd_device *__rbd_dev_create(struct rbd_client *rbdc,
>         rbd_dev->dev.parent = &rbd_root_dev;
>         device_initialize(&rbd_dev->dev);
>
> -       rbd_dev->rbd_client = rbdc;
> -       rbd_dev->spec = spec;
> -
>         return rbd_dev;
>  }
>
> @@ -5353,12 +5349,10 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
>  {
>         struct rbd_device *rbd_dev;
>
> -       rbd_dev = __rbd_dev_create(rbdc, spec);
> +       rbd_dev = __rbd_dev_create(spec);
>         if (!rbd_dev)
>                 return NULL;
>
> -       rbd_dev->opts = opts;
> -
>         /* get an id and fill in device name */
>         rbd_dev->dev_id = ida_simple_get(&rbd_dev_id_ida, 0,
>                                          minor_to_rbd_dev_id(1 << MINORBITS),
> @@ -5375,6 +5369,10 @@ static struct rbd_device *rbd_dev_create(struct rbd_client *rbdc,
>         /* we have a ref from do_rbd_add() */
>         __module_get(THIS_MODULE);
>
> +       rbd_dev->rbd_client = rbdc;
> +       rbd_dev->spec = spec;
> +       rbd_dev->opts = opts;
> +
>         dout("%s rbd_dev %p dev_id %d\n", __func__, rbd_dev, rbd_dev->dev_id);
>         return rbd_dev;
>
> @@ -6736,7 +6734,7 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>                 goto out_err;
>         }
>
> -       parent = __rbd_dev_create(rbd_dev->rbd_client, rbd_dev->parent_spec);
> +       parent = __rbd_dev_create(rbd_dev->parent_spec);
>         if (!parent) {
>                 ret = -ENOMEM;
>                 goto out_err;
> @@ -6746,8 +6744,8 @@ static int rbd_dev_probe_parent(struct rbd_device *rbd_dev, int depth)
>          * Images related by parent/child relationships always share
>          * rbd_client and spec/parent_spec, so bump their refcounts.
>          */
> -       __rbd_get_client(rbd_dev->rbd_client);
> -       rbd_spec_get(rbd_dev->parent_spec);
> +       parent->rbd_client = __rbd_get_client(rbd_dev->rbd_client);
> +       parent->spec = rbd_spec_get(rbd_dev->parent_spec);
>
>         __set_bit(RBD_DEV_FLAG_READONLY, &parent->flags);
>
> --
> 2.39.1
>

CCing everyone on the previous thread...