Re: [Qemu-riscv] [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write

[Alistair Francis]

On Wed, Jan 30, 2019 at 10:53 AM Fabien Chouteau <address@hidden> wrote:
>
> Writing a high value in timecmp leads to an integer overflow. This patch
> modifies the code to detect such case, and use the maximum integer value
> as the next trigger for the timer.
>
> Signed-off-by: Fabien Chouteau <address@hidden>
> ---
>  hw/riscv/sifive_clint.c | 31 ++++++++++++++++++++++++++-----
>  1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> index d4c159e937..1ca1f8c75e 100644
> --- a/hw/riscv/sifive_clint.c
> +++ b/hw/riscv/sifive_clint.c
> @@ -38,8 +38,10 @@ static uint64_t cpu_riscv_read_rtc(void)
>   */
>  static void sifive_clint_write_timecmp(RISCVCPU *cpu, uint64_t value)
>  {
> -    uint64_t next;
>      uint64_t diff;
> +    uint64_t lapse_ns;
> +    uint64_t clock_ns;
> +    int64_t  next_ns;
>
>      uint64_t rtc_r = cpu_riscv_read_rtc();
>
> @@ -54,10 +56,29 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu, 
> uint64_t value)
>      /* otherwise, set up the future timer interrupt */
>      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, SIFIVE_CLINT_TIMEBASE_FREQ);
> -    timer_mod(cpu->env.timer, next);
> +
> +    /*
> +     * How many nanoseconds until the next trigger (note args switched in
> +     * muldiv64)
> +     */
> +    lapse_ns = muldiv64(diff,
> +                        NANOSECONDS_PER_SECOND,
> +                        SIFIVE_CLINT_TIMEBASE_FREQ);
> +
> +    /* Current time in nanoseconds */
> +    clock_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +
> +    if ((G_MAXINT64 - clock_ns) <= lapse_ns) {
> +        /*
> +         * clock + lapse would overflow on 64bit. The highest 64bit value is
> +         * used as the next trigger time.
> +         */
> +        next_ns = G_MAXINT64;
> +    } else {
> +        next_ns = clock_ns + lapse_ns;
> +    }

Can you describe what this fixes?

Won't an overflow be ok as we then just wrap around anyway? I guess
there is a problem if we want a value so large that we wrap around
past our current time though.

Alistair

> +
> +    timer_mod(cpu->env.timer, next_ns);
>  }
>
>  /*
> --
> 2.17.1
>
>