linux-kernel - Re: [PATCH] xen/blkback: prevent leak of mode during multiple backend

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <50BF2E3802000078000AE162@nat28.tlf.novell.com>
Date:	Wed, 05 Dec 2012 10:21:28 +0000
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Olaf Hering" <olaf@...fle.de>, <konrad.wilk@...cle.com>
Cc:	<xen-devel@...ts.xen.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] xen/blkback: prevent leak of mode during multiple
 backend_changed calls

>>> On 05.12.12 at 11:01, Olaf Hering <olaf@...fle.de> wrote:
> backend_changed might be called multiple times, which will leak
> be->mode. free the previous value before storing the current mode value.

As said before - this is one possible route to take. But did you
consider at all the alternative of preventing the function from
getting called more than once for a given device? As also said
before, I think that would have other bad effects, and hence
should be preferred (and would likely also result in a smaller
patch).

And _if_ this here is the route to go, I'd clearly see this and your
earlier patch to be folded into just one (dealing with both leaks
in one go).

Jan

> Signed-off-by: Olaf Hering <olaf@...fle.de>
> ---
> 
> !! Not compile tested !!
> 
>  drivers/block/xen-blkback/xenbus.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c 
> b/drivers/block/xen-blkback/xenbus.c
> index a6585a4..8650b19 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -502,7 +502,7 @@ static void backend_changed(struct xenbus_watch *watch,
>  		= container_of(watch, struct backend_info, backend_watch);
>  	struct xenbus_device *dev = be->dev;
>  	int cdrom = 0;
> -	char *device_type;
> +	char *mode, *device_type;
>  
>  	DPRINTK("");
>  
> @@ -528,13 +528,15 @@ static void backend_changed(struct xenbus_watch *watch,
>  		return;
>  	}
>  
> -	be->mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL);
> -	if (IS_ERR(be->mode)) {
> -		err = PTR_ERR(be->mode);
> +	mode = xenbus_read(XBT_NIL, dev->nodename, "mode", NULL);
> +	kfree(be->mode);
> +	if (IS_ERR(mode)) {
> +		err = PTR_ERR(mode);
>  		be->mode = NULL;
>  		xenbus_dev_fatal(dev, err, "reading mode");
>  		return;
>  	}
> +	be->mode = mode;
>  
>  	device_type = xenbus_read(XBT_NIL, dev->otherend, "device-type", NULL);
>  	if (!IS_ERR(device_type)) {
> @@ -555,7 +557,7 @@ static void backend_changed(struct xenbus_watch *watch,
>  		be->minor = minor;
>  
>  		err = xen_vbd_create(be->blkif, handle, major, minor,
> -				 (NULL == strchr(be->mode, 'w')), cdrom);
> +				 (NULL == strchr(mode, 'w')), cdrom);
>  		if (err) {
>  			be->major = 0;
>  			be->minor = 0;
> -- 
> 1.8.0.1



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