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

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

[Denis V. Lunev]

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

[PATCH 1/2] hw/display/qxl: hold ssd.lock while replacing ssd.cursor

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

qxl_spice_reset_cursor() unrefs qxl->ssd.cursor and installs the hidden
cursor without holding qxl->ssd.lock. Every other writer of that field
takes it: qxl_render_cursor(), display_mouse_define() and
qemu_spice_cursor_refresh_bh().

The unlocked path runs on a vCPU thread, reached from ioport_write() on
QXL_IO_DESTROY_PRIMARY and QXL_IO_DESTROY_PRIMARY_ASYNC, and holds only
the BQL, which the SPICE display worker never takes. Unlike
qxl_hard_reset(), it leaves that worker running.
spice_qxl_reset_cursor() does round trip through the dispatcher, but the
worker is free again as soon as it returns, so it can enter
qxl_render_cursor() and unref the same QEMUCursor a few instructions
later. Both threads then drop one reference for what is a single
reference, freeing a cursor that another user still holds. The store to
ssd.cursor races the same way, and a guest that keeps this up also ends
up waiting forever in qxl_fence_wait().

A guest reaches this by switching QXL mode while it also updates the
pointer shape.

Fixes: 958c2bceba06 ("qxl: fix cursor reset")
Cc: qemu-stable@nongnu.org
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/display/qxl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 384b8767b8..c4f547e88b 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -294,10 +294,12 @@ void qxl_spice_reset_cursor(PCIQXLDevice *qxl)
     qemu_mutex_lock(&qxl->track_lock);
     qxl->guest_cursor = 0;
     qemu_mutex_unlock(&qxl->track_lock);
+    qemu_mutex_lock(&qxl->ssd.lock);
     if (qxl->ssd.cursor) {
         cursor_unref(qxl->ssd.cursor);
     }
     qxl->ssd.cursor = cursor_builtin_hidden();
+    qemu_mutex_unlock(&qxl->ssd.lock);
 }
 
 static uint32_t qxl_crc32(const uint8_t *p, unsigned len)
-- 
2.53.0

Re: [PATCH 1/2] hw/display/qxl: hold ssd.lock while replacing ssd.cursor

[Marc-André Lureau]

On Thu, Sep 3, 2026 at 11:26 PM Denis V. Lunev <den@openvz.org> wrote:
>
> From: Denis V. Lunev <den@openvz.org>
>
> qxl_spice_reset_cursor() unrefs qxl->ssd.cursor and installs the hidden
> cursor without holding qxl->ssd.lock. Every other writer of that field
> takes it: qxl_render_cursor(), display_mouse_define() and
> qemu_spice_cursor_refresh_bh().
>
> The unlocked path runs on a vCPU thread, reached from ioport_write() on
> QXL_IO_DESTROY_PRIMARY and QXL_IO_DESTROY_PRIMARY_ASYNC, and holds only
> the BQL, which the SPICE display worker never takes. Unlike
> qxl_hard_reset(), it leaves that worker running.
> spice_qxl_reset_cursor() does round trip through the dispatcher, but the
> worker is free again as soon as it returns, so it can enter
> qxl_render_cursor() and unref the same QEMUCursor a few instructions
> later. Both threads then drop one reference for what is a single
> reference, freeing a cursor that another user still holds. The store to
> ssd.cursor races the same way, and a guest that keeps this up also ends
> up waiting forever in qxl_fence_wait().
>
> A guest reaches this by switching QXL mode while it also updates the
> pointer shape.
>
> Fixes: 958c2bceba06 ("qxl: fix cursor reset")
> Cc: qemu-stable@nongnu.org
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>

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

thanks

