[PATCH] hw/misc: cast nand_getio value to uint64_t

[PATCH] hw/misc: cast nand_getio value to uint64_t

[Tigran Sogomonian]

s->buswidth = nand_flash_ids[s->chip_id].width >> 3;
<= 16 >> 3 <= 2.
x <= s->ioaddr[offset] << (s->buswidth << 3)
<= max_uint8_t << 16
With x << 24 overflow is possible.
Other cases are similar.
Thus, need to cast return value to uint64_t.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
---
 hw/misc/omap_gpmc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/misc/omap_gpmc.c b/hw/misc/omap_gpmc.c
index 67158eb164..b0a48c71de 100644
--- a/hw/misc/omap_gpmc.c
+++ b/hw/misc/omap_gpmc.c
@@ -139,8 +139,8 @@ static uint64_t omap_nand_read(void *opaque, hwaddr addr,
         if (size == 2) {
             return v;
         }
-        v |= (nand_getio(f->dev) << 16);
-        v |= (nand_getio(f->dev) << 24);
+        v |= ((uint64_t)nand_getio(f->dev) << 16);
+        v |= ((uint64_t)nand_getio(f->dev) << 24);
         return v;
     case OMAP_GPMC_16BIT:
         v = nand_getio(f->dev);
@@ -151,7 +151,7 @@ static uint64_t omap_nand_read(void *opaque, hwaddr addr,
         if (size == 2) {
             return v;
         }
-        v |= (nand_getio(f->dev) << 16);
+        v |= ((uint64_t)nand_getio(f->dev) << 16);
         return v;
     default:
         abort();
-- 
2.30.2

Re: [PATCH] hw/misc: cast nand_getio value to uint64_t

[Alex Bennée]

Tigran Sogomonian <tsogomonian@astralinux.ru> writes:

> s->buswidth = nand_flash_ids[s->chip_id].width >> 3;
> <= 16 >> 3 <= 2.
> x <= s->ioaddr[offset] << (s->buswidth << 3)
> <= max_uint8_t << 16
> With x << 24 overflow is possible.
> Other cases are similar.
> Thus, need to cast return value to uint64_t.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>

This code was removed in 192f75ad11 (hw/misc: Remove omap_gpmc)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] hw/misc: cast nand_getio value to uint64_t

[Тигран Согомонян]

27/12/24 01:49, Alex Bennée пишет:
> Tigran Sogomonian <tsogomonian@astralinux.ru> writes:
>
>> s->buswidth = nand_flash_ids[s->chip_id].width >> 3;
>> <= 16 >> 3 <= 2.
>> x <= s->ioaddr[offset] << (s->buswidth << 3)
>> <= max_uint8_t << 16
>> With x << 24 overflow is possible.
>> Other cases are similar.
>> Thus, need to cast return value to uint64_t.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
> This code was removed in 192f75ad11 (hw/misc: Remove omap_gpmc)
Yes, I saw that upstream master doesn't have this code, but some users 
use stable-9.1. I suggest adding these changes not to the main branch, 
but to the stable-9.1 branch.

Re: [PATCH] hw/misc: cast nand_getio value to uint64_t

[Peter Maydell]

On Fri, 27 Dec 2024 at 10:55, Тигран Согомонян
<tsogomonian@astralinux.ru> wrote:
>
> 27/12/24 01:49, Alex Bennée пишет:
> > Tigran Sogomonian <tsogomonian@astralinux.ru> writes:
> >
> >> s->buswidth = nand_flash_ids[s->chip_id].width >> 3;
> >> <= 16 >> 3 <= 2.
> >> x <= s->ioaddr[offset] << (s->buswidth << 3)
> >> <= max_uint8_t << 16
> >> With x << 24 overflow is possible.
> >> Other cases are similar.
> >> Thus, need to cast return value to uint64_t.
> >>
> >> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >>
> >> Signed-off-by: Tigran Sogomonian <tsogomonian@astralinux.ru>
> > This code was removed in 192f75ad11 (hw/misc: Remove omap_gpmc)
> Yes, I saw that upstream master doesn't have this code, but some users
> use stable-9.1. I suggest adding these changes not to the main branch,
> but to the stable-9.1 branch.

It is not worth the effort. If you want to propose making
a change to be backported to stable it needs more justification
for this, e.g. exactly what the failure is, how users might
run into it, etc. "I ran a static analyser and it produced
a warning" is not enough -- you need to look at the code and
at what the device itself is doing. At which point you'll
find that the function is not used in any situations where
the eventual caller cares about the top 32 bits.

More generally: will you all please *stop* running this
static analyser on anything older than current QEMU
head of git? It is just a waste of your time and ours.

-- PMM