Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/2] i6300esb: Fix signed integer overflow

[Paolo Bonzini <pbonzini@redhat.com>]

On 24/03/2015 01:22, David Gibson wrote:
> On Mon, Mar 23, 2015 at 10:54:39AM +0100, BALATON Zoltan wrote:
>> On Mon, 23 Mar 2015, David Gibson wrote:
>>> If the guest programs a sufficiently large timeout value an
>>> integer overflow can occur in i6300esb_restart_timer().  e.g.
>>> if the maximum possible timer preload value of 0xfffff is
>>> programmed then we end up with the calculation:
>>> 
>>> timeout = get_ticks_per_sec() * (0xfffff << 15) / 33000000;
>>> 
>>> get_ticks_per_sec() returns 1000000000 (10^9) giving:
>>> 
>>> 10^9 * (0xfffff * 2^15) == 0x1dcd632329b000000 (65 bits)
>>> 
>>> Obviously the division by 33MHz brings it back under 64-bits,
>>> but the overflow has already occurred.
>>> 
>>> Since signed integer overflow has undefined behaviour in C, in
>>> theory this could be arbitrarily bad.  In practice, the
>>> overflowed value wraps around to something negative, causing
>>> the watchdog to immediately expire, killing the guest, which is
>>> still fairly bad.
>>> 
>>> The bug can be triggered by running a Linux guest, loading the
>>> i6300esb driver with parameter "heartbeat=2046" and opening
>>> /dev/watchdog.  The watchdog will trigger as soon as the device
>>> is opened.
>>> 
>>> This patch corrects the problem by using muldiv64(), which
>>> effectively allows a 128-bit intermediate value between the
>>> multiplication and division.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- 
>>> hw/watchdog/wdt_i6300esb.c | 10 ++++++++-- 1 file changed, 8
>>> insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/watchdog/wdt_i6300esb.c
>>> b/hw/watchdog/wdt_i6300esb.c index e694fa9..c7316f5 100644 ---
>>> a/hw/watchdog/wdt_i6300esb.c +++ b/hw/watchdog/wdt_i6300esb.c 
>>> @@ -125,8 +125,14 @@ static void
>>> i6300esb_restart_timer(I6300State *d, int stage) else timeout
>>> <<= 5;
>>> 
>>> -    /* Get the timeout in units of ticks_per_sec. */ -
>>> timeout = get_ticks_per_sec() * timeout / 33000000; +    /* Get
>>> the timeout in units of ticks_per_sec. +     * +     *
>>> ticks_per_sec is typically 10^9 == 0x3B9ACA00 (30 bits), with +
>>> * 20 bits of user supplied preload, and 15 bits of scale, the +
>>> * multiply here can exceed 64-bits, before we divide by 33MHz,
>>> so +     * we use a 128-bit temporary +     */
>> 
>> Is the comment still correct saying "we use a 128-bit temporary"
>> when the code does not do that explicitely any more?
> 
> Bother.  I fixed the commit message, but not this comment.  It's
> still kind of correct, in that muldiv64 does effectively have a
> 128-bit temporary internally.

Yes, that's how I interpreted it.  Though strictly speaking it's
96-bit.  I'll change it to "higher-precision intermediate value".

Paolo