From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R429R-0000nV-QB for qemu-devel@nongnu.org; Wed, 14 Sep 2011 22:58:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R429Q-0007Zw-MK for qemu-devel@nongnu.org; Wed, 14 Sep 2011 22:58:09 -0400 Received: from ozlabs.org ([203.10.76.45]:34360) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R429Q-0007Yu-Cz for qemu-devel@nongnu.org; Wed, 14 Sep 2011 22:58:08 -0400 Date: Thu, 15 Sep 2011 12:58:03 +1000 From: David Gibson Message-ID: <20110915025803.GP9025@yookeroo.fritz.box> References: <1315983769-8287-1-git-send-email-david@gibson.dropbear.id.au> <4E70647D.10408@redhat.com> <20110915023431.GO9025@yookeroo.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110915023431.GO9025@yookeroo.fritz.box> Subject: Re: [Qemu-devel] [PATCH] Fix subtle integer overflow bug in memory API List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity , agraf@suse.de, anthony@codemonkey.ws, qemu-devel@nongnu.org On Thu, Sep 15, 2011 at 12:34:31PM +1000, David Gibson wrote: > On Wed, Sep 14, 2011 at 11:23:25AM +0300, Avi Kivity wrote: > > On 09/14/2011 10:02 AM, David Gibson wrote: > > >It is quite common to have a MemoryRegion with size of INT64_MAX. > > >When processing alias regions in render_memory_region() it's quite > > >easy to find a case where it will construct a temporary AddrRange with > > >a non-zero start, and size still of INT64_MAX. When means attempting > > >to compute the end of such a range as start + size will result in > > >signed integer overflow. > > > > > >This integer overflow means that addrrange_intersects() can > > >incorrectly report regions as not intersecting when they do. For > > >example consider the case of address ranges {0x10000000000, > > >0x7fffffffffffffff} and {0x10010000000, 0x10000000} where the second > > >is in fact included completely in the first. > > > > Good catch, thanks for digging this out. > > > > >This patch rearranges addrrange_intersects() to avoid the integer > > >overflow, correcting this behaviour. > > > > I expect that the bad behaviour can still be triggered, for example > > by pointing aliases towards the end of very large regions. Not that > > I expect this to occur in practice. > > Well.. I'm pretty sure that particular case can no longer be > triggered. But there may be other integer overflow bugs in this code, > though I didn't spot them at a glance. Actually, there definitely are. addrrange_end() should probably be redefined as the inclusive end, rather than the exclusive end, and computed as: (start + size - 1) < start ? INT64_MAX : start + size - 1 Hrm, except that that doesn't handle zero size ranges. And gets a bit weird for INT64_MAX sized regions. Hrm, yeah, these really should be changed to use unsigneds. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson