[Qemu-devel] [PATCH] migration: fix potential overflow in multifd send

[Qemu-devel] [PATCH] migration: fix potential overflow in multifd send

[Peter Xu]

I would guess it won't happen normally, but this should ease Coverity.

>>>     CID 1394385:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>>     Potentially overflowing expression "pages->used * 8192U" with type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "uint64_t" (64 bits, unsigned).
854         transferred = pages->used * TARGET_PAGE_SIZE + p->packet_len;

Fixes: CID 1394385
CC: Juan Quintela <quintela@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 52dd678092..fdd108475c 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -851,7 +851,7 @@ static void multifd_send_pages(void)
     p->pages->block = NULL;
     multifd_send_state->pages = p->pages;
     p->pages = pages;
-    transferred = pages->used * TARGET_PAGE_SIZE + p->packet_len;
+    transferred = ((uint64_t) pages->used) * TARGET_PAGE_SIZE + p->packet_len;
     ram_counters.multifd_bytes += transferred;
     ram_counters.transferred += transferred;;
     qemu_mutex_unlock(&p->mutex);
-- 
2.17.1

Re: [Qemu-devel] [PATCH] migration: fix potential overflow in multifd send

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> I would guess it won't happen normally, but this should ease Coverity.
>
>>>>     CID 1394385:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
>>>>     Potentially overflowing expression "pages->used * 8192U" with
>>>> type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit
>>>> arithmetic, and then used in a context that expects an expression
>>>> of type "uint64_t" (64 bits, unsigned).
> 854         transferred = pages->used * TARGET_PAGE_SIZE + p->packet_len;
>
> Fixes: CID 1394385
> CC: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

a - I hate C promotion rules
b - why gcc don't warn me
c - it don't matter.  If the size of the package is bigger than 4GB, we
    have other problems already.

Thanks, Juan.

Re: [Qemu-devel] [PATCH] migration: fix potential overflow in multifd send

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> I would guess it won't happen normally, but this should ease Coverity.
> 
> >>>     CID 1394385:  Integer handling issues  (OVERFLOW_BEFORE_WIDEN)
> >>>     Potentially overflowing expression "pages->used * 8192U" with type "unsigned int" (32 bits, unsigned) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "uint64_t" (64 bits, unsigned).
> 854         transferred = pages->used * TARGET_PAGE_SIZE + p->packet_len;
> 
> Fixes: CID 1394385
> CC: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Queued

> ---
>  migration/ram.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 52dd678092..fdd108475c 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -851,7 +851,7 @@ static void multifd_send_pages(void)
>      p->pages->block = NULL;
>      multifd_send_state->pages = p->pages;
>      p->pages = pages;
> -    transferred = pages->used * TARGET_PAGE_SIZE + p->packet_len;
> +    transferred = ((uint64_t) pages->used) * TARGET_PAGE_SIZE + p->packet_len;
>      ram_counters.multifd_bytes += transferred;
>      ram_counters.transferred += transferred;;
>      qemu_mutex_unlock(&p->mutex);
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: fix potential overflow in multifd send

[Eric Blake]

On 07/24/2018 10:59 AM, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
>> I would guess it won't happen normally, but this should ease Coverity.
>>

>> +++ b/migration/ram.c
>> @@ -851,7 +851,7 @@ static void multifd_send_pages(void)
>>       p->pages->block = NULL;
>>       multifd_send_state->pages = p->pages;
>>       p->pages = pages;
>> -    transferred = pages->used * TARGET_PAGE_SIZE + p->packet_len;
>> +    transferred = ((uint64_t) pages->used) * TARGET_PAGE_SIZE + p->packet_len;

The outer () are not strictly necessary, as casts bind tighter than 
multiply.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org