| Message ID | 20241106083801.219578-2-frolov@swemel.ru (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/timer: fix possible int overflow | expand |
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
+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 >
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 > > > >
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; }
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(-)