[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix subtle integer overflow bug in memory API
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH] Fix subtle integer overflow bug in memory API |
Date: |
Thu, 15 Sep 2011 12:58:03 +1000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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