Message ID | 1475054208-20288-1-git-send-email-ppandit@redhat.com |
---|---|
State | New |
Headers | show |
On 28 September 2016 at 02:16, 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. Remove the loop incrementing the > comparator value; Add check to avoid value zero. > > Reported-by: Li Qiang <liqiang6-s@360.cn> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/timer/a9gtimer.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > > Update per > -> https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05682.html > > diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c > index 772f85f..021ae59 100644 > --- a/hw/timer/a9gtimer.c > +++ b/hw/timer/a9gtimer.c > @@ -72,7 +72,7 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync) > { > > A9GTimerUpdate update = a9_gtimer_get_update(s); > - int i; > + uint32_t i, inc; Why have you changed the type of i when you haven't changed any of the code that works with it? Since the gtb->compare field is 64 bit your inc variable should probably be uint64_t. > int64_t next_cdiff = 0; > > for (i = 0; i < s->num_cpu; ++i) { > @@ -82,16 +82,14 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync) > if ((s->control & R_CONTROL_TIMER_ENABLE) && > (gtb->control & R_CONTROL_COMP_ENABLE)) { > /* R2p0+, where the compare function is >= */ > - 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) { > - DB_PRINT("Auto incrementing timer compare by %" PRId32 "\n", > - gtb->inc); > - gtb->compare += gtb->inc; > - } else { > - break; > - } > + DB_PRINT("Compare event happened for CPU %d\n", i); This is wrong -- this debug print should only happen when the comparator matches (ie when gtb->compare is less than update.new), not every time. > + gtb->status = 1; Similarly the status flag should only be set when the comparator matches. > + if (gtb->control & R_CONTROL_AUTO_INCREMENT > + && gtb->compare < update.new) { > + inc = ((update.new - gtb->compare - 1) / gtb->inc) * gtb->inc; I suggest using either QEMU_ALIGN_UP or QEMU_ALIGN_DOWN here, depending on which way round you're trying for. Also this will divide by zero if gtb->inc is 0 (as for instance it will be on reset). > + DB_PRINT("Auto incrementing timer compare by %" > + PRId32 "\n", inc); > + gtb->compare += inc; > } > cdiff = (int64_t)gtb->compare - (int64_t)update.new + 1; > if (cdiff > 0 && (cdiff < next_cdiff || !next_cdiff)) { > @@ -210,7 +208,7 @@ static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value, > gtb->compare = deposit64(gtb->compare, shift, 32, value); > break; > case R_AUTO_INCREMENT: > - gtb->inc = value; > + gtb->inc = value ? value : 1; Where has this change come from ?? > return; > default: > return; > -- > 2.5.5 > thanks -- PMM
Hello Peter, +-- On Fri, 30 Sep 2016, Peter Maydell wrote --+ | > - int i; | > + uint32_t i, inc; | | Why have you changed the type of i when you haven't changed | any of the code that works with it? Actually I declared 'inc' on the same line, then changed type to 'uint32' as 'gtb->inc' is of the same type. | Since the gtb->compare field is 64 bit your inc variable should | probably be uint64_t. Okay. I was wondering about its values, as gtb->inc is 'uint32_t' type. | > + DB_PRINT("Compare event happened for CPU %d\n", i); | | This is wrong -- this debug print should only happen when the | comparator matches (ie when gtb->compare is less than update.new), | not every time. | | > + gtb->status = 1; | | Similarly the status flag should only be set when the comparator | matches. | | > + if (gtb->control & R_CONTROL_AUTO_INCREMENT | > + && gtb->compare < update.new) { | > + inc = ((update.new - gtb->compare - 1) / gtb->inc) * gtb->inc; | | I suggest using either QEMU_ALIGN_UP or QEMU_ALIGN_DOWN here, | depending on which way round you're trying for. Okay. | Also this will divide by zero if gtb->inc is 0 (as for instance | it will be on reset). | | > + gtb->inc = value ? value : 1; | | Where has this change come from ?? This was to avoid the possible divide by zero. Couldn't find if zero could be valid value for 'gtb->inc' and a non-zero default value if not. If 'gtb->inc' could be zero, should the 'gtb->compare += inc' only happen when it's non-zero ? Ie gtb->status would be set if gtb->inc > 0. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On 1 October 2016 at 23:15, P J P <ppandit@redhat.com> wrote: > Hello Peter, > > +-- On Fri, 30 Sep 2016, Peter Maydell wrote --+ > | Since the gtb->compare field is 64 bit your inc variable should > | probably be uint64_t. > > Okay. I was wondering about its values, as gtb->inc is 'uint32_t' type. The timer can't autoincrement by more than a 32 bit value, but the timer value is 64 bits, and since this code may be doing more than one autoincrement it's better to use 64 bits. > | > + DB_PRINT("Compare event happened for CPU %d\n", i); > | > | This is wrong -- this debug print should only happen when the > | comparator matches (ie when gtb->compare is less than update.new), > | not every time. > | > | > + gtb->status = 1; > | > | Similarly the status flag should only be set when the comparator > | matches. > | > | > + if (gtb->control & R_CONTROL_AUTO_INCREMENT > | > + && gtb->compare < update.new) { > | > + inc = ((update.new - gtb->compare - 1) / gtb->inc) * gtb->inc; > | > | I suggest using either QEMU_ALIGN_UP or QEMU_ALIGN_DOWN here, > | depending on which way round you're trying for. > > Okay. > > | Also this will divide by zero if gtb->inc is 0 (as for instance > | it will be on reset). > | > | > + gtb->inc = value ? value : 1; > | > | Where has this change come from ?? > > This was to avoid the possible divide by zero. Couldn't find if zero could > be valid value for 'gtb->inc' and a non-zero default value if not. If in doubt, you need to check the datasheet/technical reference manual for the hardware, in this case http://infocenter.arm.com/help/topic/com.arm.doc.100486_0401_10_en/ada1443777319205.html Zero is a valid value for the auto-increment register, and in fact it is the documented reset value. > If > 'gtb->inc' could be zero, should the 'gtb->compare += inc' > only happen when > it's non-zero ? Ie gtb->status would be set if gtb->inc > 0. If gtb->inc is zero, then when the timer fires the compare register should increment by the increment value (which will be zero, so it won't change). This is not a particularly useful setting for the guest to put the timer into, but the behaviour is well-defined. (Effectively, setting the increment to 0 is equivalent to disabling auto-increment.) gtb->status should be set whenever the timer comparator fires, whether we are auto-incrementing or not, and whatever the value of the auto-increment register is. It is the "timer matched" interrupt output. (Your current patch is wrong in that it has moved setting gtb->status into the wrong place, as I mentioned above.) thanks -- PMM
Hello Peter, +-- On Sun, 2 Oct 2016, Peter Maydell wrote --+ | The timer can't autoincrement by more than a 32 bit value, but | the timer value is 64 bits, and since this code may be doing more than | one autoincrement it's better to use 64 bits. Okay. | If in doubt, you need to check the datasheet/technical reference manual for | the hardware, in this case | http://infocenter.arm.com/help/topic/com.arm.doc.100486_0401_10_en/ada1443777319205.html | Zero is a valid value for the auto-increment register, and in fact it is the | documented reset value. Yes, I was following the same. It does mention zero as a valid reset value, but does not mention about value to be writen to Timer Load Register. As in, when user attempts to write to a register. | gtb->status should be set whenever the timer comparator | fires, whether we are auto-incrementing or not, and | whatever the value of the auto-increment register is. | It is the "timer matched" interrupt output. (Your current | patch is wrong in that it has moved setting gtb->status | into the wrong place, as I mentioned above.) Done; I've sent a revised patch v3. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
diff --git a/hw/timer/a9gtimer.c b/hw/timer/a9gtimer.c index 772f85f..021ae59 100644 --- a/hw/timer/a9gtimer.c +++ b/hw/timer/a9gtimer.c @@ -72,7 +72,7 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync) { A9GTimerUpdate update = a9_gtimer_get_update(s); - int i; + uint32_t i, inc; int64_t next_cdiff = 0; for (i = 0; i < s->num_cpu; ++i) { @@ -82,16 +82,14 @@ static void a9_gtimer_update(A9GTimerState *s, bool sync) if ((s->control & R_CONTROL_TIMER_ENABLE) && (gtb->control & R_CONTROL_COMP_ENABLE)) { /* R2p0+, where the compare function is >= */ - 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) { - DB_PRINT("Auto incrementing timer compare by %" PRId32 "\n", - gtb->inc); - gtb->compare += gtb->inc; - } else { - break; - } + DB_PRINT("Compare event happened for CPU %d\n", i); + gtb->status = 1; + if (gtb->control & R_CONTROL_AUTO_INCREMENT + && gtb->compare < update.new) { + inc = ((update.new - gtb->compare - 1) / gtb->inc) * gtb->inc; + DB_PRINT("Auto incrementing timer compare by %" + PRId32 "\n", inc); + gtb->compare += inc; } cdiff = (int64_t)gtb->compare - (int64_t)update.new + 1; if (cdiff > 0 && (cdiff < next_cdiff || !next_cdiff)) { @@ -210,7 +208,7 @@ static void a9_gtimer_write(void *opaque, hwaddr addr, uint64_t value, gtb->compare = deposit64(gtb->compare, shift, 32, value); break; case R_AUTO_INCREMENT: - gtb->inc = value; + gtb->inc = value ? value : 1; return; default: return;