* [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write
@ 2019-01-30 18:52 Fabien Chouteau
2019-02-07 0:42 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Fabien Chouteau @ 2019-01-30 18:52 UTC (permalink / raw)
Cc: Fabien Chouteau, Michael Clark, Palmer Dabbelt, Alistair Francis,
Sagar Karandikar, Bastian Koppelmann, open list:RISC-V,
open list:All patches CC here
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;
+ }
+
+ timer_mod(cpu->env.timer, next_ns);
}
/*
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write
2019-01-30 18:52 [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write Fabien Chouteau
@ 2019-02-07 0:42 ` Alistair Francis
2019-02-07 10:08 ` Fabien Chouteau
0 siblings, 1 reply; 5+ messages in thread
From: Alistair Francis @ 2019-02-07 0:42 UTC (permalink / raw)
To: Fabien Chouteau
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, open list:All patches CC here, Michael Clark,
Alistair Francis
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write
2019-02-07 0:42 ` Alistair Francis
@ 2019-02-07 10:08 ` Fabien Chouteau
2019-02-08 18:41 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Fabien Chouteau @ 2019-02-07 10:08 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, open list:All patches CC here, Michael Clark,
Alistair Francis
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,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write
2019-02-07 10:08 ` Fabien Chouteau
@ 2019-02-08 18:41 ` Alistair Francis
2019-02-13 18:12 ` Palmer Dabbelt
0 siblings, 1 reply; 5+ messages in thread
From: Alistair Francis @ 2019-02-08 18:41 UTC (permalink / raw)
To: Fabien Chouteau
Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
Palmer Dabbelt, open list:All patches CC here, Michael Clark,
Alistair Francis
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,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write
2019-02-08 18:41 ` Alistair Francis
@ 2019-02-13 18:12 ` Palmer Dabbelt
0 siblings, 0 replies; 5+ messages in thread
From: Palmer Dabbelt @ 2019-02-13 18:12 UTC (permalink / raw)
To: alistair23
Cc: chouteau, qemu-riscv, sagark, Bastian Koppelmann, qemu-devel, mjc,
Alistair Francis
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-13 18:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-30 18:52 [Qemu-devel] [PATCH] hw/riscv/sifive_clint.c: avoid integer overflow in timecmp write Fabien Chouteau
2019-02-07 0:42 ` Alistair Francis
2019-02-07 10:08 ` Fabien Chouteau
2019-02-08 18:41 ` Alistair Francis
2019-02-13 18:12 ` Palmer Dabbelt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).