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

[David Gibson]

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