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: <1362265843.3768.162.camel@deadeye.wl.decadent.org.uk>
Date:	Sat, 02 Mar 2013 23:10:43 +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 Sat, 2013-03-02 at 23:35 +0100, Paul Bolle wrote:
[...]
> 0) I've had another look at the relevant code in v3.8.2-rc1. It can be
> summarized like this:
> 
> static int xen_vbd_translate()
> {
> 	[...]
> 	int rc = -EACCES;
> 
> 	if ([...])
> 		goto out;
> 	[...]
> 
> 	[p]req->dev  = vbd->pdevice;
> 	[p]req->bdev = vbd->bdev;
> 	[...]
> 
>  out:
> 	return rc;
> }
> 
> static int dispatch_rw_block_io()
> {
> 	struct phys_req preq;
> 	[...]
> 
> 	preq.sector_number = req->u.rw.sector_number;
> 	preq.nr_sects      = 0;
> 	[...]
> 
> 	for ([...]) {
> 		[...]
> 		preq.nr_sects += seg[i].nsec;
> 	}
> 
> 	if (xen_vbd_translate(&preq, blkif, operation) != 0) {
> 		pr_debug(DRV_PFX "access denied: %s of [%llu,%llu] on dev=%04x\n",
> 			 operation == READ ? "read" : "write",
> 			 preq.sector_number,
> 			 preq.sector_number + preq.nr_sects, preq.dev);
> 		goto [...];
> 	}
> 	[...]
> }
> 
> 1) So if xen_vbd_translate() fails, it can return before setting
> preq.dev. That makes the call of pr_debug() use an uninitialized value,
> doesn't it?

Oh yes, so it's a completely valid warning in this case!

> Does inlining xen_vbd_translate() and/or
> dispatch_rw_block_io() generate code were that can't happen anymore?
> (Both functions being static they probably are inlined.)
> 
> 2) And even if inlining does generate code where this can't happen,
> isn't it enough that preq.dev can be used uninitialized if no code were
> inlined?

If gcc inlines a function call, it analyses data flow between the two
functions.  Otherwise it assumes that the called function will
initialise any variable it's given a pointer to, and the warning doesn't
appear.  (That's my experience, anyway.)

Ben.

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

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