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

["Denis V. Lunev" <den@virtuozzo.com>]

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