From: Marc-André 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=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 *
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=16 MiB, height=256 wraps to zero), defeating the
qxl_phys2virt() bounds check. Clamp height to
vgamem_size / abs_stride to prevent both.
While touch it, fix some endianness issues.
Fixes: CVE-2026-16271
Fixes: a19cbfb34642 ("spice: add qxl device")
Fixes: 979f7ef8966b ("qxl: use guest_monitor_config for local renderer.")
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3637
Reported-by: huntr bubble
Signed-off-by: Marc-Andre Lureau <[email protected]>
---
v5:
- fix negative stride mapping in qxl_render_update_area_unlocked()
- fix fields endianness in qxl_render_resize()
---
hw/display/qxl.h | 2 ++
hw/display/qxl-render.c | 66 +++++++++++++++++++++++++----------------
hw/display/qxl.c | 59 ++++++++++++++++++++++++++++++++++++
3 files changed, 101 insertions(+), 26 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_id);
diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index 4799c9e8befd..348ddba76890 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 = qemu_console_surface(qxl->vga.con);
+ int dst_stride = surface_stride(surface);
uint8_t *dst = surface_data(surface);
uint8_t *src;
int len, i;
@@ -45,14 +46,14 @@ static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
} else {
src += rect->top * qxl->guest_primary.abs_stride;
}
- dst += rect->top * qxl->guest_primary.abs_stride;
+ dst += rect->top * dst_stride;
src += rect->left * qxl->guest_primary.bytes_pp;
dst += rect->left * qxl->guest_primary.bytes_pp;
len = (rect->right - rect->left) * qxl->guest_primary.bytes_pp;
for (i = rect->top; i < rect->bottom; i++) {
memcpy(dst, src, len);
- dst += qxl->guest_primary.abs_stride;
+ dst += dst_stride;
src += qxl->guest_primary.qxl_stride;
}
}
@@ -61,30 +62,13 @@ void qxl_render_resize(PCIQXLDevice *qxl)
{
QXLSurfaceCreate *sc = &qxl->guest_primary.surface;
- qxl->guest_primary.qxl_stride = sc->stride;
- qxl->guest_primary.abs_stride = abs(sc->stride);
+ qxl->guest_primary.qxl_stride = le32_to_cpu(sc->stride);
+ qxl->guest_primary.abs_stride = abs(qxl->guest_primary.qxl_stride);
qxl->guest_primary.resized++;
- switch (sc->format) {
- case SPICE_SURFACE_FMT_16_555:
- qxl->guest_primary.bytes_pp = 2;
- qxl->guest_primary.bits_pp = 15;
- break;
- case SPICE_SURFACE_FMT_16_565:
- qxl->guest_primary.bytes_pp = 2;
- qxl->guest_primary.bits_pp = 16;
- break;
- case SPICE_SURFACE_FMT_32_xRGB:
- case SPICE_SURFACE_FMT_32_ARGB:
- qxl->guest_primary.bytes_pp = 4;
- qxl->guest_primary.bits_pp = 32;
- break;
- default:
- fprintf(stderr, "%s: unhandled format: %x\n", __func__,
- qxl->guest_primary.surface.format);
- qxl->guest_primary.bytes_pp = 4;
- qxl->guest_primary.bits_pp = 32;
- break;
- }
+ /* fallback to default bpp if format is unknown */
+ qxl_format_bpp(qxl, le32_to_cpu(sc->format),
+ &qxl->guest_primary.bytes_pp,
+ &qxl->guest_primary.bits_pp);
}
static void qxl_set_rect_to_surface(PCIQXLDevice *qxl, QXLRect *area)
@@ -101,15 +85,45 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
DisplaySurface *surface;
int width = qxl->guest_head0_width ?: qxl->guest_primary.surface.width;
int height = qxl->guest_head0_height ?: qxl->guest_primary.surface.height;
+ uint64_t map_height;
int i;
+ if (width <= 0 || height <= 0) {
+ goto end;
+ }
+
+ if (qxl->guest_primary.bytes_pp > 0) {
+ int max_width = qxl->guest_primary.abs_stride
+ / qxl->guest_primary.bytes_pp;
+ width = MIN(width, max_width);
+ }
+
+ if (qxl->guest_primary.qxl_stride < 0) {
+ /* qxl_blit() uses the primary height to find the first scanline. */
+ height = MIN(height, (int)qxl->guest_primary.surface.height);
+ }
+
+ if (qxl->guest_primary.abs_stride > 0) {
+ int max_height = qxl->vgamem_size / qxl->guest_primary.abs_stride;
+ height = MIN(height, max_height);
+ }
+
+ /*
+ * height limits the visible update, while map_height is the guest memory
+ * span validated by qxl_phys2virt(). With a negative stride qxl_blit()
+ * addresses scanlines from the declared primary height, so a shorter
+ * monitor still requires validating the full primary surface.
+ */
+ map_height = qxl->guest_primary.qxl_stride < 0 ?
+ qxl->guest_primary.surface.height : height;
+
if (qxl->guest_primary.resized) {
qxl->guest_primary.resized = 0;
qxl->guest_primary.data = qxl_phys2virt(qxl,
qxl->guest_primary.surface.mem,
MEMSLOT_GROUP_GUEST,
qxl->guest_primary.abs_stride
- * height);
+ * map_height);
if (!qxl->guest_primary.data) {
goto end;
}
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(PCIQXLDevice *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 = 4;
+ uint32_t bipp = 32;
+ bool ret = true;
+
+ switch (format) {
+ case SPICE_SURFACE_FMT_16_555:
+ bypp = 2;
+ bipp = 15;
+ break;
+ case SPICE_SURFACE_FMT_16_565:
+ bypp = 2;
+ bipp = 16;
+ break;
+ case SPICE_SURFACE_FMT_32_xRGB:
+ case SPICE_SURFACE_FMT_32_ARGB:
+ bypp = 4;
+ bipp = 32;
+ break;
+ default:
+ ret = false;
+ qxl_set_guest_bug(qxl, "%s: unhandled format: %x", __func__, format);
+ }
+
+ if (bytes_pp != NULL) {
+ *bytes_pp = bypp;
+ }
+ if (bits_pp != NULL) {
+ *bits_pp = 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 = &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
@@ -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 == 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.