[Qemu-devel] [PATCH] hw/ppc/ppc405_uc.c: Avoid integer overflows

Peter Maydell posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1487684021-22713-1-git-send-email-peter.maydell@linaro.org
Test checkpatch passed
Test docker passed
Test s390x passed
hw/ppc/ppc405_uc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
[Qemu-devel] [PATCH] hw/ppc/ppc405_uc.c: Avoid integer overflows
Posted by Peter Maydell 7 years, 1 month ago
When performing clock calculations, the ppc405_uc code
has several places where it multiplies together two
32-bit variables and assigns the result to a 64-bit
variable. This doesn't quite do what is intended because
C will compute a 32-bit multiply result. Add casts to
ensure we don't truncate the result.

(Spotted by Coverity, CID 1005504, 1005505.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ppc/ppc405_uc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
index d6d3fc2..d5df94a 100644
--- a/hw/ppc/ppc405_uc.c
+++ b/hw/ppc/ppc405_uc.c
@@ -1881,7 +1881,7 @@ static void ppc405cr_clk_setup (ppc405cr_cpc_t *cpc)
         D1 = (((cpc->pllmr >> 20) - 1) & 0xF) + 1; /* FBDV */
         D2 = 8 - ((cpc->pllmr >> 16) & 0x7); /* FWDVA */
         M = D0 * D1 * D2;
-        VCO_out = cpc->sysclk * M;
+        VCO_out = (uint64_t)cpc->sysclk * M;
         if (VCO_out < 400000000 || VCO_out > 800000000) {
             /* PLL cannot lock */
             cpc->pllmr &= ~0x80000000;
@@ -1892,7 +1892,7 @@ static void ppc405cr_clk_setup (ppc405cr_cpc_t *cpc)
         /* Bypass PLL */
     bypass_pll:
         M = D0;
-        PLL_out = cpc->sysclk * M;
+        PLL_out = (uint64_t)cpc->sysclk * M;
     }
     CPU_clk = PLL_out;
     if (cpc->cr1 & 0x00800000)
@@ -2242,7 +2242,7 @@ static void ppc405ep_compute_clocks (ppc405ep_cpc_t *cpc)
 #ifdef DEBUG_CLOCKS_LL
         printf("FWDA %01" PRIx32 " %d\n", (cpc->pllmr[1] >> 16) & 0x7, D);
 #endif
-        VCO_out = cpc->sysclk * M * D;
+        VCO_out = (uint64_t)cpc->sysclk * M * D;
         if (VCO_out < 500000000UL || VCO_out > 1000000000UL) {
             /* Error - unlock the PLL */
             printf("VCO out of range %" PRIu64 "\n", VCO_out);
-- 
2.7.4


Re: [Qemu-devel] [PATCH] hw/ppc/ppc405_uc.c: Avoid integer overflows
Posted by Philippe Mathieu-Daudé 7 years, 1 month ago
On 02/21/2017 10:33 AM, Peter Maydell wrote:
> When performing clock calculations, the ppc405_uc code
> has several places where it multiplies together two
> 32-bit variables and assigns the result to a 64-bit
> variable. This doesn't quite do what is intended because
> C will compute a 32-bit multiply result. Add casts to
> ensure we don't truncate the result.
>
> (Spotted by Coverity, CID 1005504, 1005505.)

well hidden...

>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/ppc/ppc405_uc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index d6d3fc2..d5df94a 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -1881,7 +1881,7 @@ static void ppc405cr_clk_setup (ppc405cr_cpc_t *cpc)
>          D1 = (((cpc->pllmr >> 20) - 1) & 0xF) + 1; /* FBDV */
>          D2 = 8 - ((cpc->pllmr >> 16) & 0x7); /* FWDVA */
>          M = D0 * D1 * D2;
> -        VCO_out = cpc->sysclk * M;
> +        VCO_out = (uint64_t)cpc->sysclk * M;
>          if (VCO_out < 400000000 || VCO_out > 800000000) {
>              /* PLL cannot lock */
>              cpc->pllmr &= ~0x80000000;
> @@ -1892,7 +1892,7 @@ static void ppc405cr_clk_setup (ppc405cr_cpc_t *cpc)
>          /* Bypass PLL */
>      bypass_pll:
>          M = D0;
> -        PLL_out = cpc->sysclk * M;
> +        PLL_out = (uint64_t)cpc->sysclk * M;
>      }
>      CPU_clk = PLL_out;
>      if (cpc->cr1 & 0x00800000)
> @@ -2242,7 +2242,7 @@ static void ppc405ep_compute_clocks (ppc405ep_cpc_t *cpc)
>  #ifdef DEBUG_CLOCKS_LL
>          printf("FWDA %01" PRIx32 " %d\n", (cpc->pllmr[1] >> 16) & 0x7, D);
>  #endif
> -        VCO_out = cpc->sysclk * M * D;
> +        VCO_out = (uint64_t)cpc->sysclk * M * D;
>          if (VCO_out < 500000000UL || VCO_out > 1000000000UL) {
>              /* Error - unlock the PLL */
>              printf("VCO out of range %" PRIu64 "\n", VCO_out);
>

Re: [Qemu-devel] [PATCH] hw/ppc/ppc405_uc.c: Avoid integer overflows
Posted by David Gibson 7 years, 1 month ago
On Tue, Feb 21, 2017 at 01:33:41PM +0000, Peter Maydell wrote:
> When performing clock calculations, the ppc405_uc code
> has several places where it multiplies together two
> 32-bit variables and assigns the result to a 64-bit
> variable. This doesn't quite do what is intended because
> C will compute a 32-bit multiply result. Add casts to
> ensure we don't truncate the result.
> 
> (Spotted by Coverity, CID 1005504, 1005505.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Ouch.  Applied to ppc-for-2.9.

> ---
>  hw/ppc/ppc405_uc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/ppc405_uc.c b/hw/ppc/ppc405_uc.c
> index d6d3fc2..d5df94a 100644
> --- a/hw/ppc/ppc405_uc.c
> +++ b/hw/ppc/ppc405_uc.c
> @@ -1881,7 +1881,7 @@ static void ppc405cr_clk_setup (ppc405cr_cpc_t *cpc)
>          D1 = (((cpc->pllmr >> 20) - 1) & 0xF) + 1; /* FBDV */
>          D2 = 8 - ((cpc->pllmr >> 16) & 0x7); /* FWDVA */
>          M = D0 * D1 * D2;
> -        VCO_out = cpc->sysclk * M;
> +        VCO_out = (uint64_t)cpc->sysclk * M;
>          if (VCO_out < 400000000 || VCO_out > 800000000) {
>              /* PLL cannot lock */
>              cpc->pllmr &= ~0x80000000;
> @@ -1892,7 +1892,7 @@ static void ppc405cr_clk_setup (ppc405cr_cpc_t *cpc)
>          /* Bypass PLL */
>      bypass_pll:
>          M = D0;
> -        PLL_out = cpc->sysclk * M;
> +        PLL_out = (uint64_t)cpc->sysclk * M;
>      }
>      CPU_clk = PLL_out;
>      if (cpc->cr1 & 0x00800000)
> @@ -2242,7 +2242,7 @@ static void ppc405ep_compute_clocks (ppc405ep_cpc_t *cpc)
>  #ifdef DEBUG_CLOCKS_LL
>          printf("FWDA %01" PRIx32 " %d\n", (cpc->pllmr[1] >> 16) & 0x7, D);
>  #endif
> -        VCO_out = cpc->sysclk * M * D;
> +        VCO_out = (uint64_t)cpc->sysclk * M * D;
>          if (VCO_out < 500000000UL || VCO_out > 1000000000UL) {
>              /* Error - unlock the PLL */
>              printf("VCO out of range %" PRIu64 "\n", VCO_out);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson