timer: a9gtimer: check auto-increment register value

PATCH Superseded P J P 2 participants 2 messages Sep 21, 2016 – Sep 22, 2016

lore

Share & tools

lore

P J P

author

2016-09-21 20:13 UTC #0 lore raw

PATCH 1/1 timer: a9gtimer: check auto-increment register value

From: Prasad J Pandit <[email protected]>

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 <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/timer/a9gtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/timer/a9gtimer.c +1 -1

index 772f85f..3f752ce 100644
@@ -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;

Signature · 1 line

2.5.5

Peter Maydell

2016-09-22 13:59 UTC #1 lore raw

Quoted 19 lines · P J P

From: Prasad J Pandit <[email protected]>
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 <[email protected]>
Signed-off-by Prasad J Pandit <[email protected]>
---
 hw/timer/a9gtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/timer/a9gtimer.c +0 -0
index 772f85f..3f752ce 100644

hw/timer/a9gtimer.c static void a9_gtimer_update(A9GTimerState *s, bool sync) · line 91

Quoted 8 lines

hw/timer/a9gtimer.c +1 -1

@@ -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