[PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[Andrey Drobyshev]

We've experienced crashes when spice-server aborts on migration target
when processing an invalid/stale guest address pointer.  While those are
hard to reproduce, it seems they can be fairly easily avoided by simply
skipping commands with invalid addresses in qxl_post_load.

Andrey Drobyshev (3):
  hw/display/qxl: factor out qxl_guest_phys2virt()
  hw/display/qxl: validate replayed commands in qxl_post_load
  hw/display/qxl: trace skipped stale loadvm commands

 hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
 hw/display/trace-events |   1 +
 2 files changed, 97 insertions(+), 36 deletions(-)

-- 
2.47.1

[PATCH 1/3] hw/display/qxl: factor out qxl_guest_phys2virt()

[Andrey Drobyshev]

Split the GROUP_GUEST half of qxl_phys2virt() into the helper
qxl_guest_phys2virt().  Also add a bool 'report_bug' param to the
qxl_get_check_slot_offset() called from it: when it's false, a failing
check just returns false without calling qxl_set_guest_bug().  All
existing callers pass true, so there's no functional change.  This
is in preparation for a quiet caller that validates guest addresses
which might be legitimately stale, when flagging a guest bug would be
wrong.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/display/qxl.c | 83 +++++++++++++++++++++++++++++-------------------
 1 file changed, 51 insertions(+), 32 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index b7d871b9ee3..be620b9c94d 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1407,7 +1407,7 @@ static void qxl_reset_surfaces(PCIQXLDevice *d)
 /* can be also called from spice server thread context */
 static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
                                       uint32_t *s, uint64_t *o,
-                                      size_t size_requested)
+                                      size_t size_requested, bool report_bug)
 {
     uint64_t phys   = le64_to_cpu(pqxl);
     uint32_t slot   = (phys >> (64 -  8)) & 0xff;
@@ -1415,42 +1415,55 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
     uint64_t size_available;
 
     if (slot >= NUM_MEMSLOTS) {
-        qxl_set_guest_bug(qxl, "slot too large %d >= %d", slot,
-                          NUM_MEMSLOTS);
+        if (report_bug) {
+            qxl_set_guest_bug(qxl, "slot too large %d >= %d", slot,
+                              NUM_MEMSLOTS);
+        }
         return false;
     }
     if (!qxl->guest_slots[slot].active) {
-        qxl_set_guest_bug(qxl, "inactive slot %d\n", slot);
+        if (report_bug) {
+            qxl_set_guest_bug(qxl, "inactive slot %d\n", slot);
+        }
         return false;
     }
     if (offset < qxl->guest_slots[slot].delta) {
-        qxl_set_guest_bug(qxl,
-                          "slot %d offset %"PRIu64" < delta %"PRIu64"\n",
-                          slot, offset, qxl->guest_slots[slot].delta);
+        if (report_bug) {
+            qxl_set_guest_bug(qxl,
+                              "slot %d offset %"PRIu64" < delta %"PRIu64"\n",
+                              slot, offset, qxl->guest_slots[slot].delta);
+        }
         return false;
     }
     offset -= qxl->guest_slots[slot].delta;
     if (offset > qxl->guest_slots[slot].size) {
-        qxl_set_guest_bug(qxl,
-                          "slot %d offset %"PRIu64" > size %"PRIu64"\n",
-                          slot, offset, qxl->guest_slots[slot].size);
+        if (report_bug) {
+            qxl_set_guest_bug(qxl,
+                              "slot %d offset %"PRIu64" > size %"PRIu64"\n",
+                              slot, offset, qxl->guest_slots[slot].size);
+        }
         return false;
     }
     size_available = memory_region_size(qxl->guest_slots[slot].mr);
     if (qxl->guest_slots[slot].offset + offset >= size_available) {
-        qxl_set_guest_bug(qxl,
-                          "slot %d offset %"PRIu64" > region size %"PRIu64"\n",
-                          slot, qxl->guest_slots[slot].offset + offset,
-                          size_available);
+        if (report_bug) {
+            qxl_set_guest_bug(qxl,
+                              "slot %d offset %"PRIu64" > region size %"PRIu64
+                              "\n", slot,
+                              qxl->guest_slots[slot].offset + offset,
+                              size_available);
+        }
         return false;
     }
     size_available -= qxl->guest_slots[slot].offset + offset;
     if (size_requested > size_available) {
-        qxl_set_guest_bug(qxl,
-                          "slot %d offset %"PRIu64" size %zu: "
-                          "overrun by %"PRIu64" bytes\n",
-                          slot, offset, size_requested,
-                          size_requested - size_available);
+        if (report_bug) {
+            qxl_set_guest_bug(qxl,
+                              "slot %d offset %"PRIu64" size %zu: "
+                              "overrun by %"PRIu64" bytes\n",
+                              slot, offset, size_requested,
+                              size_requested - size_available);
+        }
         return false;
     }
 
@@ -1460,25 +1473,31 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
 }
 
 /* can be also called from spice server thread context */
