[Qemu-devel] [PATCH 0/2] Fix bugs in i6300esb watchdog timer

[Qemu-devel] [PATCH 0/2] Fix bugs in i6300esb watchdog timer

[David Gibson]

This series fixes two bugs in the i6300esb watchdog timer device.  The
first only affects big-endian targets (including targets like ppc
which support both endians, but are considered big-endian by default).
The second affects all targets, but only when the guest uses unusually
large timeout values.

David Gibson (2):
  i6300esb: Correct endiannness
  i6300esb: Fix signed integer overflow

 hw/watchdog/wdt_i6300esb.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] i6300esb: Correct endiannness

[David Gibson]

The IO operations for the i6300esb watchdog timer are marked as
DEVICE_NATIVE_ENDIAN.  This is not correct, and - as a PCI device - should
be DEVICE_LITTLE_ENDIAN.

This allows i6300esb to work on ppc targets (yes, using an Intel ICH
derived device on ppc is a bit odd, but the driver exists on the guest
and there's no more obviously suitable watchdog device).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/watchdog/wdt_i6300esb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index b2d158f..e694fa9 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -369,7 +369,7 @@ static const MemoryRegionOps i6300esb_ops = {
             i6300esb_mem_writel,
         },
     },
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static const VMStateDescription vmstate_i6300esb = {
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/2] i6300esb: Correct endiannness

[Richard W.M. Jones]

On Fri, Mar 20, 2015 at 02:11:55PM +1100, David Gibson wrote:
> The IO operations for the i6300esb watchdog timer are marked as
> DEVICE_NATIVE_ENDIAN.  This is not correct, and - as a PCI device - should
> be DEVICE_LITTLE_ENDIAN.
> 
> This allows i6300esb to work on ppc targets (yes, using an Intel ICH
> derived device on ppc is a bit odd, but the driver exists on the guest
> and there's no more obviously suitable watchdog device).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/watchdog/wdt_i6300esb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
> index b2d158f..e694fa9 100644
> --- a/hw/watchdog/wdt_i6300esb.c
> +++ b/hw/watchdog/wdt_i6300esb.c
> @@ -369,7 +369,7 @@ static const MemoryRegionOps i6300esb_ops = {
>              i6300esb_mem_writel,
>          },
>      },
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
>  static const VMStateDescription vmstate_i6300esb = {

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

 - - -

For your amusement, the physical device only ever existed as part of
an Intel "Enterprise SouthBridge" (hence ESB).  I highly doubt it was
ever connected to anything that was not an Intel x86 processor.  There
is a photo of one here:
http://www.tomshardware.com/reviews/amd,889-18.html [requires javascript]

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

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

[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 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

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

[Richard W.M. Jones]

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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

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

[Paolo Bonzini]

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);
>  
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2086 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]