| From 7f476950b0f5780d1112f8e9d0d92ece55ae6912 Mon Sep 17 00:00:00 2001 |
| From: Richard Jones <rjones@redhat.com> |
| Date: Wed, 1 Nov 2017 11:33:00 +0100 |
| Subject: [PATCH 5/7] i6300esb: Fix signed integer overflow |
| |
| RH-Author: Richard Jones <rjones@redhat.com> |
| Message-id: <1509535982-27927-2-git-send-email-rjones@redhat.com> |
| Patchwork-id: 77461 |
| O-Subject: [RHEL-7.5 qemu-kvm PATCH v3 1/3] i6300esb: Fix signed integer overflow |
| Bugzilla: 1470244 |
| RH-Acked-by: Paolo Bonzini <pbonzini@redhat.com> |
| RH-Acked-by: Thomas Huth <thuth@redhat.com> |
| RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com> |
| |
| From: 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 muldiv64(), which effectively |
| allows a 128-bit intermediate value between the multiplication and |
| division. |
| |
| Signed-off-by: David Gibson <david@gibson.dropbear.id.au> |
| Message-Id: <1427075508-12099-3-git-send-email-david@gibson.dropbear.id.au> |
| Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> |
| (cherry picked from commit 4bc7b4d56657ebf75b986ad46e959cf7232ff26a) |
| |
| BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1470244 |
| Upstream-status: 4bc7b4d56657ebf75b986ad46e959cf7232ff26a |
| Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com> |
| |
| 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 a2ace52..be35034 100644 |
| |
| |
| @@ -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); |
| |
| -- |
| 1.8.3.1 |
| |