[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/2] i6300esb: Fix signed integer
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/2] i6300esb: Fix signed integer overflow |
Date: |
Tue, 24 Mar 2015 08:48:37 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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 <address@hidden> ---
>>> 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