[PATCH 0/2] ui/cursor: fix two races that free a cursor early

Denis V. Lunev posted 2 patches 3 weeks, 2 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260903192647.2677279-1-den@openvz.org
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>
hw/display/qxl.c     |  2 ++
include/ui/console.h |  9 +++++++++
ui/cursor.c          | 17 +++++++++++------
3 files changed, 22 insertions(+), 6 deletions(-)
[PATCH 0/2] ui/cursor: fix two races that free a cursor early
Posted by Denis V. Lunev 3 weeks, 2 days ago
A guest with a qxl display can make QEMU drop more references to a
QEMUCursor than were taken. The cursor is freed while another owner
still points at it, and that owner's later cursor_unref() decrements
four bytes of a chunk the allocator has handed out again. Nothing
aborts and nothing is logged; QEMU dies later in an unrelated
allocation, in another thread.

Two defects get there, and neither fix is sufficient alone:

 - qxl_spice_reset_cursor() replaces qxl->ssd.cursor with no lock held,
   while every other writer of that field takes ssd.lock. It runs on a
   vCPU thread from QXL_IO_DESTROY_PRIMARY and, unlike qxl_hard_reset(),
   leaves the SPICE display worker running.

 - QEMUCursor.refcount is a plain int, taken and dropped from the main
   loop, the SPICE worker, ui/cocoa.m and ui/dbus-listener.c, with no
   lock common to all of them, so an increment can be lost.

A qxl device starts a spice-server instance for local rendering even
with no -spice, so this is not limited to SPICE console setups.

Patch 2 also asserts that the refcount was positive. Only qxl was
exercised here, so if another display backend drops a reference it
never took, that assert turns a silent leak into an abort.

Reproducer: a libdrm program in the guest queues cursor SET commands,
then disables the CRTC so the driver issues QXL_IO_DESTROY_PRIMARY.
Unpatched QEMU dies within seconds; with the series it does not. Happy
to post it.

This is independent of the pending "ui/console: fix use-after-free in
qemu_console_set_cursor", which fixes a self-assignment ordering bug at
the same line. With that applied the crash here is unchanged.

Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>

Denis V. Lunev (2):
  hw/display/qxl: hold ssd.lock while replacing ssd.cursor
  ui/cursor: make the cursor refcount atomic

 hw/display/qxl.c     |  2 ++
 include/ui/console.h |  9 +++++++++
 ui/cursor.c          | 17 +++++++++++------
 3 files changed, 22 insertions(+), 6 deletions(-)

-- 
2.53.0


Re: [PATCH 0/2] ui/cursor: fix two races that free a cursor early
Posted by Denis V. Lunev 1 week, 6 days ago
On 9/3/26 21:26, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> A guest with a qxl display can make QEMU drop more references to a
> QEMUCursor than were taken. The cursor is freed while another owner
> still points at it, and that owner's later cursor_unref() decrements
> four bytes of a chunk the allocator has handed out again. Nothing
> aborts and nothing is logged; QEMU dies later in an unrelated
> allocation, in another thread.
>
> Two defects get there, and neither fix is sufficient alone:
>
>  - qxl_spice_reset_cursor() replaces qxl->ssd.cursor with no lock held,
>    while every other writer of that field takes ssd.lock. It runs on a
>    vCPU thread from QXL_IO_DESTROY_PRIMARY and, unlike qxl_hard_reset(),
>    leaves the SPICE display worker running.
>
>  - QEMUCursor.refcount is a plain int, taken and dropped from the main
>    loop, the SPICE worker, ui/cocoa.m and ui/dbus-listener.c, with no
>    lock common to all of them, so an increment can be lost.
>
> A qxl device starts a spice-server instance for local rendering even
> with no -spice, so this is not limited to SPICE console setups.
>
> Patch 2 also asserts that the refcount was positive. Only qxl was
> exercised here, so if another display backend drops a reference it
> never took, that assert turns a silent leak into an abort.
>
> Reproducer: a libdrm program in the guest queues cursor SET commands,
> then disables the CRTC so the driver issues QXL_IO_DESTROY_PRIMARY.
> Unpatched QEMU dies within seconds; with the series it does not. Happy
> to post it.
>
> This is independent of the pending "ui/console: fix use-after-free in
> qemu_console_set_cursor", which fixes a self-assignment ordering bug at
> the same line. With that applied the crash here is unchanged.
>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
>
> Denis V. Lunev (2):
>   hw/display/qxl: hold ssd.lock while replacing ssd.cursor
>   ui/cursor: make the cursor refcount atomic
>
>  hw/display/qxl.c     |  2 ++
>  include/ui/console.h |  9 +++++++++
>  ui/cursor.c          | 17 +++++++++++------
>  3 files changed, 22 insertions(+), 6 deletions(-)
>
Hi, Marc-Andre!

