hw/timer: fix possible int overflow

[PATCH] hw/timer: fix possible int overflow

Posted by Dmitry Frolov 2 weeks, 3 days ago

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;
         }
-- 
2.43.0

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

Posted by Peter Maydell 2 weeks, 1 day ago

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;
>          }
> --



Applied to target-arm.next, thanks.

-- PMM

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

Posted by Philippe Mathieu-Daudé 2 weeks, 1 day ago

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

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

Posted by Евгений Воеводин 2 weeks ago

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