linux-generic: system info: fix overflowed returned value

Message ID 1464286981-2052-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov May 26, 2016, 6:23 p.m.
https://bugs.linaro.org/show_bug.cgi?id=2253

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
---
 platform/linux-generic/odp_system_info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Maxim Uvarov May 27, 2016, 3:41 p.m. | #1
please review,
Maxim.

On 05/26/16 21:23, Maxim Uvarov wrote:
> https://bugs.linaro.org/show_bug.cgi?id=2253
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>   platform/linux-generic/odp_system_info.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c
> index fca35ce..0a09443 100644
> --- a/platform/linux-generic/odp_system_info.c
> +++ b/platform/linux-generic/odp_system_info.c
> @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void)
>   		if (sscanf(str, "Hugepagesize:   %8lu kB", &sz) == 1) {
>   			ODP_DBG("defaut hp size is %" PRIu64 " kB\n", sz);
>   			fclose(file);
> -			return (uint64_t)sz * 1024;
> +			return (uint64_t)sz * 1024ULL;
>   		}
>   	}
>
Bill Fischofer May 27, 2016, 6:16 p.m. | #2
On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> https://bugs.linaro.org/show_bug.cgi?id=2253
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
> ---
>  platform/linux-generic/odp_system_info.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/platform/linux-generic/odp_system_info.c
> b/platform/linux-generic/odp_system_info.c
> index fca35ce..0a09443 100644
> --- a/platform/linux-generic/odp_system_info.c
> +++ b/platform/linux-generic/odp_system_info.c
> @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void)
>                 if (sscanf(str, "Hugepagesize:   %8lu kB", &sz) == 1) {
>                         ODP_DBG("defaut hp size is %" PRIu64 " kB\n", sz);
>                         fclose(file);
> -                       return (uint64_t)sz * 1024;
> +                       return (uint64_t)sz * 1024ULL;
>

I'm not sure that this is fixing the issue that Coverity is complaining
about. What it's saying is that if sz is >= UINT64_MAX / 1024 then
multiplying this expression by 1024 will cause an overflow. Coverity
doesn't understand that sz will never be anywhere near that large.

The solution would be to mask sz so that Coverity can see that the
multiplication can never overflow. Or we could just mark this as a false
positive.  I think either would be acceptable.



>                 }
>         }
>
> --
> 2.7.1.250.gff4ea60
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Maxim Uvarov May 27, 2016, 6:21 p.m. | #3
On 05/27/16 21:16, Bill Fischofer wrote:
>
>
> On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>     https://bugs.linaro.org/show_bug.cgi?id=2253
>
>     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>     <mailto:maxim.uvarov@linaro.org>>
>     ---
>      platform/linux-generic/odp_system_info.c | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
>
>     diff --git a/platform/linux-generic/odp_system_info.c
>     b/platform/linux-generic/odp_system_info.c
>     index fca35ce..0a09443 100644
>     --- a/platform/linux-generic/odp_system_info.c
>     +++ b/platform/linux-generic/odp_system_info.c
>     @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void)
>                     if (sscanf(str, "Hugepagesize:   %8lu kB", &sz) ==
>     1) {
>                             ODP_DBG("defaut hp size is %" PRIu64 "
>     kB\n", sz);
>                             fclose(file);
>     -                       return (uint64_t)sz * 1024;
>     +                       return (uint64_t)sz * 1024ULL;
>
>
> I'm not sure that this is fixing the issue that Coverity is 
> complaining about. What it's saying is that if sz is >= UINT64_MAX / 
> 1024 then multiplying this expression by 1024 will cause an overflow. 
> Coverity doesn't understand that sz will never be anywhere near that 
> large.
>
> The solution would be to mask sz so that Coverity can see that the 
> multiplication can never overflow. Or we could just mark this as a 
> false positive.  I think either would be acceptable.

return (uint64_t)sz << 10

will work here?
>
>                     }
>             }
>
>     --
>     2.7.1.250.gff4ea60
>
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Bill Fischofer May 27, 2016, 6:40 p.m. | #4
On Fri, May 27, 2016 at 1:21 PM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> On 05/27/16 21:16, Bill Fischofer wrote:
>
>>
>>
>> On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov <maxim.uvarov@linaro.org
>> <mailto:maxim.uvarov@linaro.org>> wrote:
>>
>>     https://bugs.linaro.org/show_bug.cgi?id=2253
>>
>>     Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>     <mailto:maxim.uvarov@linaro.org>>
>>     ---
>>      platform/linux-generic/odp_system_info.c | 2 +-
>>      1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>     diff --git a/platform/linux-generic/odp_system_info.c
>>     b/platform/linux-generic/odp_system_info.c
>>     index fca35ce..0a09443 100644
>>     --- a/platform/linux-generic/odp_system_info.c
>>     +++ b/platform/linux-generic/odp_system_info.c
>>     @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void)
>>                     if (sscanf(str, "Hugepagesize:   %8lu kB", &sz) ==
>>     1) {
>>                             ODP_DBG("defaut hp size is %" PRIu64 "
>>     kB\n", sz);
>>                             fclose(file);
>>     -                       return (uint64_t)sz * 1024;
>>     +                       return (uint64_t)sz * 1024ULL;
>>
>>
>> I'm not sure that this is fixing the issue that Coverity is complaining
>> about. What it's saying is that if sz is >= UINT64_MAX / 1024 then
>> multiplying this expression by 1024 will cause an overflow. Coverity
>> doesn't understand that sz will never be anywhere near that large.
>>
>> The solution would be to mask sz so that Coverity can see that the
>> multiplication can never overflow. Or we could just mark this as a false
>> positive.  I think either would be acceptable.
>>
>
> return (uint64_t)sz << 10
>
> will work here?
>

That would have the same result since the compiler already optimizes
multiplications by known powers of two into shifts.

What would likely keep Coverity happy would be something like:

#define RANGE_MASK (UINT64_MAX / 1024)

...

return ((uint64_t)sz & RANGE_MASK) * 1024;


>
>>                     }
>>             }
>>
>>     --
>>     2.7.1.250.gff4ea60
>>
>>     _______________________________________________
>>     lng-odp mailing list
>>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>     https://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>
Maxim Uvarov May 27, 2016, 7:38 p.m. | #5
On 05/27/16 21:40, Bill Fischofer wrote:
>
>
> On Fri, May 27, 2016 at 1:21 PM, Maxim Uvarov <maxim.uvarov@linaro.org 
> <mailto:maxim.uvarov@linaro.org>> wrote:
>
>     On 05/27/16 21:16, Bill Fischofer wrote:
>
>
>
>         On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov
>         <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>         <mailto:maxim.uvarov@linaro.org
>         <mailto:maxim.uvarov@linaro.org>>> wrote:
>
>         https://bugs.linaro.org/show_bug.cgi?id=2253
>
>             Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>         <mailto:maxim.uvarov@linaro.org>
>             <mailto:maxim.uvarov@linaro.org
>         <mailto:maxim.uvarov@linaro.org>>>
>             ---
>              platform/linux-generic/odp_system_info.c | 2 +-
>              1 file changed, 1 insertion(+), 1 deletion(-)
>
>             diff --git a/platform/linux-generic/odp_system_info.c
>             b/platform/linux-generic/odp_system_info.c
>             index fca35ce..0a09443 100644
>             --- a/platform/linux-generic/odp_system_info.c
>             +++ b/platform/linux-generic/odp_system_info.c
>             @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void)
>                             if (sscanf(str, "Hugepagesize:  %8lu kB",
>         &sz) ==
>             1) {
>                                     ODP_DBG("defaut hp size is %" PRIu64 "
>             kB\n", sz);
>                                     fclose(file);
>             -                       return (uint64_t)sz * 1024;
>             +                       return (uint64_t)sz * 1024ULL;
>
>
>         I'm not sure that this is fixing the issue that Coverity is
>         complaining about. What it's saying is that if sz is >=
>         UINT64_MAX / 1024 then multiplying this expression by 1024
>         will cause an overflow. Coverity doesn't understand that sz
>         will never be anywhere near that large.
>
>         The solution would be to mask sz so that Coverity can see that
>         the multiplication can never overflow. Or we could just mark
>         this as a false positive.  I think either would be acceptable.
>
>
>     return (uint64_t)sz << 10
>
>     will work here?
>
>
> That would have the same result since the compiler already optimizes 
> multiplications by known powers of two into shifts.
>
> What would likely keep Coverity happy would be something like:
>
> #define RANGE_MASK (UINT64_MAX / 1024)
>
> ...
>
> return ((uint64_t)sz & RANGE_MASK) * 1024;


I think we should have such reading in number of places. Or we need to 
do some internal function to scan in megabytes or just set this as false 
positive in Coverity. Because it's parsed /proc there can not be number 
more than in unsigned long and uint64_t has to be enough for any case.

Maxim.

>
>                             }
>                     }
>
>             --
>             2.7.1.250.gff4ea60
>
>             _______________________________________________
>             lng-odp mailing list
>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>         <mailto:lng-odp@lists.linaro.org
>         <mailto:lng-odp@lists.linaro.org>>
>         https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
Bill Fischofer May 27, 2016, 8:49 p.m. | #6
On Fri, May 27, 2016 at 2:38 PM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> On 05/27/16 21:40, Bill Fischofer wrote:
>
>>
>>
>> On Fri, May 27, 2016 at 1:21 PM, Maxim Uvarov <maxim.uvarov@linaro.org
>> <mailto:maxim.uvarov@linaro.org>> wrote:
>>
>>     On 05/27/16 21:16, Bill Fischofer wrote:
>>
>>
>>
>>         On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov
>>         <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>
>>         <mailto:maxim.uvarov@linaro.org
>>         <mailto:maxim.uvarov@linaro.org>>> wrote:
>>
>>         https://bugs.linaro.org/show_bug.cgi?id=2253
>>
>>             Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org
>>         <mailto:maxim.uvarov@linaro.org>
>>             <mailto:maxim.uvarov@linaro.org
>>
>>         <mailto:maxim.uvarov@linaro.org>>>
>>             ---
>>              platform/linux-generic/odp_system_info.c | 2 +-
>>              1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>             diff --git a/platform/linux-generic/odp_system_info.c
>>             b/platform/linux-generic/odp_system_info.c
>>             index fca35ce..0a09443 100644
>>             --- a/platform/linux-generic/odp_system_info.c
>>             +++ b/platform/linux-generic/odp_system_info.c
>>             @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void)
>>                             if (sscanf(str, "Hugepagesize:  %8lu kB",
>>         &sz) ==
>>             1) {
>>                                     ODP_DBG("defaut hp size is %" PRIu64 "
>>             kB\n", sz);
>>                                     fclose(file);
>>             -                       return (uint64_t)sz * 1024;
>>             +                       return (uint64_t)sz * 1024ULL;
>>
>>
>>         I'm not sure that this is fixing the issue that Coverity is
>>         complaining about. What it's saying is that if sz is >=
>>         UINT64_MAX / 1024 then multiplying this expression by 1024
>>         will cause an overflow. Coverity doesn't understand that sz
>>         will never be anywhere near that large.
>>
>>         The solution would be to mask sz so that Coverity can see that
>>         the multiplication can never overflow. Or we could just mark
>>         this as a false positive.  I think either would be acceptable.
>>
>>
>>     return (uint64_t)sz << 10
>>
>>     will work here?
>>
>>
>> That would have the same result since the compiler already optimizes
>> multiplications by known powers of two into shifts.
>>
>> What would likely keep Coverity happy would be something like:
>>
>> #define RANGE_MASK (UINT64_MAX / 1024)
>>
>> ...
>>
>> return ((uint64_t)sz & RANGE_MASK) * 1024;
>>
>
>
> I think we should have such reading in number of places. Or we need to do
> some internal function to scan in megabytes or just set this as false
> positive in Coverity. Because it's parsed /proc there can not be number
> more than in unsigned long and uint64_t has to be enough for any case.
>
> Maxim.
>

I agree. I've marked this as a false positive in Coverity. We can close
this bug as invalid.


>
>
>>                             }
>>                     }
>>
>>             --
>>             2.7.1.250.gff4ea60
>>
>>             _______________________________________________
>>             lng-odp mailing list
>>         lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>>         <mailto:lng-odp@lists.linaro.org
>>         <mailto:lng-odp@lists.linaro.org>>
>>         https://lists.linaro.org/mailman/listinfo/lng-odp
>>
>>
>>
>>
>>
>

Patch

diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c
index fca35ce..0a09443 100644
--- a/platform/linux-generic/odp_system_info.c
+++ b/platform/linux-generic/odp_system_info.c
@@ -86,7 +86,7 @@  static uint64_t default_huge_page_size(void)
 		if (sscanf(str, "Hugepagesize:   %8lu kB", &sz) == 1) {
 			ODP_DBG("defaut hp size is %" PRIu64 " kB\n", sz);
 			fclose(file);
-			return (uint64_t)sz * 1024;
+			return (uint64_t)sz * 1024ULL;
 		}
 	}