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

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.

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?
> 
>> -        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]>
>>
>>
>