conf: Remove multiplication to avoid overflow

[PATCH] conf: Remove multiplication to avoid overflow

Posted by Egor Makrushin 4 months, 1 week ago

Multiplication results in integer overflow.
Replace value of 6th agrument with ULLONG_MAX.

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

Fixes: 04bd77a19f ("conf: Move and rename virDomainParseScaledValue()")
Signed-off-by: Egor Makrushin <emakrushin@astralinux.ru>
---
 src/conf/domain_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 58a985fc5d..871fd3a874 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8523,7 +8523,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt,
             unsigned long long bytes;
             if ((rc = virParseScaledValue("./pcihole64", NULL,
                                           ctxt, &bytes, 1024,
-                                          1024ULL * ULONG_MAX, false)) < 0)
+                                          ULLONG_MAX, false)) < 0)
                 return NULL;
 
             if (rc == 1)
-- 
2.30.2
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH] conf: Remove multiplication to avoid overflow

Posted by Michal Prívozník 4 months, 1 week ago

On 12/19/23 12:27, Egor Makrushin wrote:
> Multiplication results in integer overflow.
> Replace value of 6th agrument with ULLONG_MAX.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 04bd77a19f ("conf: Move and rename virDomainParseScaledValue()")
> Signed-off-by: Egor Makrushin <emakrushin@astralinux.ru>
> ---
>  src/conf/domain_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 58a985fc5d..871fd3a874 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8523,7 +8523,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt,
>              unsigned long long bytes;
>              if ((rc = virParseScaledValue("./pcihole64", NULL,
>                                            ctxt, &bytes, 1024,
> -                                          1024ULL * ULONG_MAX, false)) < 0)
> +                                          ULLONG_MAX, false)) < 0)

So IIUC, overflow happens when sizeof(ulong) == sizeof(ull). Now, the
reason there is 1024 * ULONG_MAX is because ..

>                  return NULL;
>  
>              if (rc == 1)

... down here, 'bytes' is divided by 1024 and stored in an ulong
variable. If we do the change you are suggesting then there is no
overflow up there, but there might be an overflow here, because at least
on one of my 32bit machines ulong is 4 bytes, and ulong long is 8 bytes.

A proper fix IMO would be to change the type of the variable
(def->opts.pciopts.pcihole64size) to ULL and then we can pass ULLONG_MAX
here. Alternatively, we may have some compile time evaluation of
maximums and decide whether we need the multiplication by 1024 or not.

Michal
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH] conf: Remove multiplication to avoid overflow

Posted by Peter Krempa 4 months, 1 week ago

On Tue, Dec 19, 2023 at 14:27:11 +0300, Egor Makrushin wrote:
> Multiplication results in integer overflow.
> Replace value of 6th agrument with ULLONG_MAX.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 04bd77a19f ("conf: Move and rename virDomainParseScaledValue()")

The multiplication was there before this patch, since that's just moving
the code. If you want to use the 'Fixes' tag make sure to find the
commit that actually added the problem.

I can drop this tag if you want or you can find the proper commit.

> Signed-off-by: Egor Makrushin <emakrushin@astralinux.ru>
> ---
>  src/conf/domain_conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 58a985fc5d..871fd3a874 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8523,7 +8523,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt,
>              unsigned long long bytes;
>              if ((rc = virParseScaledValue("./pcihole64", NULL,
>                                            ctxt, &bytes, 1024,
> -                                          1024ULL * ULONG_MAX, false)) < 0)
> +                                          ULLONG_MAX, false)) < 0)
>                  return NULL;
>  
>              if (rc == 1)

Reviewed-by: Peter Krempa <pkrempa@redhat.com>
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH] conf: Remove multiplication to avoid overflow

Posted by Egor Makrushin 4 months, 1 week ago

19/12/23 17:24, Peter Krempa пишет:
> On Tue, Dec 19, 2023 at 14:27:11 +0300, Egor Makrushin wrote:
>> Multiplication results in integer overflow.
>> Replace value of 6th agrument with ULLONG_MAX.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: 04bd77a19f ("conf: Move and rename virDomainParseScaledValue()")
> The multiplication was there before this patch, since that's just moving
> the code. If you want to use the 'Fixes' tag make sure to find the
> commit that actually added the problem.
>
> I can drop this tag if you want or you can find the proper commit.
Thanks for the remark. I think it would be better to remove this tag,
which I will do in the next version of the patch.
>> Signed-off-by: Egor Makrushin <emakrushin@astralinux.ru>
>> ---
>>   src/conf/domain_conf.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 58a985fc5d..871fd3a874 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -8523,7 +8523,7 @@ virDomainControllerDefParseXML(virDomainXMLOption *xmlopt,
>>               unsigned long long bytes;
>>               if ((rc = virParseScaledValue("./pcihole64", NULL,
>>                                             ctxt, &bytes, 1024,
>> -                                          1024ULL * ULONG_MAX, false)) < 0)
>> +                                          ULLONG_MAX, false)) < 0)
>>                   return NULL;
>>   
>>               if (rc == 1)
> Reviewed-by: Peter Krempa <pkrempa@redhat.com>
>
Egor Makrushin
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org