Thanks for the patch! ... I've got a question below...
On 20/07/2026 04.48, [email protected] wrote:
From: Haotian Jiang <[email protected]>
qxl_realize_common() registers a vm_change_state handler via
qemu_add_vm_change_state_handler() and creates three bottom halves
(update_irq, update_area_bh, cursor_bh), but none are ever cleaned up.
The return value of qemu_add_vm_change_state_handler() is discarded, so
the handler is never removed from the global list, and there is no
PCIDeviceClass.exit callback to delete the BHs.
When a secondary QXL device (hotpluggable by default) is hot-unplugged
via device_del, the PCIQXLDevice memory is freed but the vm_state
handler and BH entries remain with dangling opaque pointers. On the
next VM state change (stop/cont/migrate) or BH dispatch, the callback
dereferences freed memory, causing a use-after-free.
Fix this by storing the VMChangeStateEntry returned by
qemu_add_vm_change_state_handler() and adding a qxl_exit() callback
that deletes the vm_state handler, all three BHs, and the
guest_surfaces.cmds allocation before the device memory is freed.
Fixes: a19cbfb34642 ("spice: add qxl device")
Fixes: CVE-2026-63322
Reported-by: Haotian Jiang of Tencent Security (Yunding Lab)
<[email protected]>
Signed-off-by: Haotian Jiang <[email protected]>
Cc: [email protected]
Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3607
---
hw/display/qxl.c | 15 ++++++++++++++-
hw/display/qxl.h | 1 +
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 74258afa58..c9323672a6 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2203,7 +2203,8 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error
**errp)
error_report_err(err);
}
- qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
+ qxl->vmstate_handler =
+ qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
qxl->update_irq = qemu_bh_new_guarded(qxl_update_irq_bh, qxl,
&DEVICE(qxl)->mem_reentrancy_guard);
@@ -2475,6 +2476,17 @@ static const Property qxl_properties[] = {
DEFINE_PROP_UINT32("yres", PCIQXLDevice, yres, 0),
};
+static void qxl_exit(PCIDevice *dev)
+{
+ PCIQXLDevice *qxl = PCI_QXL(dev);
+
+ g_clear_pointer(&qxl->vmstate_handler, qemu_del_vm_change_state_handler);
+ g_clear_pointer(&qxl->update_irq, qemu_bh_delete);
+ g_clear_pointer(&qxl->update_area_bh, qemu_bh_delete);
+ g_clear_pointer(&qxl->ssd.cursor_bh, qemu_bh_delete);
+ g_clear_pointer(&qxl->guest_surfaces.cmds, g_free);
+}
+
static void qxl_pci_class_init(ObjectClass *klass, const void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
@@ -2482,6 +2494,7 @@ static void qxl_pci_class_init(ObjectClass *klass, const
void *data)
k->vendor_id = REDHAT_PCI_VENDOR_ID;
k->device_id = QXL_DEVICE_ID_STABLE;
+ k->exit = qxl_exit;
The resources are allocated during the realize() function of the qxl device,
so not sure, but using the PCI->exit() function might be the wrong spot?
Could you put the clean-up into a unrealize() function instead, i.e. add
this via "k->unrealize = ..." in qxl_primary_class_init() and
qxl_secondary_class_init() ?
Also, there are some more resources allocated in the realize() function, e.g. :
qemu_mutex_init(&qxl->track_lock);
qemu_mutex_init(&qxl->async_lock);
...
qxl->guest_surfaces.cmds = g_new0(QXLPHYSICAL, qxl->ssd.num_surfaces);
Should these get deleted, too, to avoid leaking memory?
Thomas
