From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59256) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbAF6-0001zY-LD for qemu-devel@nongnu.org; Thu, 26 Mar 2015 12:02:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbAF5-0002uh-Fe for qemu-devel@nongnu.org; Thu, 26 Mar 2015 12:02:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54569) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbAF5-0002ua-B0 for qemu-devel@nongnu.org; Thu, 26 Mar 2015 12:02:47 -0400 From: Paolo Bonzini Date: Thu, 26 Mar 2015 17:02:24 +0100 Message-Id: <1427385754-13012-5-git-send-email-pbonzini@redhat.com> In-Reply-To: <1427385754-13012-1-git-send-email-pbonzini@redhat.com> References: <1427385754-13012-1-git-send-email-pbonzini@redhat.com> Subject: [Qemu-devel] [PULL 04/14] i6300esb: Fix signed integer overflow List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: David Gibson From: David Gibson 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 Message-Id: <1427075508-12099-3-git-send-email-david@gibson.dropbear.id.au> Signed-off-by: Paolo Bonzini --- 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..4ebdbb8 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 higher-precision intermediate result. + */ + timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000); i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout); -- 2.3.3