Re: [PATCH] hw/timer: fix possible int overflow

[Евгений Воеводин]

Hey guys,

I can't remember details about this particular work which has been done more than decade ago, but I guess that these uint32_t variables reflect the architectural state of the HW, so if it might overflow over time, there is high probability that this is what was architecturally going to happen.

пт, 8 нояб. 2024 г. в 09:22, Philippe Mathieu-Daudé <philmd@linaro.org>:

+Evgeny

On 8/11/24 16:47, Peter Maydell wrote:
> On Wed, 6 Nov 2024 at 08:38, Dmitry Frolov <frolov@swemel.ru> wrote:
>>
>> The product "icnto * s->tcntb" may overflow uint32_t.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>> ---
>>   hw/timer/exynos4210_mct.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
>> index e807fe2de9..5c6e139b20 100644
>> --- a/hw/timer/exynos4210_mct.c
>> +++ b/hw/timer/exynos4210_mct.c
>> @@ -815,7 +815,7 @@ static uint32_t exynos4210_ltick_cnt_get_cnto(struct tick_timer *s)
>>           /* Both are counting */
>>           icnto = remain / s->tcntb;
>>           if (icnto) {
>> -            tcnto = remain % (icnto * s->tcntb);
>> +            tcnto = remain % ((uint64_t)icnto * s->tcntb);
>>           } else {
>>               tcnto = remain % s->tcntb;
>>           }
>> --

Alternatively we can declaring icnto as uint64_t locally:

-- >8 --
diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index e807fe2de9..9fae2ceda9 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -787,7 +787,6 @@ static void exynos4210_ltick_tx_commit(struct
tick_timer *s)
  static uint32_t exynos4210_ltick_cnt_get_cnto(struct tick_timer *s)
  {
      uint32_t tcnto;
-    uint32_t icnto;
      uint64_t remain;
      uint64_t counted;
      uint64_t count;
@@ -813,7 +812,7 @@ static uint32_t exynos4210_ltick_cnt_get_cnto(struct
tick_timer *s)
          tcnto = remain % s->tcntb;
      } else {
          /* Both are counting */
-        icnto = remain / s->tcntb;
+        uint64_t icnto = remain / s->tcntb;
          if (icnto) {
              tcnto = remain % (icnto * s->tcntb);
          } else {
---

But then isn't it equivalent to this? Dunno, I might be
missing something...

-- >8 --
diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
index e807fe2de9..d8b2c72b73 100644
--- a/hw/timer/exynos4210_mct.c
+++ b/hw/timer/exynos4210_mct.c
@@ -787,7 +787,6 @@ static void exynos4210_ltick_tx_commit(struct
tick_timer *s)
  static uint32_t exynos4210_ltick_cnt_get_cnto(struct tick_timer *s)
  {
      uint32_t tcnto;
-    uint32_t icnto;
      uint64_t remain;
      uint64_t counted;
      uint64_t count;
@@ -813,9 +812,8 @@ static uint32_t exynos4210_ltick_cnt_get_cnto(struct
tick_timer *s)
          tcnto = remain % s->tcntb;
      } else {
          /* Both are counting */
-        icnto = remain / s->tcntb;
-        if (icnto) {
-            tcnto = remain % (icnto * s->tcntb);
+        if (remain / s->tcntb) {
+            tcnto = 0;
          } else {
              tcnto = remain % s->tcntb;
          }
---

> Applied to target-arm.next, thanks.
>
> -- PMM
>