Re: [PATCH v2] hw/display/qxl: validate primary surface stride against width

On Sat, Jul 25, 2026 at 4:24=E2=80=AFPM <[email protected]> wrote=
:
>
> From: Marc-Andr=C3=A9 Lureau <[email protected]>
>
> The existing validation in qxl_create_guest_primary() checks that
> abs(stride) * height fits in vgamem_size and that stride is 4-byte
> aligned, but never checks that abs(stride) is large enough to hold one
> row of pixels for the declared width and format.
>
> A malicious guest can create a primary surface with a stride much
> smaller than width * bytes_per_pixel (e.g. stride=3D4 for a 64-wide 32bpp
> surface). The spice server rejects this via red_validate_surface(), but
> the return is void and QEMU unconditionally proceeds to set up the local
> rendering state. On the next display refresh, VNC or SDL reads width *
> bytes_pp per scanline from a region backed by only stride bytes per
> row, causing a host-side out-of-bounds read.
>
> Add three checks in qxl_create_guest_primary() before creating the
> surface:
>  - reject unknown surface formats
>  - reject zero width or height
>  - reject surfaces where abs(stride) < width * bytes_per_pixel
>
> Also fix three related issues in qxl-render.c:
>  - qxl_blit() used abs_stride to advance the dst pointer into the
>    DisplaySurface, but when stride is negative the DisplaySurface is a
>    packed buffer whose stride may be smaller. Use surface_stride()
>    instead.
>  - qxl_render_update_area_unlocked() uses guest_head0_width (set via
>    QXL_IO_MONITORS_CONFIG_ASYNC) without validating it against
>    abs_stride, bypassing the new validation. Clamp the effective width
>    to abs_stride / bytes_pp to prevent out-of-bounds access while
>    tolerating the normal transient where the monitor config arrives
>    before the primary surface is resized to match.
>  - Similarly, guest_head0_height bypasses qxl_create_guest_primary()
>    validation. Without clamping, abs_stride * height can overrun
>    vgamem_size, and the product can also overflow 32 bits (e.g.
>    abs_stride=3D16 MiB, height=3D256 wraps to zero), defeating the
>    qxl_phys2virt() bounds check. Clamp height to
>    vgamem_size / abs_stride to prevent both.
>
> Fixes: CVE-2026-16271
> Fixes: 3761abb16784 ("hw/display/qxl: fix signed to unsigned comparison")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3637
> Reported-by: huntr bubble
> Signed-off-by: Marc-Andre Lureau <[email protected]>
> ---
> v2:
>  - also clamp height
>  - factor out qxl_format_bpp() helper

ping

