Re: [PATCH] hw/display/virtio-gpu: Always reject invalid scanout bounds

On 3/8/26 12:32, Akihiko Odaki wrote:
> On 2026/08/03 19:17, Philippe Mathieu-Daudé wrote:
>> Hi Akihiko,
>>
>> On 3/8/26 10:45, Akihiko Odaki wrote:
>>> virtio-gpu does not consistently check scanout bounds with wraparound
>>> handling. In the unchecked virgl SET_SCANOUT path, guest dimensions
>>> reach qemu_console_resize(), qemu_create_displaysurface(), and
>>> ultimately qemu_pixman_image_new_shareable(..., &error_abort), so an
>>> invalid rectangle can terminate QEMU. Implement a check with proper
>>> wraparound handling and apply it consistently.
>>>
>>> Fixes: 9d9e152136bd ("virtio-gpu: add 3d mode and virgl rendering 
>>> support.")
>>> Fixes: 32db3c63ae11 ("virtio-gpu: Add virtio_gpu_set_scanout_blob")
>>> Fixes: 7c092f17ccee ("virtio-gpu: Handle resource blob commands")
>>> Fixes: 1dcc6adbc168 ("gfxstream + rutabaga: add initial support for 
>>> gfxstream")
>>> Signed-off-by: Akihiko Odaki <[email protected]>
>>> ---
>>>   include/hw/virtio/virtio-gpu.h   |  5 +++++
>>>   hw/display/virtio-gpu-rutabaga.c |  6 ++++++
>>>   hw/display/virtio-gpu-virgl.c    | 20 +++++++++-----------
>>>   hw/display/virtio-gpu.c          | 36 +++++++++++++++++++++ 
>>> +--------------
>>>   4 files changed, 42 insertions(+), 25 deletions(-)
>>
>>
>>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>>> index 4d46a4eb10fa..3c0e38c1def0 100644
>>> --- a/hw/display/virtio-gpu.c
>>> +++ b/hw/display/virtio-gpu.c
>>> @@ -622,6 +622,26 @@ static uint32_t 
>>> virtio_gpu_format_bytes_pp(pixman_format_code_t format)
>>>       return DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
>>>   }
>>> +bool virtio_gpu_check_scanout_bounds(uint32_t scanout_id, uint32_t 
>>> resource_id,
>>> +                                     uint32_t width, uint32_t height,
>>> +                                     const struct virtio_gpu_rect *r,
>>> +                                     uint32_t *error)
>>> +{
>>> +    if (r->width < 16 ||
>>> +        r->height < 16 ||
>>> +        (uint64_t)r->x + r->width > width ||
>>> +        (uint64_t)r->y + r->height > height) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d 
>>> bounds for"
>>> +                      " resource %d, fb %d %d, rect (%d,%d)+%d,%d\n",
>>> +                      __func__, scanout_id, resource_id, width, height,
>>
>> __func__ will always be "virtio_gpu_check_scanout_bounds", maybe we
>> want the caller instead? Otherwise simply drop?
>>
>> Since we rework this code, in case we are debugging broken guest driver,
>> it could be more helpful to log one LOG_GUEST_ERROR per error type:
>>
>>      if (r->width < 16) { ... }
>>      if (r->height < 16) { ... }
>>      ...
> 
> I don't have a strong opinion here, but if I'm debugging a broken guest, 
> probably I would use the printed function name to look up this code and 
> then figure out what's wrong by comparing with the actual condition.
> 
> This applies to other instances of LOG_GUEST_ERROR; ideally it would be 
> nice if there is some sort of guideline, though I think people have 
> different opinions what such a guideline should say.

Fine.

> 
> Regards,
> Akihiko Odaki
> 
>>
>>> +                      r->x, r->y, r->width, r->height);
>>> +        *error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>>> +        return false;
>>> +    }
>>> +
>>> +    return true;
>>> +}
>>> +
>>>   static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
>>>                                         uint32_t scanout_id,
>>>                                         struct virtio_gpu_framebuffer 
>>> *fb,
>>> @@ -635,20 +655,8 @@ static bool virtio_gpu_do_set_scanout(VirtIOGPU *g,
>>>       scanout = &g->parent_obj.scanout[scanout_id];
>>> -    if (r->x > fb->width ||
>>> -        r->y > fb->height ||
>>
>>  > -        r->width > fb->width ||
>>  > -        r->height > fb->height ||
>>
>> Do we want to remove these checks?

I'm interested by your view on this, I might have misunderstood
part of the change.

Otherwise removing __func__:
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>

>>
>>> -        r->width < 16 ||
>>> -        r->height < 16 ||
>>
>>> -        r->x + r->width > fb->width ||
>>> -        r->y + r->height > fb->height) {
>>> -        qemu_log_mask(LOG_GUEST_ERROR, "%s: illegal scanout %d 
>>> bounds for"
>>> -                      " resource %d, rect (%d,%d)+%d,%d, fb %d %d\n",
>>> -                      __func__, scanout_id, res->resource_id,
>>> -                      r->x, r->y, r->width, r->height,
>>> -                      fb->width, fb->height);
>>> -        *error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>>> +    if (!virtio_gpu_check_scanout_bounds(scanout_id, res->resource_id,
>>> +                                         fb->width, fb->height, r, 
>>> error)) {
>>>           return false;
>>>       }
>>>
>>> ---
>>> base-commit: 299e7557ed15a9a325620698add379a3ce2d1d95
>>> change-id: 20260803-scanout-c7cf28c1890e
>>>
>>> Best regards,
>>> -- 
>>> Akihiko Odaki <[email protected]>
>>>
>>>
>>
> 
>