linux-kernel - Re: [ 34/77] xen/blkback: Dont trust the handle from the frontend.

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: <1362253682.3768.136.camel@deadeye.wl.decadent.org.uk>
Date:	Sat, 02 Mar 2013 19:48:02 +0000
From:	Ben Hutchings <ben@...adent.org.uk>
To:	Paul Bolle <pebolle@...cali.nl>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org,
	Jan Beulich <jbeulich@...e.com>,
	Ian Campbell <ian.campbell@...rix.com>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
Subject: Re: [ 34/77] xen/blkback: Dont trust the handle from the frontend.

On Fri, 2013-03-01 at 22:12 +0100, Paul Bolle wrote:
> On Fri, 2013-03-01 at 11:44 -0800, Greg Kroah-Hartman wrote:
> > 3.8-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> > 
> > commit 01c681d4c70d64cb72142a2823f27c4146a02e63 upstream.
> > 
> > The 'handle' is the device that the request is from. For the life-time
> > of the ring we copy it from a request to a response so that the frontend
> > is not surprised by it. But we do not need it - when we start processing
> > I/Os we have our own 'struct phys_req' which has only most essential
> > information about the request. In fact the 'vbd_translate' ends up
> > over-writing the preq.dev with a value from the backend.
> 
> Unless that call to vb_translate() fails, doesn't it? Wouldn't preq.dev
> still contain random data in that case?
> 
> > This assignment of preq.dev with the 'handle' value is superfluous
> > so lets not do it.
> > 
> > Acked-by: Jan Beulich <jbeulich@...e.com>
> > Acked-by: Ian Campbell <ian.campbell@...rix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > 
> > ---
> >  drivers/block/xen-blkback/blkback.c |    1 -
> >  1 file changed, 1 deletion(-)
> > 
> > --- a/drivers/block/xen-blkback/blkback.c
> > +++ b/drivers/block/xen-blkback/blkback.c
> > @@ -879,7 +879,6 @@ static int dispatch_rw_block_io(struct x
> >  		goto fail_response;
> >  	}
> >  
> > -	preq.dev           = req->u.rw.handle;
> >  	preq.sector_number = req->u.rw.sector_number;
> >  	preq.nr_sects      = 0;
> >  
> 
> This introduces a new GCC warning in the stable 3.8.y tree:
>     drivers/block/xen-blkback/blkback.c: In function 'dispatch_rw_block_io':
>     drivers/block/xen-blkback/blkback.c:904:3: warning: 'preq.dev' may be used uninitialized in this function [-Wuninitialized]
> 
> It does look GCC is right here. But I'm totally new to the code in
> question, so I'll just ask whether this can really go in stable as is.

When gcc compiles something like this:

static int foo(int *p)
{
	if (rand() & 1)
		return -1;
	*p = 0;
	return 0;
}

int bar(void)
{
	int i;
	if (foo(&i) < 0)
		return 1;
	return i;
}

and inlines foo() into bar(), sometimes it fails to recognise that i
will definitely be initialised before use.  This simple example seems to
be OK but more complex functions such as these will often trigger this
warning.  The warning is really quite useless now.

Ben.

-- 
Ben Hutchings
Computers are not intelligent.	They only think they are.

Download attachment "signature.asc" of type "application/pgp-signature" (829 bytes)