[PATCH v3] hw/intc/sifive_clint: Fix overflow in sifive_clint_write_timecmp()

[PATCH v3] hw/intc/sifive_clint: Fix overflow in sifive_clint_write_timecmp()

[David Hoppenbrouwers]

`muldiv64` would overflow in cases where the final 96-bit value does not
fit in a `uint64_t`. This would result in small values that cause an
interrupt to be triggered much sooner than intended.

The overflow can be detected in most cases by checking if the new value is
smaller than the previous value. If the final result is larger than
`diff` it is either correct or it doesn't matter as it is effectively
infinite anyways.

`next` is an `uint64_t` value, but `timer_mod` takes an `int64_t`. This
resulted in high values such as `UINT64_MAX` being converted to `-1`,
which caused an immediate timer interrupt.

By limiting `next` to `INT64_MAX` no overflow will happen while the
timer will still be effectively set to "infinitely" far in the future.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/493
Signed-off-by: David Hoppenbrouwers <david@salt-inc.org>
---
I did not account for the multiplication overflow mentioned in the bug
report. I've amended the patch and I do not spot any erroneous interrupts
anymore.

I see that the previous patch already got applied to
riscv-to-apply.next. Do I need to create a new patch?

David

 hw/intc/sifive_clint.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
index 0f41e5ea1c..aa76e639a9 100644
--- a/hw/intc/sifive_clint.c
+++ b/hw/intc/sifive_clint.c
@@ -59,8 +59,23 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value,
     riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
     diff = cpu->env.timecmp - rtc_r;
     /* back to ns (note args switched in muldiv64) */
-    next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-        muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
+    uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
+
+    /*
+     * check if ns_diff overflowed and check if the addition would potentially
+     * overflow
+     */
+    if ((NANOSECONDS_PER_SECOND > timebase_freq && ns_diff < diff) ||
+        ns_diff > INT64_MAX) {
+        next = INT64_MAX;
+    } else {
+        /*
+         * as it is very unlikely qemu_clock_get_ns will return a value
+         * greater than INT64_MAX, no additional check is needed.
+         */
+        next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ns_diff;
+    }
+
     timer_mod(cpu->env.timer, next);
 }
 
-- 
2.20.1

Re: [PATCH v3] hw/intc/sifive_clint: Fix overflow in sifive_clint_write_timecmp()

[Alistair Francis]

On Thu, Aug 19, 2021 at 3:19 PM David Hoppenbrouwers <david@salt-inc.org> wrote:
>
> `muldiv64` would overflow in cases where the final 96-bit value does not
> fit in a `uint64_t`. This would result in small values that cause an
> interrupt to be triggered much sooner than intended.
>
> The overflow can be detected in most cases by checking if the new value is
> smaller than the previous value. If the final result is larger than
> `diff` it is either correct or it doesn't matter as it is effectively
> infinite anyways.
>
> `next` is an `uint64_t` value, but `timer_mod` takes an `int64_t`. This
> resulted in high values such as `UINT64_MAX` being converted to `-1`,
> which caused an immediate timer interrupt.
>
> By limiting `next` to `INT64_MAX` no overflow will happen while the
> timer will still be effectively set to "infinitely" far in the future.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/493
> Signed-off-by: David Hoppenbrouwers <david@salt-inc.org>
> ---
> I did not account for the multiplication overflow mentioned in the bug
> report. I've amended the patch and I do not spot any erroneous interrupts
> anymore.
>
> I see that the previous patch already got applied to
> riscv-to-apply.next. Do I need to create a new patch?

I have just removed that patch, so this is fine.

>
> David
>
>  hw/intc/sifive_clint.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/sifive_clint.c b/hw/intc/sifive_clint.c
> index 0f41e5ea1c..aa76e639a9 100644
> --- a/hw/intc/sifive_clint.c
> +++ b/hw/intc/sifive_clint.c
> @@ -59,8 +59,23 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value,
>      riscv_cpu_update_mip(cpu, MIP_MTIP, BOOL_TO_MASK(0));
>      diff = cpu->env.timecmp - rtc_r;
>      /* back to ns (note args switched in muldiv64) */
> -    next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -        muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> +    uint64_t ns_diff = muldiv64(diff, NANOSECONDS_PER_SECOND, timebase_freq);
> +
> +    /*
> +     * check if ns_diff overflowed and check if the addition would potentially
> +     * overflow
> +     */
> +    if ((NANOSECONDS_PER_SECOND > timebase_freq && ns_diff < diff) ||
> +        ns_diff > INT64_MAX) {
> +        next = INT64_MAX;
> +    } else {
> +        /*
> +         * as it is very unlikely qemu_clock_get_ns will return a value
> +         * greater than INT64_MAX, no additional check is needed.
> +         */
> +        next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + ns_diff;

What if ns_diff is INT64_MAX, won't this overflow?

Alistair

> +    }
> +
>      timer_mod(cpu->env.timer, next);
>  }
>
> --
> 2.20.1
>
>