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

[Qemu-devel] [PATCH v2 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.

Changes in v2:
  * Use muldiv64() instead of an __int128_t

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

[Qemu-devel] [PATCH v2 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 muldiv64(), which effectively
allows a 128-bit intermediate value between the multiplication and
division.

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..c7316f5 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 = muldiv64(get_ticks_per_sec(), timeout, 33000000);
 
     i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);
 
-- 
2.1.0

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

[Paolo Bonzini]

On 23/03/2015 02:51, David Gibson wrote:
> 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.
> 
> Changes in v2:
>   * Use muldiv64() instead of an __int128_t
> 
> David Gibson (2):
>   i6300esb: Correct endiannness
>   i6300esb: Fix signed integer overflow
> 
>  hw/watchdog/wdt_i6300esb.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 

Thanks, queueing for 2.3.0-rc2.

Paolo

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

[Richard W.M. Jones]

On Mon, Mar 23, 2015 at 12:51:48PM +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 muldiv64(), which effectively
> allows a 128-bit intermediate value between the multiplication and
> division.
> 
> 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..c7316f5 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 = muldiv64(get_ticks_per_sec(), timeout, 33000000);

ACK.

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-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

[Richard W.M. Jones]

On Mon, Mar 23, 2015 at 12:51:47PM +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 = {

ACK.

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-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

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

[BALATON Zoltan]

On Mon, 23 Mar 2015, 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 muldiv64(), which effectively
> allows a 128-bit intermediate value between the multiplication and
> division.
>
> 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..c7316f5 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
> +     */

Is the comment still correct saying "we use a 128-bit temporary" when the 
code does not do that explicitely any more?

Regards,
BALATON Zoltan

> +    timeout = muldiv64(get_ticks_per_sec(), timeout, 33000000);
>
>     i6300esb_debug("stage %d, timeout %" PRIi64 "\n", d->stage, timeout);

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

[David Gibson]

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

On Mon, Mar 23, 2015 at 10:54:39AM +0100, BALATON Zoltan wrote:
> On Mon, 23 Mar 2015, 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 muldiv64(), which effectively
> >allows a 128-bit intermediate value between the multiplication and
> >division.
> >
> >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..c7316f5 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
> >+     */
> 
> Is the comment still correct saying "we use a 128-bit temporary" when the
> code does not do that explicitely any more?

Bother.  I fixed the commit message, but not this comment.  It's still
kind of correct, in that muldiv64 does effectively have a 128-bit
temporary internally.  Not quite what I meant, and a little misleading
though.

Paolo, worth fixing?

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

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

[Paolo Bonzini]

On 24/03/2015 01:22, David Gibson wrote:
> On Mon, Mar 23, 2015 at 10:54:39AM +0100, BALATON Zoltan wrote:
>> On Mon, 23 Mar 2015, 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 muldiv64(), which
>>> effectively allows a 128-bit intermediate value between the
>>> multiplication and division.
>>> 
>>> 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..c7316f5 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 +     */
>> 
>> Is the comment still correct saying "we use a 128-bit temporary"
>> when the code does not do that explicitely any more?
> 
> Bother.  I fixed the commit message, but not this comment.  It's
> still kind of correct, in that muldiv64 does effectively have a
> 128-bit temporary internally.

Yes, that's how I interpreted it.  Though strictly speaking it's
96-bit.  I'll change it to "higher-precision intermediate value".

Paolo