Xen project Mailing List

Re: [Xen-devel] [PATCH] xen-blkback: do not leak mode property

From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Date: Fri, 11 Jan 2013 12:36:52 -0500

Cc: Jens Axboe <axboe@xxxxxxxxx>, Olaf Hering <olaf@xxxxxxxxx>, linux-kernel@xxxxxxxxxxxxxxx, xen-devel <xen-devel@xxxxxxxxxxxxx>

Delivery-date: Fri, 11 Jan 2013 17:37:40 +0000

List-id: Xen developer discussion <xen-devel.lists.xen.org>

On Thu, Dec 20, 2012 at 10:31:11AM +0000, Jan Beulich wrote: > "be->mode" is obtained from xenbus_read(), which does a kmalloc() for > the message body. The short string is never released, so do it along > with freeing "be" itself, and make sure the string isn't kept when > backend_changed() doesn't complete successfully (which made it > desirable to slightly re-structure that function, so that the error > cleanup can be done in one place). applied. > > Reported-by: Olaf Hering <olaf@xxxxxxxxx> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > > --- > drivers/block/xen-blkback/xenbus.c | 49 > ++++++++++++++++++------------------- > 1 file changed, 24 insertions(+), 25 deletions(-) > > --- 3.7/drivers/block/xen-blkback/xenbus.c > +++ 3.7-xen-blkback-mode-leak/drivers/block/xen-blkback/xenbus.c > @@ -366,6 +366,7 @@ static int xen_blkbk_remove(struct xenbu > be->blkif = NULL; > } > > + kfree(be->mode); > kfree(be); > dev_set_drvdata(&dev->dev, NULL); > return 0; > @@ -501,6 +502,7 @@ static void backend_changed(struct xenbu > = container_of(watch, struct backend_info, backend_watch); > struct xenbus_device *dev = be->dev; > int cdrom = 0; > + unsigned long handle; > char *device_type; > > DPRINTK(""); > @@ -520,10 +522,10 @@ static void backend_changed(struct xenbu > return; > } > > - if ((be->major || be->minor) && > - ((be->major != major) || (be->minor != minor))) { > - pr_warn(DRV_PFX "changing physical device (from %x:%x to %x:%x) > not supported.\n", > - be->major, be->minor, major, minor); > + if (be->major | be->minor) { > + if (be->major != major || be->minor != minor) > + pr_warn(DRV_PFX "changing physical device (from %x:%x > to %x:%x) not supported.\n", > + be->major, be->minor, major, minor); > return; > } > > @@ -541,36 +543,33 @@ static void backend_changed(struct xenbu > kfree(device_type); > } > > - if (be->major == 0 && be->minor == 0) { > - /* Front end dir is a number, which is used as the handle. */ > + /* Front end dir is a number, which is used as the handle. */ > + err = strict_strtoul(strrchr(dev->otherend, '/') + 1, 0, &handle); > + if (err) > + return; > > - char *p = strrchr(dev->otherend, '/') + 1; > - long handle; > - err = strict_strtoul(p, 0, &handle); > - if (err) > - return; > + be->major = major; > + be->minor = minor; > > - be->major = major; > - be->minor = minor; > - > - err = xen_vbd_create(be->blkif, handle, major, minor, > - (NULL == strchr(be->mode, 'w')), cdrom); > - if (err) { > - be->major = 0; > - be->minor = 0; > - xenbus_dev_fatal(dev, err, "creating vbd structure"); > - return; > - } > + err = xen_vbd_create(be->blkif, handle, major, minor, > + !strchr(be->mode, 'w'), cdrom); > > + if (err) > + xenbus_dev_fatal(dev, err, "creating vbd structure"); > + else { > err = xenvbd_sysfs_addif(dev); > if (err) { > xen_vbd_free(&be->blkif->vbd); > - be->major = 0; > - be->minor = 0; > xenbus_dev_fatal(dev, err, "creating sysfs entries"); > - return; > } > + } > > + if (err) { > + kfree(be->mode); > + be->mode = NULL; > + be->major = 0; > + be->minor = 0; > + } else { > /* We're potentially connected now */ > xen_update_blkif_status(be->blkif); > } > > > > xen-blkback: do not leak mode property > > "be->mode" is obtained from xenbus_read(), which does a kmalloc() for > the message body. The short string is never released, so do it along > with freeing "be" itself, and make sure the string isn't kept when > backend_changed() doesn't complete successfully (which made it > desirable to slightly re-structure that function, so that the error > cleanup can be done in one place). > > Reported-by: Olaf Hering <olaf@xxxxxxxxx> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > > --- > drivers/block/xen-blkback/xenbus.c | 49 > ++++++++++++++++++------------------- > 1 file changed, 24 insertions(+), 25 deletions(-) > > --- 3.7/drivers/block/xen-blkback/xenbus.c > +++ 3.7-xen-blkback-mode-leak/drivers/block/xen-blkback/xenbus.c > @@ -366,6 +366,7 @@ static int xen_blkbk_remove(struct xenbu > be->blkif = NULL; > } > > + kfree(be->mode); > kfree(be); > dev_set_drvdata(&dev->dev, NULL); > return 0; > @@ -501,6 +502,7 @@ static void backend_changed(struct xenbu > = container_of(watch, struct backend_info, backend_watch); > struct xenbus_device *dev = be->dev; > int cdrom = 0; > + unsigned long handle; > char *device_type; > > DPRINTK(""); > @@ -520,10 +522,10 @@ static void backend_changed(struct xenbu > return; > } > > - if ((be->major || be->minor) && > - ((be->major != major) || (be->minor != minor))) { > - pr_warn(DRV_PFX "changing physical device (from %x:%x to %x:%x) > not supported.\n", > - be->major, be->minor, major, minor); > + if (be->major | be->minor) { > + if (be->major != major || be->minor != minor) > + pr_warn(DRV_PFX "changing physical device (from %x:%x > to %x:%x) not supported.\n", > + be->major, be->minor, major, minor); > return; > } > > @@ -541,36 +543,33 @@ static void backend_changed(struct xenbu > kfree(device_type); > } > > - if (be->major == 0 && be->minor == 0) { > - /* Front end dir is a number, which is used as the handle. */ > + /* Front end dir is a number, which is used as the handle. */ > + err = strict_strtoul(strrchr(dev->otherend, '/') + 1, 0, &handle); > + if (err) > + return; > > - char *p = strrchr(dev->otherend, '/') + 1; > - long handle; > - err = strict_strtoul(p, 0, &handle); > - if (err) > - return; > + be->major = major; > + be->minor = minor; > > - be->major = major; > - be->minor = minor; > - > - err = xen_vbd_create(be->blkif, handle, major, minor, > - (NULL == strchr(be->mode, 'w')), cdrom); > - if (err) { > - be->major = 0; > - be->minor = 0; > - xenbus_dev_fatal(dev, err, "creating vbd structure"); > - return; > - } > + err = xen_vbd_create(be->blkif, handle, major, minor, > + !strchr(be->mode, 'w'), cdrom); > > + if (err) > + xenbus_dev_fatal(dev, err, "creating vbd structure"); > + else { > err = xenvbd_sysfs_addif(dev); > if (err) { > xen_vbd_free(&be->blkif->vbd); > - be->major = 0; > - be->minor = 0; > xenbus_dev_fatal(dev, err, "creating sysfs entries"); > - return; > } > + } > > + if (err) { > + kfree(be->mode); > + be->mode = NULL; > + be->major = 0; > + be->minor = 0; > + } else { > /* We're potentially connected now */ > xen_update_blkif_status(be->blkif); > } _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.