You have not posted the notification that patches
have been queued. Are you going to accept them?
Unfortunately I see your review without any further
actions and patches were not landed yet.

Sorry for bothering,
    Den

Re: [PATCH 0/2] ui/cursor: fix two races that free a cursor early
Posted by Marc-André Lureau 1 week, 5 days ago
Hi Denis

On Sun, Sep 13, 2026 at 11:58 PM Denis V. Lunev <den@virtuozzo.com> wrote:
>
> On 9/3/26 21:26, Denis V. Lunev wrote:
> > This email originated from an IP that might not be authorized by the domain it was sent from.
> > Do not click links or open attachments unless it is an email you expected to receive.
> > A guest with a qxl display can make QEMU drop more references to a
> > QEMUCursor than were taken. The cursor is freed while another owner
> > still points at it, and that owner's later cursor_unref() decrements
> > four bytes of a chunk the allocator has handed out again. Nothing
> > aborts and nothing is logged; QEMU dies later in an unrelated
> > allocation, in another thread.
> >
> > Two defects get there, and neither fix is sufficient alone:
> >
> >  - qxl_spice_reset_cursor() replaces qxl->ssd.cursor with no lock held,
> >    while every other writer of that field takes ssd.lock. It runs on a
> >    vCPU thread from QXL_IO_DESTROY_PRIMARY and, unlike qxl_hard_reset(),
> >    leaves the SPICE display worker running.
> >
> >  - QEMUCursor.refcount is a plain int, taken and dropped from the main
> >    loop, the SPICE worker, ui/cocoa.m and ui/dbus-listener.c, with no
> >    lock common to all of them, so an increment can be lost.
> >
> > A qxl device starts a spice-server instance for local rendering even
> > with no -spice, so this is not limited to SPICE console setups.
> >
> > Patch 2 also asserts that the refcount was positive. Only qxl was
> > exercised here, so if another display backend drops a reference it
> > never took, that assert turns a silent leak into an abort.
> >
> > Reproducer: a libdrm program in the guest queues cursor SET commands,
> > then disables the CRTC so the driver issues QXL_IO_DESTROY_PRIMARY.
> > Unpatched QEMU dies within seconds; with the series it does not. Happy
> > to post it.
> >
> > This is independent of the pending "ui/console: fix use-after-free in
> > qemu_console_set_cursor", which fixes a self-assignment ordering bug at
> > the same line. With that applied the crash here is unchanged.
> >
> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> >
> > Denis V. Lunev (2):
> >   hw/display/qxl: hold ssd.lock while replacing ssd.cursor
> >   ui/cursor: make the cursor refcount atomic
> >
> >  hw/display/qxl.c     |  2 ++
> >  include/ui/console.h |  9 +++++++++
> >  ui/cursor.c          | 17 +++++++++++------
> >  3 files changed, 22 insertions(+), 6 deletions(-)
> >
> Hi, Marc-Andre!
>
> You have not posted the notification that patches
> have been queued. Are you going to accept them?
> Unfortunately I see your review without any further
> actions and patches were not landed yet.

They are merged upstream now.
thanks