aspeed/smc: Fix potential overflow

[PATCH] aspeed/smc: Fix potential overflow

Cédric Le Goater posted 1 patch 3 years, 7 months ago

Download mbox

Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220628165512.1133590-1-clg@kaod.org

Maintainers: "Cédric Le Goater" <clg@kaod.org>, Peter Maydell <peter.maydell@linaro.org>, Andrew Jeffery <andrew@aj.id.au>, Joel Stanley <joel@jms.id.au>, Alistair Francis <alistair@alistair23.me>

hw/ssi/aspeed_smc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

Expand all Fold all

[PATCH] aspeed/smc: Fix potential overflow

Posted by Cédric Le Goater 3 years, 7 months ago

Coverity warns that "ssi_transfer(s->spi, 0U) << 8 * i" might overflow
because the expression is evaluated using 32-bit arithmetic and then
used in a context expecting a uint64_t.

Fixes: Coverity CID 1487244
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index d2b1dde604e3..26640539ae64 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -490,7 +490,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
     switch (aspeed_smc_flash_mode(fl)) {
     case CTRL_USERMODE:
         for (i = 0; i < size; i++) {
-            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
+            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
         }
         break;
     case CTRL_READMODE:
@@ -499,7 +499,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
         aspeed_smc_flash_setup(fl, addr);
 
         for (i = 0; i < size; i++) {
-            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
+            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
         }
 
         aspeed_smc_flash_unselect(fl);
-- 
2.35.3

Re: [PATCH] aspeed/smc: Fix potential overflow

Posted by Joel Stanley 3 years, 7 months ago

On Tue, 28 Jun 2022 at 16:55, Cédric Le Goater <clg@kaod.org> wrote:
>
> Coverity warns that "ssi_transfer(s->spi, 0U) << 8 * i" might overflow
> because the expression is evaluated using 32-bit arithmetic and then
> used in a context expecting a uint64_t.

Would it make sense to also place a limit on "size"?

assert(size < something)

>
> Fixes: Coverity CID 1487244
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ssi/aspeed_smc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index d2b1dde604e3..26640539ae64 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -490,7 +490,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>      switch (aspeed_smc_flash_mode(fl)) {
>      case CTRL_USERMODE:
>          for (i = 0; i < size; i++) {
> -            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
> +            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
>          }
>          break;
>      case CTRL_READMODE:
> @@ -499,7 +499,7 @@ static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>          aspeed_smc_flash_setup(fl, addr);
>
>          for (i = 0; i < size; i++) {
> -            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
> +            ret |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
>          }
>
>          aspeed_smc_flash_unselect(fl);
> --
> 2.35.3
>