-void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
-                    size_t size)
+static void *qxl_guest_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
+                                 size_t size, bool report_bug)
 {
     uint64_t offset;
     uint32_t slot;
-    void *ptr;
+    uint8_t *ptr;
 
+    if (!qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset, size,
+                                   report_bug)) {
+        return NULL;
+    }
+    ptr  = memory_region_get_ram_ptr(qxl->guest_slots[slot].mr);
+    ptr += qxl->guest_slots[slot].offset;
+    ptr += offset;
+    return ptr;
+}
+
+void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
+                    size_t size)
+{
     switch (group_id) {
     case MEMSLOT_GROUP_HOST:
-        offset = le64_to_cpu(pqxl) & 0xffffffffffff;
-        return (void *)(intptr_t)offset;
+        return (void *)(intptr_t)(le64_to_cpu(pqxl) & 0xffffffffffff);
     case MEMSLOT_GROUP_GUEST:
-        if (!qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset, size)) {
-            return NULL;
-        }
-        ptr = memory_region_get_ram_ptr(qxl->guest_slots[slot].mr);
-        ptr += qxl->guest_slots[slot].offset;
-        ptr += offset;
-        return ptr;
+        return qxl_guest_phys2virt(qxl, pqxl, size, true);
     }
     return NULL;
 }
@@ -1942,7 +1961,7 @@ static void qxl_dirty_one_surface(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
     bool rc;
 
     size = (uint64_t)height * abs(stride);
-    rc = qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset, size);
+    rc = qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset, size, true);
     assert(rc == true);
     trace_qxl_surfaces_dirty(qxl->id, offset, size);
     qxl_set_dirty(qxl->guest_slots[slot].mr,
-- 
2.47.1

[PATCH 2/3] hw/display/qxl: validate replayed commands in qxl_post_load

[Andrey Drobyshev]

On incoming migration qxl_post_load() replays the tracked cursor and
surface commands by handing their guest addresses straight to spice,
without revalidating them. Those addresses were checked when the guest
submitted them, but the guest may have freed or reused that memory
before migration, so qxl's tracked pointer can be stale. spice-server
then re-parses the command from that memory and, for a stale cursor,
reads a garbage shape pointer -- aborting the target in memslot_get_virt()
(again, spice-server function) and failing the migration.

Validate each replayed command with qxl_guest_phys2virt(report_bug=false)
before adding it to the replay list, and for a cursor also validate the
nested shape pointer. Commands that no longer resolve are skipped rather
than replayed. report_bug is false so a stale pointer is not mistaken for
a live guest error, which would needlessly disable a healthy guest's
display.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/display/qxl.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index be620b9c94d..b9b19849738 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2329,6 +2329,37 @@ static void qxl_create_memslots(PCIQXLDevice *d)
     }
 }
 
