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>

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