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

[Konrad Rzeszutek Wilk]

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);
>  	}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/