+/*
+ * Validate a command tracked for loadvm replay before handing its guest
+ * address to spice-server.
+ */
+static bool qxl_loadvm_cmd_valid(PCIQXLDevice *d, QXLPHYSICAL data,
+                                 uint32_t type)
+{
+    switch (type) {
+    case QXL_CMD_SURFACE:
+        return qxl_guest_phys2virt(d, data,
+                                   sizeof(QXLSurfaceCmd), false) != NULL;
+
+    case QXL_CMD_CURSOR: {
+        QXLCursorCmd *cmd = qxl_guest_phys2virt(d, data, sizeof(QXLCursorCmd),
+                                                false);
+
+        if (!cmd) {
+            return false;
+        }
+        if (le32_to_cpu(cmd->type) == QXL_CURSOR_SET) {
+            return qxl_guest_phys2virt(d, le64_to_cpu(cmd->u.set.shape),
+                                       sizeof(QXLCursor), false) != NULL;
+        }
+        return true;
+    }
+
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static int qxl_post_load(void *opaque, int version)
 {
     PCIQXLDevice* d = opaque;
@@ -2367,12 +2398,17 @@ static int qxl_post_load(void *opaque, int version)
             if (d->guest_surfaces.cmds[in] == 0) {
                 continue;
             }
+            if (!qxl_loadvm_cmd_valid(d, d->guest_surfaces.cmds[in],
+                                      QXL_CMD_SURFACE)) {
+                continue;
+            }
             cmds[out].cmd.data = d->guest_surfaces.cmds[in];
             cmds[out].cmd.type = QXL_CMD_SURFACE;
             cmds[out].group_id = MEMSLOT_GROUP_GUEST;
             out++;
         }
-        if (d->guest_cursor) {
+        if (d->guest_cursor &&
+            qxl_loadvm_cmd_valid(d, d->guest_cursor, QXL_CMD_CURSOR)) {
             cmds[out].cmd.data = d->guest_cursor;
             cmds[out].cmd.type = QXL_CMD_CURSOR;
             cmds[out].group_id = MEMSLOT_GROUP_GUEST;
-- 
2.47.1

[PATCH 3/3] hw/display/qxl: trace skipped stale loadvm commands

[Andrey Drobyshev]

Emit a trace event when qxl_post_load() drops a cursor or surface
command whose guest address no longer resolves, so a migration that
lands on a stale tracked pointer is visible instead of silent.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 hw/display/qxl.c        | 17 +++++++++++------
 hw/display/trace-events |  1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index b9b19849738..9e7bc5c668f 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2400,6 +2400,8 @@ static int qxl_post_load(void *opaque, int version)
             }
             if (!qxl_loadvm_cmd_valid(d, d->guest_surfaces.cmds[in],
                                       QXL_CMD_SURFACE)) {
+                trace_qxl_post_load_stale_cmd(d->id, "surface",
+                                              d->guest_surfaces.cmds[in]);
                 continue;
             }
             cmds[out].cmd.data = d->guest_surfaces.cmds[in];
@@ -2407,12 +2409,15 @@ static int qxl_post_load(void *opaque, int version)
             cmds[out].group_id = MEMSLOT_GROUP_GUEST;
             out++;
         }
-        if (d->guest_cursor &&
-            qxl_loadvm_cmd_valid(d, d->guest_cursor, QXL_CMD_CURSOR)) {
-            cmds[out].cmd.data = d->guest_cursor;
-            cmds[out].cmd.type = QXL_CMD_CURSOR;
-            cmds[out].group_id = MEMSLOT_GROUP_GUEST;
-            out++;
+        if (d->guest_cursor) {
+            if (qxl_loadvm_cmd_valid(d, d->guest_cursor, QXL_CMD_CURSOR)) {
+                cmds[out].cmd.data = d->guest_cursor;
+                cmds[out].cmd.type = QXL_CMD_CURSOR;
+                cmds[out].group_id = MEMSLOT_GROUP_GUEST;
+                out++;
+            } else {
+                trace_qxl_post_load_stale_cmd(d->id, "cursor", d->guest_cursor);
+            }
         }
         qxl_spice_loadvm_commands(d, cmds, out);
         g_free(cmds);
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 4bfc457fbac..c5e7e42af23 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -82,6 +82,7 @@ qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *des
 qxl_io_write(int qid, const char *mode, uint64_t addr, const char *aname, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " (%s) val=%"PRIu64" size=%u async=%d"
 qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
 qxl_post_load(int qid, const char *mode) "%d %s"
+qxl_post_load_stale_cmd(int qid, const char *kind, uint64_t data) "%d skip stale %s cmd 0x%"PRIx64
 qxl_pre_load(int qid) "%d"
 qxl_pre_save(int qid) "%d"
 qxl_reset_surfaces(int qid) "%d"
-- 
2.47.1

Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[Marc-André Lureau]

Hi Andrey

On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
<andrey.drobyshev@virtuozzo.com> wrote:
>
> We've experienced crashes when spice-server aborts on migration target
> when processing an invalid/stale guest address pointer.  While those are
> hard to reproduce, it seems they can be fairly easily avoided by simply
> skipping commands with invalid addresses in qxl_post_load.

I would rather try to find the root cause, rather than applying a
post-load workaround. It may be related to this issue
https://gitlab.com/qemu-project/qemu/-/work_items/3766 (confidential):
there is a lack of resource tracking, when a guest release
slot/cursors etc, the track commands may not be correctly updated - or
preventing resource release. Unfortunately, spice/qxl is no longer
maintained - we should clearly mark this device as unsecure imho.

> Andrey Drobyshev (3):
>   hw/display/qxl: factor out qxl_guest_phys2virt()
>   hw/display/qxl: validate replayed commands in qxl_post_load
>   hw/display/qxl: trace skipped stale loadvm commands
>
>  hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>  hw/display/trace-events |   1 +
>  2 files changed, 97 insertions(+), 36 deletions(-)
>
> --
> 2.47.1
>
>

Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[Andrey Drobyshev]

On 8/26/26 11:10 AM, Marc-André Lureau wrote:
> Hi Andrey
> 
> On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com> wrote:
>>
>> We've experienced crashes when spice-server aborts on migration target
>> when processing an invalid/stale guest address pointer.  While those are
>> hard to reproduce, it seems they can be fairly easily avoided by simply
>> skipping commands with invalid addresses in qxl_post_load.
> 
> I would rather try to find the root cause, rather than applying a
> post-load workaround. It may be related to this issue
> https://gitlab.com/qemu-project/qemu/-/work_items/3766 (confidential):
> there is a lack of resource tracking, when a guest release
> slot/cursors etc, the track commands may not be correctly updated - or
> preventing resource release. Unfortunately, spice/qxl is no longer
> maintained - we should clearly mark this device as unsecure imho.
> 
>> [...]
I agree on all points. AFAIU this is a use-after-free of a tracked guest
pointer.  I'm not sure whether we can reliably track whenever guest
frees a buffer, so we drop it from the cmd list.  Still, I'd argue that
my 2nd patch here adds some hardening:

1. Currently we consume guest-controlled bytes which reach
memslot_get_virt() in spice-server, which aborts on the migration
target.  So there's a potential for guest->host DoS.
2. Dropping invalid commands is cheap, as we just don't replay cursor
movements on the target - instead wait until guest sets it again.  IMHO
this kind of hardening would make sense even if we did remedy the root
cause.

Andrey

Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[Denis V. Lunev]

On 8/26/26 10:10, Marc-André Lureau wrote:
> Hi Andrey
>
> On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com> wrote:
>> We've experienced crashes when spice-server aborts on migration target
>> when processing an invalid/stale guest address pointer.  While those are
>> hard to reproduce, it seems they can be fairly easily avoided by simply
>> skipping commands with invalid addresses in qxl_post_load.
> I would rather try to find the root cause, rather than applying a
> post-load workaround. It may be related to this issue
> https://gitlab.com/qemu-project/qemu/-/work_items/3766 (confidential):
> there is a lack of resource tracking, when a guest release
> slot/cursors etc, the track commands may not be correctly updated - or
> preventing resource release. Unfortunately, spice/qxl is no longer
> maintained - we should clearly mark this device as unsecure imho.

Unfortunately we do not have access to this item in gitlab.
Can you grant it at least to den@openvz.org?

Den

Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[Marc-André Lureau]

On Tue, Aug 25, 2026 at 9:26 PM Andrey Drobyshev
<andrey.drobyshev@virtuozzo.com> wrote:
>
> We've experienced crashes when spice-server aborts on migration target
> when processing an invalid/stale guest address pointer.  While those are
> hard to reproduce, it seems they can be fairly easily avoided by simply
> skipping commands with invalid addresses in qxl_post_load.
>
> Andrey Drobyshev (3):
>   hw/display/qxl: factor out qxl_guest_phys2virt()
>   hw/display/qxl: validate replayed commands in qxl_post_load
>   hw/display/qxl: trace skipped stale loadvm commands
>
>  hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>  hw/display/trace-events |   1 +
>  2 files changed, 97 insertions(+), 36 deletions(-)

series lgtm:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>
> --
> 2.47.1
>
>

Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[Michael Tokarev]

On 8/25/26 20:20, Andrey Drobyshev wrote:
> We've experienced crashes when spice-server aborts on migration target
> when processing an invalid/stale guest address pointer.  While those are
> hard to reproduce, it seems they can be fairly easily avoided by simply
> skipping commands with invalid addresses in qxl_post_load.
> 
> Andrey Drobyshev (3):
>    hw/display/qxl: factor out qxl_guest_phys2virt()
>    hw/display/qxl: validate replayed commands in qxl_post_load
>    hw/display/qxl: trace skipped stale loadvm commands
> 
>   hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>   hw/display/trace-events |   1 +
>   2 files changed, 97 insertions(+), 36 deletions(-)
Hi!

Is this a qemu-stable material?  Or there's no need to bother?

Thanks,

/mjt

Re: [PATCH 0/3] hw/display/qxl: Avoid crashing on stale guest addresses in qxl_post_load

[Andrey Drobyshev]

On 9/14/26 4:57 PM, Michael Tokarev wrote:
> On 8/25/26 20:20, Andrey Drobyshev wrote:
>> We've experienced crashes when spice-server aborts on migration target
>> when processing an invalid/stale guest address pointer.  While those are
>> hard to reproduce, it seems they can be fairly easily avoided by simply
>> skipping commands with invalid addresses in qxl_post_load.
>>
>> Andrey Drobyshev (3):
>>    hw/display/qxl: factor out qxl_guest_phys2virt()
>>    hw/display/qxl: validate replayed commands in qxl_post_load
>>    hw/display/qxl: trace skipped stale loadvm commands
>>
>>   hw/display/qxl.c        | 132 +++++++++++++++++++++++++++++-----------
>>   hw/display/trace-events |   1 +
>>   2 files changed, 97 insertions(+), 36 deletions(-)
> Hi!
> 
> Is this a qemu-stable material?  Or there's no need to bother?
> 
> Thanks,
> 
> /mjt

IMHO it's not necessarily a qemu-stable material as the crash itself is
hard to reproduce and I haven't seen other people complaining about it.
Still, feel free to pull it.

Andrey