[Qemu-devel] [PATCH] timer: a9gtimer: check auto-increment register value

[Qemu-devel] [PATCH] timer: a9gtimer: check auto-increment register value

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

ARM A9MP processor has a peripheral timer with an auto-increment
register, which holds an increment step value. A user could set
this value to zero, when auto-increment control bit is enabled.
This leads to an infinite loop in 'a9_gtimer_update' while
updating comparator value. Add check to avoid it.

Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/timer/a9gtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
index 772f85f..3f752ce 100644
--- a/hw/timer/a9gtimer.c
+++ b/hw/timer/a9gtimer.c
@@ -85,7 +85,7 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync)
             while (gtb->compare < update.new) {
                 DB_PRINT("Compare event happened for CPU %d\n", i);
                 gtb->status = 1;
-                if (gtb->control & R_CONTROL_AUTO_INCREMENT) {
+                if (gtb->inc && gtb->control & R_CONTROL_AUTO_INCREMENT) {
                     DB_PRINT("Auto incrementing timer compare by %" PRId32 "\n",
                              gtb->inc);
                     gtb->compare += gtb->inc;
-- 
2.5.5

Re: [Qemu-devel] [PATCH] timer: a9gtimer: check auto-increment register value

[Peter Maydell]

On 21 September 2016 at 21:13, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> ARM A9MP processor has a peripheral timer with an auto-increment
> register, which holds an increment step value. A user could set
> this value to zero, when auto-increment control bit is enabled.
> This leads to an infinite loop in 'a9_gtimer_update' while
> updating comparator value. Add check to avoid it.
>
> Reported-by: Li Qiang <liqiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/timer/a9gtimer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c
> index 772f85f..3f752ce 100644
> --- a/hw/timer/a9gtimer.c
> +++ b/hw/timer/a9gtimer.c
> @@ -85,7 +85,7 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync)
>              while (gtb->compare < update.new) {
>                  DB_PRINT("Compare event happened for CPU %d\n", i);
>                  gtb->status = 1;
> -                if (gtb->control & R_CONTROL_AUTO_INCREMENT) {
> +                if (gtb->inc && gtb->control & R_CONTROL_AUTO_INCREMENT) {
>                      DB_PRINT("Auto incrementing timer compare by %" PRId32 "\n",
>                               gtb->inc);
>                      gtb->compare += gtb->inc;

The code as it stands is definitely wrong, but it's also rather
weird, because it loops round just incrementing gtb->compare
by gtb->inc each time until it's >= update.new, when we ought
in theory to be able to calculate the new compare value without
having to loop round to do it.

The loop code also introduces some other odd corner cases which
are might loop forever or at least run for a long time before
they terminate. For instance suppose gtb->compare is
0xFFFF_FFFF_FFFF_0000, update.new is slightly larger than that,
and gtb->inc is 0x10000. This will loop forever because
compare will wrap round to 0x10000, then increment steadily
until it hits 0xFFFF_FFFF_FFFF_0000 again, and repeat.

So I think we need to fix this bug by rewriting the code to
not have a loop in it at all. (Something like, take
(update.new - gtb->compare), round it up to a multiple of
gtb->inc, and then add that to gtb->compare.)

thanks
-- PMM