[Spice-devel] [spice-server PATCH 1/3] red_get_surface_cmd: avoid overflow

[Uri Lublin]

On 07/20/2017 11:45 AM, Frediano Ziglio wrote:
>>
>> On 07/17/2017 11:22 AM, Frediano Ziglio wrote:
>>>>
>>>> Although unlikely, theoretically, multiplying two 32-bit
>>>> numbers may overflow.
>>>>
>>>> Found by coverity.
>>>>
>>>> Signed-off-by: Uri Lublin <uril at redhat.com>
>>>> ---
>>>>    server/red-parse-qxl.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/server/red-parse-qxl.c b/server/red-parse-qxl.c
>>>> index 33f36923a..0ffa5f7d4 100644
>>>> --- a/server/red-parse-qxl.c
>>>> +++ b/server/red-parse-qxl.c
>>>> @@ -1397,7 +1397,7 @@ bool red_get_surface_cmd(RedMemSlotInfo *slots, int
>>>> group_id,
>>>>                return false;
>>>>            }
>>>>    
>>>> -        size = red->u.surface_create.height *
>>>> abs(red->u.surface_create.stride);
>>>> +        size = red->u.surface_create.height *
>>>> (uint64_t)abs(red->u.surface_create.stride);
>>>>            red->u.surface_create.data =
>>>>                (uint8_t*)memslot_get_virt(slots,
>>>>                qxl->u.surface_create.data,
>>>>                size, group_id, &error);
>>>>            if (error) {
>>>
>>> This overflow is already handled by red_validate_surface call.
>>
>> Indeed.
>> I'll drop this patch.
>>
>>
>>> Also note that silently the uint64_t size is converted to 32 bit calling
>>> memslot_get_virt so maybe would be better to change size to uint32_t.
>>>
>>> Frediano
>>>
>>
>> Thanks,
>>       Uri.
>>
>>
> 
> Have you try to change the variable to uint32_t ?
> This should make everything happy (coverity and code).
> 
> Frediano
> 

I did not try it.
I'll give it a try next time I'm running coverity.

I see that many other calls to memslot_get_virt are
passing sizeof(something) as size parameter so basically
also silently converting to 32 bit.

An overflow would still be possible with a uint32_t size;
I do not know if coverity checks that too or not.

Thanks,
     Uri.