Hi On Mon, Jul 20, 2026 at 10:16 AM Thomas Huth <[email protected]> wrote: > > > 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? >
I am afraid trying to fix the leaks is going into a rabbit hole (at least I did). Probably it is best to queue this fix and handle the leaks post 11.1 imho @[email protected] Are you working on a new version?