> ---
>  hw/display/qxl.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 384b8767b8..c4f547e88b 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -294,10 +294,12 @@ void qxl_spice_reset_cursor(PCIQXLDevice *qxl)
>      qemu_mutex_lock(&qxl->track_lock);
>      qxl->guest_cursor = 0;
>      qemu_mutex_unlock(&qxl->track_lock);
> +    qemu_mutex_lock(&qxl->ssd.lock);
>      if (qxl->ssd.cursor) {
>          cursor_unref(qxl->ssd.cursor);
>      }
>      qxl->ssd.cursor = cursor_builtin_hidden();
> +    qemu_mutex_unlock(&qxl->ssd.lock);
>  }
>
>  static uint32_t qxl_crc32(const uint8_t *p, unsigned len)
> --
> 2.53.0
>

[PATCH 2/2] ui/cursor: make the cursor refcount atomic

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

A QEMUCursor outlives the call that publishes it and is shared between
threads, but its refcount was a plain int with no single lock covering
every user. qemu_console_set_cursor() takes and drops references from
the main loop under the BQL alone, hw/display/qxl-render.c does so from
the SPICE display worker thread, and ui/spice-display.c does so under
SimpleSpiceDisplay::lock. ui/cocoa.m and ui/dbus-listener.c add two more
threads.

The pair that collides is qemu_spice_cursor_refresh_bh(), which drops
ssd->lock before calling qemu_console_set_cursor(), and the worker
refcounting the same cursor under that lock. A lost increment frees the
cursor while the console still points at it, so the console's next unref
decrements memory the allocator has already handed out again. Locking
ssd.cursor is not enough on its own: with that done, this is the race
that remains.

Assert on the value the decrement observed while here. Dropping a
reference that was never taken used to be silent, because the decrement
lands in the allocator metadata of the freed chunk: nothing is logged,
the object is not freed twice, and the process runs on until some later
allocation walks the damaged free list and faults, arbitrarily far from
the code that caused it.

Fixes: 0b2824e5e48a ("spice: use bottom half instead of refresh timer for cursor updates")
Cc: qemu-stable@nongnu.org
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 include/ui/console.h |  9 +++++++++
 ui/cursor.c          | 17 +++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/ui/console.h b/include/ui/console.h
index 29bf722888..3634956949 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -126,6 +126,15 @@ typedef struct QEMUCursor {
 } QEMUCursor;
 
 QEMUCursor *cursor_alloc(uint16_t width, uint16_t height);
+
+/*
+ * A cursor may be shared between the main loop, a vCPU thread and a
+ * display backend's own thread, so the refcount is atomic and these two
+ * may be called from any of them.  The object itself is not otherwise
+ * thread-safe: take a reference before publishing the pointer anywhere
+ * another thread can reach it, and never dereference a cursor you do
+ * not hold a reference to.
+ */
 QEMUCursor *cursor_ref(QEMUCursor *c);
 void cursor_unref(QEMUCursor *c);
 QEMUCursor *cursor_builtin_hidden(void);
diff --git a/ui/cursor.c b/ui/cursor.c
index 6e23244fbe..69d27d49a1 100644
--- a/ui/cursor.c
+++ b/ui/cursor.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "qemu/atomic.h"
 #include "ui/console.h"
 
 #include "cursor_hidden.xpm"
@@ -103,24 +104,28 @@ QEMUCursor *cursor_alloc(uint16_t width, uint16_t height)
     c = g_malloc0(sizeof(QEMUCursor) + datasize);
     c->width  = width;
     c->height = height;
-    c->refcount = 1;
+    qatomic_set(&c->refcount, 1);
     return c;
 }
 
 QEMUCursor *cursor_ref(QEMUCursor *c)
 {
-    c->refcount++;
+    qatomic_inc(&c->refcount);
     return c;
 }
 
 void cursor_unref(QEMUCursor *c)
 {
+    int refcount;
+
     if (c == NULL)
         return;
-    c->refcount--;
-    if (c->refcount)
-        return;
-    g_free(c);
+
+    refcount = qatomic_fetch_dec(&c->refcount);
+    assert(refcount > 0);
+    if (refcount == 1) {
+        g_free(c);
+    }
 }
 
 int cursor_get_mono_bpl(QEMUCursor *c)
-- 
2.53.0

Re: [PATCH 2/2] ui/cursor: make the cursor refcount atomic

