| Message ID | 1426821116-16617-3-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 20, 2015 at 02:11:56PM +1100, 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. > > 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..11728af 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 > + */ > + timeout = (__int128_t)get_ticks_per_sec() * timeout / 33000000; > > i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout); > > -- > 2.1.0 Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich.
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. Paolo > 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..11728af 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 > + */ > + timeout = (__int128_t)get_ticks_per_sec() * timeout / 33000000; > > i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout); > >
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.
diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c index e694fa9..11728af 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 + */ + timeout = (__int128_t)get_ticks_per_sec() * timeout / 33000000; i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);
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. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/watchdog/wdt_i6300esb.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)