[PATCH] ui/console: fix use-after-free in qemu_console_set_cursor

[PATCH] ui/console: fix use-after-free in qemu_console_set_cursor

[marcandre.lureau]

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

When the new cursor is the same object as the one already held by
the console and the refcount is 1, cursor_unref frees it before
cursor_ref can increment the count.

Ref the incoming cursor before unreffing the old one so the
refcount goes 1-2-1 instead of 1-0 (freed) then use-after-free.

This is related to 385ac97f8fad ("ui: keep current cursor with
QemuConsole"), but could have happened earlier with VNC too.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index a8a2a247d8f4..4db8f1cb431f 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -904,8 +904,9 @@ void qemu_console_set_cursor(QemuConsole *c, QEMUCursor *cursor)
     DisplayState *s = c->ds;
     DisplayChangeListener *dcl;
 
+    cursor_ref(cursor);
     cursor_unref(con->cursor);
-    con->cursor = cursor_ref(cursor);
+    con->cursor = cursor;
     QLIST_FOREACH(dcl, &s->listeners, next) {
         if (c != dcl->con) {
             continue;
-- 
2.55.0.543.g5ebe2ebe4ea8

Re: [PATCH] ui/console: fix use-after-free in qemu_console_set_cursor

[Daniel P. Berrangé]

On Thu, Aug 27, 2026 at 11:06:47AM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> When the new cursor is the same object as the one already held by
> the console and the refcount is 1, cursor_unref frees it before
> cursor_ref can increment the count.
> 
> Ref the incoming cursor before unreffing the old one so the
> refcount goes 1-2-1 instead of 1-0 (freed) then use-after-free.

I don't see where/how this can happen.  The caller of
qemu_console_set_cursor should own its own reference
on 'cursor'. So on entry to this method, 'cursor'
should have a ref of 2 if it is the same as the
current con->cursor.

if anything the behaviour you describe sounds like a
bug in a caller not holding its own reference.

> 
> This is related to 385ac97f8fad ("ui: keep current cursor with
> QemuConsole"), but could have happened earlier with VNC too.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  ui/console.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ui/console.c b/ui/console.c
> index a8a2a247d8f4..4db8f1cb431f 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -904,8 +904,9 @@ void qemu_console_set_cursor(QemuConsole *c, QEMUCursor *cursor)
>      DisplayState *s = c->ds;
>      DisplayChangeListener *dcl;
>  
> +    cursor_ref(cursor);
>      cursor_unref(con->cursor);
> -    con->cursor = cursor_ref(cursor);
> +    con->cursor = cursor;
>      QLIST_FOREACH(dcl, &s->listeners, next) {
>          if (c != dcl->con) {
>              continue;
> -- 
> 2.55.0.543.g5ebe2ebe4ea8
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] ui/console: fix use-after-free in qemu_console_set_cursor

[Marc-André Lureau]

Hi

On Thu, Aug 27, 2026 at 12:47 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Aug 27, 2026 at 11:06:47AM +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > When the new cursor is the same object as the one already held by
> > the console and the refcount is 1, cursor_unref frees it before
> > cursor_ref can increment the count.
> >
> > Ref the incoming cursor before unreffing the old one so the
> > refcount goes 1-2-1 instead of 1-0 (freed) then use-after-free.
>
> I don't see where/how this can happen.  The caller of
> qemu_console_set_cursor should own its own reference
> on 'cursor'. So on entry to this method, 'cursor'
> should have a ref of 2 if it is the same as the
> current con->cursor.
>
> if anything the behaviour you describe sounds like a
> bug in a caller not holding its own reference.

You are right, I thought virtio-gpu kept a weak pointer, but it should
have a strong ref, so we should never reach 0. I can't explain the
crash I observed then.

>
> >
> > This is related to 385ac97f8fad ("ui: keep current cursor with
> > QemuConsole"), but could have happened earlier with VNC too.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  ui/console.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui/console.c b/ui/console.c
> > index a8a2a247d8f4..4db8f1cb431f 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -904,8 +904,9 @@ void qemu_console_set_cursor(QemuConsole *c, QEMUCursor *cursor)
> >      DisplayState *s = c->ds;
> >      DisplayChangeListener *dcl;
> >
> > +    cursor_ref(cursor);
> >      cursor_unref(con->cursor);
> > -    con->cursor = cursor_ref(cursor);
> > +    con->cursor = cursor;
> >      QLIST_FOREACH(dcl, &s->listeners, next) {
> >          if (c != dcl->con) {
> >              continue;
> > --
> > 2.55.0.543.g5ebe2ebe4ea8
> >
> >
>
> With regards,
> Daniel
> --
> |: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
> |: https://libvirt.org          ~~          https://entangle-photo.org :|
> |: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|
>
>

Re: [PATCH] ui/console: fix use-after-free in qemu_console_set_cursor

[Daniel P. Berrangé]

On Thu, Aug 27, 2026 at 01:58:14PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 27, 2026 at 12:47 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Aug 27, 2026 at 11:06:47AM +0400, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > When the new cursor is the same object as the one already held by
> > > the console and the refcount is 1, cursor_unref frees it before
> > > cursor_ref can increment the count.
> > >
> > > Ref the incoming cursor before unreffing the old one so the
> > > refcount goes 1-2-1 instead of 1-0 (freed) then use-after-free.
> >
> > I don't see where/how this can happen.  The caller of
> > qemu_console_set_cursor should own its own reference
> > on 'cursor'. So on entry to this method, 'cursor'
> > should have a ref of 2 if it is the same as the
> > current con->cursor.
> >
> > if anything the behaviour you describe sounds like a
> > bug in a caller not holding its own reference.
> 
> You are right, I thought virtio-gpu kept a weak pointer, but it should
> have a strong ref, so we should never reach 0. I can't explain the
> crash I observed then.

Is there a bug report for this or crash scenario info ? 

Something wrong with the 'cursor' object stored in virtio_gpu_scanout ?
I notice in the VMState:

        VMSTATE_UINT32(cursor.resource_id, struct virtio_gpu_scanout),
        VMSTATE_UINT32(cursor.hot_x, struct virtio_gpu_scanout),
        VMSTATE_UINT32(cursor.hot_y, struct virtio_gpu_scanout),
        VMSTATE_UINT32(cursor.pos.x, struct virtio_gpu_scanout),
        VMSTATE_UINT32(cursor.pos.y, struct virtio_gpu_scanout),

what is restoring the "refcount" to "1" when vmstate is loaded ?

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] ui/console: fix use-after-free in qemu_console_set_cursor

[Marc-André Lureau]

Hi

On Thu, Aug 27, 2026 at 2:31 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Aug 27, 2026 at 01:58:14PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Aug 27, 2026 at 12:47 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Thu, Aug 27, 2026 at 11:06:47AM +0400, marcandre.lureau@redhat.com wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > > > When the new cursor is the same object as the one already held by
> > > > the console and the refcount is 1, cursor_unref frees it before
> > > > cursor_ref can increment the count.
> > > >
> > > > Ref the incoming cursor before unreffing the old one so the
> > > > refcount goes 1-2-1 instead of 1-0 (freed) then use-after-free.
> > >
> > > I don't see where/how this can happen.  The caller of
> > > qemu_console_set_cursor should own its own reference
> > > on 'cursor'. So on entry to this method, 'cursor'
> > > should have a ref of 2 if it is the same as the
> > > current con->cursor.
> > >
> > > if anything the behaviour you describe sounds like a
> > > bug in a caller not holding its own reference.
> >
> > You are right, I thought virtio-gpu kept a weak pointer, but it should
> > have a strong ref, so we should never reach 0. I can't explain the
> > crash I observed then.
>
> Is there a bug report for this or crash scenario info ?

No, I just had this report from ASAN when testing -display dbus with
virtio-gpu. Pretty regular test.

> Something wrong with the 'cursor' object stored in virtio_gpu_scanout ?
> I notice in the VMState:
>
>         VMSTATE_UINT32(cursor.resource_id, struct virtio_gpu_scanout),
>         VMSTATE_UINT32(cursor.hot_x, struct virtio_gpu_scanout),
>         VMSTATE_UINT32(cursor.hot_y, struct virtio_gpu_scanout),
>         VMSTATE_UINT32(cursor.pos.x, struct virtio_gpu_scanout),
>         VMSTATE_UINT32(cursor.pos.y, struct virtio_gpu_scanout),
>
> what is restoring the "refcount" to "1" when vmstate is loaded ?

cursor != current_cursor (allocated with update_cursor on post_load)

Re: [PATCH] ui/console: fix use-after-free in qemu_console_set_cursor

[Akihiko Odaki]

On 2026/08/27 19:36, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 27, 2026 at 2:31 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Thu, Aug 27, 2026 at 01:58:14PM +0400, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Thu, Aug 27, 2026 at 12:47 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>>
>>>> On Thu, Aug 27, 2026 at 11:06:47AM +0400, marcandre.lureau@redhat.com wrote:
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> When the new cursor is the same object as the one already held by
>>>>> the console and the refcount is 1, cursor_unref frees it before
>>>>> cursor_ref can increment the count.
>>>>>
>>>>> Ref the incoming cursor before unreffing the old one so the
>>>>> refcount goes 1-2-1 instead of 1-0 (freed) then use-after-free.
>>>>
>>>> I don't see where/how this can happen.  The caller of
>>>> qemu_console_set_cursor should own its own reference
>>>> on 'cursor'. So on entry to this method, 'cursor'
>>>> should have a ref of 2 if it is the same as the
>>>> current con->cursor.
>>>>
>>>> if anything the behaviour you describe sounds like a
>>>> bug in a caller not holding its own reference.
>>>
>>> You are right, I thought virtio-gpu kept a weak pointer, but it should
>>> have a strong ref, so we should never reach 0. I can't explain the
>>> crash I observed then.
>>
>> Is there a bug report for this or crash scenario info ?
> 
> No, I just had this report from ASAN when testing -display dbus with
> virtio-gpu. Pretty regular test.

dbus_cursor_define() looks suspici

 > @data is not modified by this function and must remain valid with an
 > unchanging value until such a time as @notify is called with
 > @user_data.  ous; it exposes mutable cursor pixels through a 
zero-copy GVariant and arranges for cursor_unref() to run when that 
variant is destroyed.

The documentation of g_dbus_connection_add_filter() says "filters are 
run in a dedicated message handling thread", but QEMUCursor::refcount 
uses unsynchronized ++/--, which may explain your ASAN crash.

Meanwhile, virtio-gpu repeatedly mutates the same cursor, which violates 
g_variant_new_from_data()’s requirement that its bytes remain unchanged 
until notification. Its documentation says "if the contents of @data 
change before that time then the result is undefined."

Regards,
Akihiko Odaki

> 
>> Something wrong with the 'cursor' object stored in virtio_gpu_scanout ?
>> I notice in the VMState:
>>
>>          VMSTATE_UINT32(cursor.resource_id, struct virtio_gpu_scanout),
>>          VMSTATE_UINT32(cursor.hot_x, struct virtio_gpu_scanout),
>>          VMSTATE_UINT32(cursor.hot_y, struct virtio_gpu_scanout),
>>          VMSTATE_UINT32(cursor.pos.x, struct virtio_gpu_scanout),
>>          VMSTATE_UINT32(cursor.pos.y, struct virtio_gpu_scanout),
>>
>> what is restoring the "refcount" to "1" when vmstate is loaded ?
> 
> cursor != current_cursor (allocated with update_cursor on post_load)