| Message ID | 20190130185242.12490-1-chouteau@adacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write | expand |
On Wed, Jan 30, 2019 at 10:53 AM Fabien Chouteau <chouteau@adacore.com> 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 <chouteau@adacore.com> > --- > 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 > >
Hello Alistair, On 07/02/2019 01:42, Alistair Francis wrote:> > Can you describe what this fixes? > I encountered this problem when I tried to write 0xffffffffffffffff in timecmp. With the integer overflow in QEMU, writing this value means that the QEMU timer will be set in the past. > 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. > The overflow was in the computation of the value `next_ns`. It is used to set the QEMU timer: timer_mod(cpu->env.timer, next_ns); A negative `next_ns` -because of the overflow- means that the timer triggers immediately instead of far in the future. Regards,
On Thu, Feb 7, 2019 at 2:08 AM Fabien Chouteau <chouteau@adacore.com> wrote: > > Hello Alistair, > > On 07/02/2019 01:42, Alistair Francis wrote:> > > Can you describe what this fixes? > > > > I encountered this problem when I tried to write 0xffffffffffffffff in timecmp. > > With the integer overflow in QEMU, writing this value means that the QEMU timer > will be set in the past. > > > 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. > > > > The overflow was in the computation of the value `next_ns`. It is used to set > the QEMU timer: > > timer_mod(cpu->env.timer, next_ns); > > A negative `next_ns` -because of the overflow- means that the timer > triggers immediately instead of far in the future. Ah you are right here. The expired time of the timer will be set to zero (it looks like QEMU ensures it can't be negative) but then it detects that as being in the past and will trigger the timer event as timer_expired_ns() will return true. Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > > Regards,
On Fri, 08 Feb 2019 10:41:17 PST (-0800), alistair23@gmail.com wrote: > On Thu, Feb 7, 2019 at 2:08 AM Fabien Chouteau <chouteau@adacore.com> wrote: >> >> Hello Alistair, >> >> On 07/02/2019 01:42, Alistair Francis wrote:> >> > Can you describe what this fixes? >> > >> >> I encountered this problem when I tried to write 0xffffffffffffffff in timecmp. >> >> With the integer overflow in QEMU, writing this value means that the QEMU timer >> will be set in the past. >> >> > 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. >> > >> >> The overflow was in the computation of the value `next_ns`. It is used to set >> the QEMU timer: >> >> timer_mod(cpu->env.timer, next_ns); >> >> A negative `next_ns` -because of the overflow- means that the timer >> triggers immediately instead of far in the future. > > Ah you are right here. The expired time of the timer will be set to > zero (it looks like QEMU ensures it can't be negative) but then it > detects that as being in the past and will trigger the timer event as > timer_expired_ns() will return true. > > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Thanks. I'll target this for the next PR.
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; + } + + timer_mod(cpu->env.timer, next_ns); } /*
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 <chouteau@adacore.com> --- hw/riscv/sifive_clint.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)