> ---
>  hw/display/qxl.h        |  2 ++
>  hw/display/qxl-render.c | 46 ++++++++++++++++----------------
>  hw/display/qxl.c        | 59 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 84 insertions(+), 23 deletions(-)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index 48d664f77736..6f5b96fe86cc 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -181,6 +181,8 @@ void qxl_spice_oom(PCIQXLDevice *qxl);
>  void qxl_spice_reset_memslots(PCIQXLDevice *qxl);
>  void qxl_spice_reset_image_cache(PCIQXLDevice *qxl);
>  void qxl_spice_reset_cursor(PCIQXLDevice *qxl);
> +bool qxl_format_bpp(PCIQXLDevice *qxl, SpiceSurfaceFmt format,
> +                    uint32_t *bytes_pp, uint32_t *bits_pp);
>
>  /* qxl-logger.c */
>  int qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_i=
d);
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index 4799c9e8befd..b0a71a95ad66 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -27,6 +27,7 @@
>  static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
>  {
>      DisplaySurface *surface =3D qemu_console_surface(qxl->vga.con);
> +    int dst_stride =3D surface_stride(surface);
>      uint8_t *dst =3D surface_data(surface);
>      uint8_t *src;
>      int len, i;
> @@ -45,14 +46,14 @@ static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect=
)
>      } else {
>          src +=3D rect->top * qxl->guest_primary.abs_stride;
>      }
> -    dst +=3D rect->top  * qxl->guest_primary.abs_stride;
> +    dst +=3D rect->top  * dst_stride;
>      src +=3D rect->left * qxl->guest_primary.bytes_pp;
>      dst +=3D rect->left * qxl->guest_primary.bytes_pp;
>      len  =3D (rect->right - rect->left) * qxl->guest_primary.bytes_pp;
>
>      for (i =3D rect->top; i < rect->bottom; i++) {
>          memcpy(dst, src, len);
> -        dst +=3D qxl->guest_primary.abs_stride;
> +        dst +=3D dst_stride;
>          src +=3D qxl->guest_primary.qxl_stride;
>      }
>  }
> @@ -64,27 +65,9 @@ void qxl_render_resize(PCIQXLDevice *qxl)
>      qxl->guest_primary.qxl_stride =3D sc->stride;
>      qxl->guest_primary.abs_stride =3D abs(sc->stride);
>      qxl->guest_primary.resized++;
> -    switch (sc->format) {
> -    case SPICE_SURFACE_FMT_16_555:
> -        qxl->guest_primary.bytes_pp =3D 2;
> -        qxl->guest_primary.bits_pp =3D 15;
> -        break;
> -    case SPICE_SURFACE_FMT_16_565:
> -        qxl->guest_primary.bytes_pp =3D 2;
> -        qxl->guest_primary.bits_pp =3D 16;
> -        break;
> -    case SPICE_SURFACE_FMT_32_xRGB:
> -    case SPICE_SURFACE_FMT_32_ARGB:
> -        qxl->guest_primary.bytes_pp =3D 4;
> -        qxl->guest_primary.bits_pp =3D 32;
> -        break;
> -    default:
> -        fprintf(stderr, "%s: unhandled format: %x\n", __func__,
> -                qxl->guest_primary.surface.format);
> -        qxl->guest_primary.bytes_pp =3D 4;
> -        qxl->guest_primary.bits_pp =3D 32;
> -        break;
> -    }
> +    /* fallback to default bpp if format is unknown */
> +    qxl_format_bpp(qxl, sc->format, &qxl->guest_primary.bytes_pp,
> +                   &qxl->guest_primary.bits_pp);
>  }
>
>  static void qxl_set_rect_to_surface(PCIQXLDevice *qxl, QXLRect *area)
> @@ -103,6 +86,23 @@ static void qxl_render_update_area_unlocked(PCIQXLDev=
ice *qxl)
>      int height =3D qxl->guest_head0_height ?: qxl->guest_primary.surface=
.height;
>      int i;
>
> +    if (width <=3D 0 || height <=3D 0) {
> +        qxl_set_guest_bug(qxl, "%s: invalid dimension %dx%d",
> +                          __func__, width, height);
> +        goto end;
> +    }
> +
> +    if (qxl->guest_primary.bytes_pp > 0) {
> +        int max_width =3D qxl->guest_primary.abs_stride
> +                        / qxl->guest_primary.bytes_pp;
> +        width =3D MIN(width, max_width);
> +    }
> +
> +    if (qxl->guest_primary.abs_stride > 0) {
> +        int max_height =3D qxl->vgamem_size / qxl->guest_primary.abs_str=
ide;
> +        height =3D MIN(height, max_height);
> +    }
> +
>      if (qxl->guest_primary.resized) {
>          qxl->guest_primary.resized =3D 0;
>          qxl->guest_primary.data =3D qxl_phys2virt(qxl,
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index b7d871b9ee33..384b8767b8e6 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1489,6 +1489,47 @@ static void qxl_create_guest_primary_complete(PCIQ=
XLDevice *qxl)
>      qxl_render_resize(qxl);
>  }
>
> +/*
> + * Convert a SpiceSurfaceFormat to bytes per pixel and bits per pixel.
> + *
> + * Only valid for surface suitable for rendering.
> + */
> +bool qxl_format_bpp(PCIQXLDevice *qxl, SpiceSurfaceFmt format,
> +                    uint32_t *bytes_pp, uint32_t *bits_pp)
> +{
> +    uint32_t bypp =3D 4;
> +    uint32_t bipp =3D 32;
> +    bool ret =3D true;
> +
> +    switch (format) {
> +    case SPICE_SURFACE_FMT_16_555:
> +        bypp =3D 2;
> +        bipp =3D 15;
> +        break;
> +    case SPICE_SURFACE_FMT_16_565:
> +        bypp =3D 2;
> +        bipp =3D 16;
> +        break;
> +    case SPICE_SURFACE_FMT_32_xRGB:
> +    case SPICE_SURFACE_FMT_32_ARGB:
> +        bypp =3D 4;
> +        bipp =3D 32;
> +        break;
> +    default:
> +        ret =3D false;
> +        qxl_set_guest_bug(qxl, "%s: unhandled format: %x", __func__, for=
mat);
> +    }
> +
> +    if (bytes_pp !=3D NULL) {
> +        *bytes_pp =3D bypp;
> +    }
> +    if (bits_pp !=3D NULL) {
> +        *bits_pp =3D bipp;
> +    }
> +
> +    return ret;
> +}
> +
>  static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>                                       qxl_async_io async)
>  {
> @@ -1496,6 +1537,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *=
qxl, int loadvm,
>      QXLSurfaceCreate *sc =3D &qxl->guest_primary.surface;
>      uint32_t requested_height =3D le32_to_cpu(sc->height);
>      int requested_stride =3D le32_to_cpu(sc->stride);
> +    uint32_t bytes_pp;
>
>      if (requested_stride =3D=3D INT32_MIN ||
>          abs(requested_stride) * (uint64_t)requested_height
> @@ -1532,6 +1574,23 @@ static void qxl_create_guest_primary(PCIQXLDevice =
*qxl, int loadvm,
>          return;
>      }
>
> +    if (!qxl_format_bpp(qxl, surface.format, &bytes_pp, NULL)) {
> +        return;
> +    }
> +
> +    if (surface.width =3D=3D 0 || surface.height =3D=3D 0) {
> +        qxl_set_guest_bug(qxl, "%s: zero dimension %ux%u",
> +                          __func__, surface.width, surface.height);
> +        return;
> +    }
> +
> +    if ((uint64_t)surface.width * bytes_pp > abs(surface.stride)) {
> +        qxl_set_guest_bug(qxl, "%s: stride too small for width:"
> +                          " stride %d width %u bpp %u",
> +                          __func__, surface.stride, surface.width, bytes=
_pp);
> +        return;
> +    }
> +
>      surface.mouse_mode =3D true;
>      surface.group_id   =3D MEMSLOT_GROUP_GUEST;
>      if (loadvm) {
> --
> 2.55.0
>