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

[David Gibson <david@gibson.dropbear.id.au>]

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