[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 2/2] i6300esb: Fix signed integer overflow
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH 2/2] i6300esb: Fix signed integer overflow |
Date: |
Mon, 23 Mar 2015 11:18:26 +1100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Fri, Mar 20, 2015 at 10:13:26AM +0100, Paolo Bonzini wrote:
>
>
> On 20/03/2015 04:11, 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 an __int128_t temporary. With
> > suitable rearrangement of the calculations, I expect it would be possible
> > to avoid the __int128_t. But since we already use __int128_t extensively
> > in the memory region code, and this is not a hot path, the super-wide
> > integer seems like the simplest approach.
>
> We don't use __int128_t, we use the Int128 struct---which however
> doesn't have a multiplication function. __int128_t is not available on
> 32-bit machines, and is only used under #ifdef CONFIG_INT128.
>
> Instead, you can use muldiv64 which has exactly this purpose.
Ah, good point. I'll repost using muldiv64.
--
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
pgpZ2h_1hNGSg.pgp
Description: PGP signature