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: <1362263724.1334.18.camel@x61.thuisdomein>
Date:	Sat, 02 Mar 2013 23:35:24 +0100
From:	Paul Bolle <pebolle@...cali.nl>
To:	Ben Hutchings <ben@...adent.org.uk>
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 19:48 +0000, Ben Hutchings wrote:
> 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.

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? 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?


Paul Bolle

--
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/