| Message ID | 20230226195227.185393-1-idryomov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rbd: avoid use-after-free in do_rbd_add() when rbd_dev_create() fails | expand |
On Sun, Feb 26, 2023 at 8:52 PM Ilya Dryomov <idryomov@gmail.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@fintech.ru>. > > Cc: stable@vger.kernel.org > Fixes: 1643dfa4c2c8 ("rbd: introduce a per-device ordered workqueue") > Signed-off-by: Ilya Dryomov <idryomov@gmail.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...
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);
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@fintech.ru>. Cc: stable@vger.kernel.org Fixes: 1643dfa4c2c8 ("rbd: introduce a per-device ordered workqueue") Signed-off-by: Ilya Dryomov <idryomov@gmail.com> --- drivers/block/rbd.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)