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

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) { ... }
     ...

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