[Marc-André Lureau]

On Thu, Sep 3, 2026 at 11:27 PM Denis V. Lunev <den@openvz.org> wrote:
>
> From: Denis V. Lunev <den@openvz.org>
>
> A QEMUCursor outlives the call that publishes it and is shared between
> threads, but its refcount was a plain int with no single lock covering
> every user. qemu_console_set_cursor() takes and drops references from
> the main loop under the BQL alone, hw/display/qxl-render.c does so from
> the SPICE display worker thread, and ui/spice-display.c does so under
> SimpleSpiceDisplay::lock. ui/cocoa.m and ui/dbus-listener.c add two more
> threads.
>
> The pair that collides is qemu_spice_cursor_refresh_bh(), which drops
> ssd->lock before calling qemu_console_set_cursor(), and the worker
> refcounting the same cursor under that lock. A lost increment frees the
> cursor while the console still points at it, so the console's next unref
> decrements memory the allocator has already handed out again. Locking
> ssd.cursor is not enough on its own: with that done, this is the race
> that remains.
>
> Assert on the value the decrement observed while here. Dropping a
> reference that was never taken used to be silent, because the decrement
> lands in the allocator metadata of the freed chunk: nothing is logged,
> the object is not freed twice, and the process runs on until some later
> allocation walks the damaged free list and faults, arbitrarily far from
> the code that caused it.
>
> Fixes: 0b2824e5e48a ("spice: use bottom half instead of refresh timer for cursor updates")
> Cc: qemu-stable@nongnu.org
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>

I suspected that, never had the time or motivation to find the arguments:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

thanks

> ---
>  include/ui/console.h |  9 +++++++++
>  ui/cursor.c          | 17 +++++++++++------
>  2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 29bf722888..3634956949 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -126,6 +126,15 @@ typedef struct QEMUCursor {
>  } QEMUCursor;
>
>  QEMUCursor *cursor_alloc(uint16_t width, uint16_t height);
> +
> +/*
> + * A cursor may be shared between the main loop, a vCPU thread and a
> + * display backend's own thread, so the refcount is atomic and these two
> + * may be called from any of them.  The object itself is not otherwise
> + * thread-safe: take a reference before publishing the pointer anywhere
> + * another thread can reach it, and never dereference a cursor you do
> + * not hold a reference to.
> + */
>  QEMUCursor *cursor_ref(QEMUCursor *c);
>  void cursor_unref(QEMUCursor *c);
>  QEMUCursor *cursor_builtin_hidden(void);
> diff --git a/ui/cursor.c b/ui/cursor.c
> index 6e23244fbe..69d27d49a1 100644
> --- a/ui/cursor.c
> +++ b/ui/cursor.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "qemu/atomic.h"
>  #include "ui/console.h"
>
>  #include "cursor_hidden.xpm"
> @@ -103,24 +104,28 @@ QEMUCursor *cursor_alloc(uint16_t width, uint16_t height)
>      c = g_malloc0(sizeof(QEMUCursor) + datasize);
>      c->width  = width;
>      c->height = height;
> -    c->refcount = 1;
> +    qatomic_set(&c->refcount, 1);
>      return c;
>  }
>
>  QEMUCursor *cursor_ref(QEMUCursor *c)
>  {
> -    c->refcount++;
> +    qatomic_inc(&c->refcount);
>      return c;
>  }
>
>  void cursor_unref(QEMUCursor *c)
>  {
> +    int refcount;
> +
>      if (c == NULL)
>          return;
> -    c->refcount--;
> -    if (c->refcount)
> -        return;
> -    g_free(c);
> +
> +    refcount = qatomic_fetch_dec(&c->refcount);
> +    assert(refcount > 0);
> +    if (refcount == 1) {
> +        g_free(c);
> +    }
>  }
>
>  int cursor_get_mono_bpl(QEMUCursor *c)
> --
> 2.53.0
>

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

[Denis V. Lunev]

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

[Marc-André Lureau]

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