[v3] util/cutils: Fix Coverity array overrun in freq_to_str()

[PATCH-for-5.2 v3] util/cutils: Fix Coverity array overrun in freq_to_str()

Posted by Philippe Mathieu-Daudé 5 years, 3 months ago

Fix Coverity CID 1435957:  Memory - illegal accesses (OVERRUN):

>>> Overrunning array "suffixes" of 7 8-byte elements at element
    index 7 (byte offset 63) using index "idx" (which evaluates to 7).

Note, the biggest input value freq_to_str() can accept is UINT64_MAX,
which is ~18.446 EHz, less than 1000 EHz.

Reported-by: Eduardo Habkost <ehabkost@redhat.com>
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
v3: Follow Peter's suggestion
---
 util/cutils.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/cutils.c b/util/cutils.c
index c395974fab4..2f869a843a5 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -891,10 +891,11 @@ char *freq_to_str(uint64_t freq_hz)
     double freq = freq_hz;
     size_t idx = 0;
 
-    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
+    while (freq >= 1000.0) {
         freq /= 1000.0;
         idx++;
     }
+    assert(idx < ARRAY_SIZE(suffixes));
 
     return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);
 }
-- 
2.26.2

Re: [PATCH-for-5.2 v3] util/cutils: Fix Coverity array overrun in freq_to_str()

Posted by Eduardo Habkost 5 years, 3 months ago

On Sun, Nov 01, 2020 at 10:57:55PM +0100, Philippe Mathieu-Daudé wrote:
> Fix Coverity CID 1435957:  Memory - illegal accesses (OVERRUN):
> 
> >>> Overrunning array "suffixes" of 7 8-byte elements at element
>     index 7 (byte offset 63) using index "idx" (which evaluates to 7).
> 
> Note, the biggest input value freq_to_str() can accept is UINT64_MAX,
> which is ~18.446 EHz, less than 1000 EHz.
> 
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

Thanks for taking the time to fix this!

-- 
Eduardo

Re: [PATCH-for-5.2 v3] util/cutils: Fix Coverity array overrun in freq_to_str()

Posted by Luc Michel 5 years, 3 months ago

On 22:57 Sun 01 Nov     , Philippe Mathieu-Daudé wrote:
> Fix Coverity CID 1435957:  Memory - illegal accesses (OVERRUN):
> 
> >>> Overrunning array "suffixes" of 7 8-byte elements at element
>     index 7 (byte offset 63) using index "idx" (which evaluates to 7).
> 
> Note, the biggest input value freq_to_str() can accept is UINT64_MAX,
> which is ~18.446 EHz, less than 1000 EHz.
> 
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Luc Michel <luc@lmichel.fr>

> ---
> v3: Follow Peter's suggestion
> ---
>  util/cutils.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index c395974fab4..2f869a843a5 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -891,10 +891,11 @@ char *freq_to_str(uint64_t freq_hz)
>      double freq = freq_hz;
>      size_t idx = 0;
>  
> -    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
> +    while (freq >= 1000.0) {
>          freq /= 1000.0;
>          idx++;
>      }
> +    assert(idx < ARRAY_SIZE(suffixes));
>  
>      return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);
>  }
> -- 
> 2.26.2
> 

--

Re: [PATCH-for-5.2 v3] util/cutils: Fix Coverity array overrun in freq_to_str()

Posted by Peter Maydell 5 years, 2 months ago

On Sun, 1 Nov 2020 at 21:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Fix Coverity CID 1435957:  Memory - illegal accesses (OVERRUN):
>
> >>> Overrunning array "suffixes" of 7 8-byte elements at element
>     index 7 (byte offset 63) using index "idx" (which evaluates to 7).
>
> Note, the biggest input value freq_to_str() can accept is UINT64_MAX,
> which is ~18.446 EHz, less than 1000 EHz.
>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> v3: Follow Peter's suggestion

Since nobody else has picked this up I'll take it via
target-arm.next.

thanks
-- PMM

Re: [PATCH-for-5.2 v3] util/cutils: Fix Coverity array overrun in freq_to_str()

Posted by Peter Maydell 5 years, 3 months ago

On Sun, 1 Nov 2020 at 21:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Fix Coverity CID 1435957:  Memory - illegal accesses (OVERRUN):
>
> >>> Overrunning array "suffixes" of 7 8-byte elements at element
>     index 7 (byte offset 63) using index "idx" (which evaluates to 7).
>
> Note, the biggest input value freq_to_str() can accept is UINT64_MAX,
> which is ~18.446 EHz, less than 1000 EHz.
>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> v3: Follow Peter's suggestion
> ---
>  util/cutils.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/util/cutils.c b/util/cutils.c
> index c395974fab4..2f869a843a5 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -891,10 +891,11 @@ char *freq_to_str(uint64_t freq_hz)
>      double freq = freq_hz;
>      size_t idx = 0;
>
> -    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
> +    while (freq >= 1000.0) {
>          freq /= 1000.0;
>          idx++;
>      }
> +    assert(idx < ARRAY_SIZE(suffixes));
>
>      return g_strdup_printf("%0.3g %sHz", freq, suffixes[idx]);
>  }
> --

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

thanks
-- PMM