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

[Philippe Mathieu-Daudé]

+Evgeny

On 8/11/24 16:47, Peter Maydell wrote:
On Wed, 6 Nov 2024 at 08:38, Dmitry Frolov <fro...@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 <fro...@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