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

marcandre.lureau@redhat.com posted 1 patch 4 days, 23 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260720114513.2500370-1-marcandre.lureau@redhat.com
There is a newer version of this series
hw/display/qxl.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
[PATCH] hw/display/qxl: validate primary surface stride against width
Posted by marcandre.lureau@redhat.com 4 days, 23 hours ago
From: Marc-André Lureau <marcandre.lureau@redhat.com>

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=4 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 *
byptes_pp per scanline from a region backed by only stride bytes per
row, causing a host-side out-of-bounds read.

Add three checks before creating the surface:
 - reject unknown surface formats
 - reject zero width or height
 - reject surfaces where abs(stride) < width * bytes_per_pixel

Fixes: CVE-2026-16271
Fixes: 3761abb16784 ("hw/display/qxl: fix signed to unsigned comparison")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3637
Signed-off-by: Marc-Andre Lureau <marcandre.lureau@redhat.com>
---
 hw/display/qxl.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 4a56f07b157..36fc681cbfb 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1524,6 +1524,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
     QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
     uint32_t requested_height = le32_to_cpu(sc->height);
     int requested_stride = le32_to_cpu(sc->stride);
+    uint32_t bytes_pp;
 
     if (requested_stride == INT32_MIN ||
         abs(requested_stride) * (uint64_t)requested_height
@@ -1560,6 +1561,34 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
         return;
     }
 
+    switch (surface.format) {
+    case SPICE_SURFACE_FMT_16_555:
+    case SPICE_SURFACE_FMT_16_565:
+        bytes_pp = 2;
+        break;
+    case SPICE_SURFACE_FMT_32_xRGB:
+    case SPICE_SURFACE_FMT_32_ARGB:
+        bytes_pp = 4;
+        break;
+    default:
+        qxl_set_guest_bug(qxl, "%s: unhandled format %d",
+                          __func__, surface.format);
+        return;
+    }
+
+    if (surface.width == 0 || surface.height == 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 = true;
     surface.group_id   = MEMSLOT_GROUP_GUEST;
     if (loadvm) {
-- 
2.55.0


Re: [PATCH] hw/display/qxl: validate primary surface stride against width
Posted by Akihiko Odaki 1 day, 22 hours ago
On 2026/07/20 20:45, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 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=4 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 *
> byptes_pp per scanline from a region backed by only stride bytes per
> row, causing a host-side out-of-bounds read.
> 
> Add three checks before creating the surface:
>   - reject unknown surface formats
>   - reject zero width or height
>   - reject surfaces where abs(stride) < width * bytes_per_pixel
> 
> Fixes: CVE-2026-16271
> Fixes: 3761abb16784 ("hw/display/qxl: fix signed to unsigned comparison")
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3637
> Signed-off-by: Marc-Andre Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/display/qxl.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 4a56f07b157..36fc681cbfb 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1524,6 +1524,7 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>       QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
>       uint32_t requested_height = le32_to_cpu(sc->height);
>       int requested_stride = le32_to_cpu(sc->stride);
> +    uint32_t bytes_pp;
>   
>       if (requested_stride == INT32_MIN ||
>           abs(requested_stride) * (uint64_t)requested_height
> @@ -1560,6 +1561,34 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,
>           return;
>       }
>   
> +    switch (surface.format) {
> +    case SPICE_SURFACE_FMT_16_555:
> +    case SPICE_SURFACE_FMT_16_565:
> +        bytes_pp = 2;
> +        break;
> +    case SPICE_SURFACE_FMT_32_xRGB:
> +    case SPICE_SURFACE_FMT_32_ARGB:
> +        bytes_pp = 4;
> +        break;
> +    default:
> +        qxl_set_guest_bug(qxl, "%s: unhandled format %d",
> +                          __func__, surface.format);
> +        return;
> +    }
> +
> +    if (surface.width == 0 || surface.height == 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;
> +    }
> +

It looks like this check doesn't fully align with how 
qxl_render_update_area_unlocked() uses these values to construct the 
DisplaySurface.

First, qxl_render_update_area_unlocked() ignores a negative stride and 
constructs a packed DisplaySurface for it. However, qxl_blit() still 
uses the absolute stride to advance pointers inside that DisplaySurface. 
As a result, qxl_blit() will perform an out-of-bounds write to the 
DisplaySurface if a padded negative stride is provided.

Second, when qxl->guest_head0_width is set, 
qxl_render_update_area_unlocked() uses it instead of 
qxl->guest_primary.surface.width. At the same time, it relies on 
qxl->guest_primary.abs_stride, effectively bypassing your new check. VNC 
or SDL then reads this incorrectly constructed DisplaySurface, which 
still results in the host-side out-of-bounds read.

Regards,
Akihiko Odaki

>       surface.mouse_mode = true;
>       surface.group_id   = MEMSLOT_GROUP_GUEST;
>       if (loadvm) {