hw/display/qxl.c | 2 ++ ui/spice-display.c | 12 +----------- 2 files changed, 3 insertions(+), 11 deletions(-)
From 62a6bd2f3c3492ff9d7260319bdfd8c55c53ae06 Mon Sep 17 00:00:00 2001
From: lyuming <707242047@qq.com>
Date: Wed, 10 Apr 2024 17:29:40 +0800
Subject: [PATCH] display: fix heap use after free in cursor_put
vnc_dpy_cursor_define() and qxl_spice_reset_cursor() update refcount without ssd.lock,
lock to avoid the problems below:
issues: https://gitlab.com/qemu-project/qemu/-/issues/2261
==57296==ERROR: AddressSanitizer: heap-use-after-free on address 0x623000738110 at pc 0x55cec2ed06aa bp 0x7ffc54d1fea0 sp 0x7ffc54d1fe90
READ of size 4 at 0x623000738110 thread T0
#0 0x55cec2ed06a9 in cursor_put ../qemu-6.0.1/ui/cursor.c:112
#1 0x55cec2f05d40 in vnc_dpy_cursor_define ../qemu-6.0.1/ui/vnc.c:1041
#2 0x55cec2ec6352 in dpy_cursor_define ../qemu-6.0.1/ui/console.c:1841
#3 0x55cec3ab176c in qemu_spice_cursor_refresh_bh ../qemu-6.0.1/ui/spice-display.c:469
#4 0x55cec4abc6eb in aio_bh_call ../qemu-6.0.1/util/async.c:136
#5 0x55cec4abce43 in aio_bh_poll ../qemu-6.0.1/util/async.c:164
#6 0x55cec4a5f457 in aio_dispatch ../qemu-6.0.1/util/aio-posix.c:381
#7 0x55cec4abe386 in aio_ctx_dispatch ../qemu-6.0.1/util/async.c:306
#8 0x7fa4fadcdd3a in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x55d3a)
#9 0x55cec4b0b5d6 in glib_pollfds_poll ../qemu-6.0.1/util/main-loop.c:231
#10 0x55cec4b0b7c0 in os_host_main_loop_wait ../qemu-6.0.1/util/main-loop.c:254
#11 0x55cec4b0bac5 in main_loop_wait ../qemu-6.0.1/util/main-loop.c:530
#12 0x55cec3f49e70 in qemu_main_loop ../qemu-6.0.1/softmmu/runstate.c:786
#13 0x55cec2e7f679 in main ../qemu-6.0.1/softmmu/main.c:50
#14 0x7fa4f96f4d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#15 0x7fa4f96f4e3f in __libc_start_main_impl ../csu/libc-start.c:392
#16 0x55cec2e7f584 in _start (/usr/bin/qemu-system-x86_64+0x298a584)
0x623000738110 is located 16 bytes inside of 6416-byte region [0x623000738100,0x623000739a10)
freed by thread T0 here:
#0 0x7fa4fb7d9537 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127
#1 0x55cec2ed0769 in cursor_put ../qemu-6.0.1/ui/cursor.c:115
#2 0x55cec3ab1818 in qemu_spice_cursor_refresh_bh ../qemu-6.0.1/ui/spice-display.c:471
#3 0x55cec4abc6eb in aio_bh_call ../qemu-6.0.1/util/async.c:136
#4 0x55cec4abce43 in aio_bh_poll ../qemu-6.0.1/util/async.c:164
#5 0x55cec4a5f457 in aio_dispatch ../qemu-6.0.1/util/aio-posix.c:381
#6 0x55cec4abe386 in aio_ctx_dispatch ../qemu-6.0.1/util/async.c:306
#7 0x7fa4fadcdd3a in g_main_context_dispatch (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x55d3a)
previously allocated by thread T14 here:
#0 0x7fa4fb7d9a57 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154
#1 0x7fa4fadd6c50 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x5ec50)
#2 0x55cec3b16918 in qxl_cursor ../qemu-6.0.1/hw/display/qxl-render.c:361
#3 0x55cec3b18698 in qxl_render_cursor ../qemu-6.0.1/hw/display/qxl-render.c:448
#4 0x55cec3af53a5 in interface_get_cursor_command ../qemu-6.0.1/hw/display/qxl.c:856
#5 0x7fa4fb39ca1f in red_process_cursor ../../server/red-worker.c:152
#6 0x7fa4fb39ca1f in red_process_cursor ../../server/red-worker.c:140
Thread T14 created by T0 here:
#0 0x7fa4fb77d685 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:216
#1 0x7fa4fb39ece5 in red_worker_run ../../server/red-worker.c:1588
#2 0x62100002d94f (<unknown module>)
Signed-off-by: lyuming <707242047@qq.com>
---
hw/display/qxl.c | 2 ++
ui/spice-display.c | 12 +-----------
2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 7178dec85d..2bf8ed1e23 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -299,7 +299,9 @@ void qxl_spice_reset_cursor(PCIQXLDevice *qxl)
qxl->guest_cursor = 0;
qemu_mutex_unlock(&qxl->track_lock);
if (qxl->ssd.cursor) {
+ qemu_mutex_lock(&qxl->ssd.lock);
cursor_unref(qxl->ssd.cursor);
+ qemu_mutex_unlock(&qxl->ssd.lock);
}
qxl->ssd.cursor = cursor_builtin_hidden();
}
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 6eb98a5a5c..5703c58d26 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -461,11 +461,7 @@ void qemu_spice_cursor_refresh_bh(void *opaque)
if (ssd->cursor) {
QEMUCursor *c = ssd->cursor;
assert(ssd->dcl.con);
- cursor_ref(c);
- qemu_mutex_unlock(&ssd->lock);
dpy_cursor_define(ssd->dcl.con, c);
- qemu_mutex_lock(&ssd->lock);
- cursor_unref(c);
}
if (ssd->mouse_x != -1 && ssd->mouse_y != -1) {
@@ -475,11 +471,9 @@ void qemu_spice_cursor_refresh_bh(void *opaque)
y = ssd->mouse_y;
ssd->mouse_x = -1;
ssd->mouse_y = -1;
- qemu_mutex_unlock(&ssd->lock);
dpy_mouse_set(ssd->dcl.con, x, y, 1);
- } else {
- qemu_mutex_unlock(&ssd->lock);
}
+ qemu_mutex_unlock(&ssd->lock);
}
void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
@@ -751,12 +745,10 @@ static void display_mouse_set(DisplayChangeListener *dcl,
{
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
- qemu_mutex_lock(&ssd->lock);
ssd->ptr_x = x;
ssd->ptr_y = y;
g_free(ssd->ptr_move);
ssd->ptr_move = qemu_spice_create_cursor_update(ssd, NULL, on);
- qemu_mutex_unlock(&ssd->lock);
qemu_spice_wakeup(ssd);
}
@@ -765,7 +757,6 @@ static void display_mouse_define(DisplayChangeListener *dcl,
{
SimpleSpiceDisplay *ssd = container_of(dcl, SimpleSpiceDisplay, dcl);
- qemu_mutex_lock(&ssd->lock);
cursor_ref(c);
cursor_unref(ssd->cursor);
ssd->cursor = c;
@@ -775,7 +766,6 @@ static void display_mouse_define(DisplayChangeListener *dcl,
ssd->ptr_move = NULL;
g_free(ssd->ptr_define);
ssd->ptr_define = qemu_spice_create_cursor_update(ssd, c, 0);
- qemu_mutex_unlock(&ssd->lock);
qemu_spice_wakeup(ssd);
}
--
2.34.1
Hi On Wed, Apr 10, 2024 at 2:06 PM ゞlym <707242047@qq.com> wrote: > > Please send the patch as inline: https://www.qemu.org/docs/master/devel/submitting-a-patch.html#do-not-send-as-an-attachment The patch is doing too much changes to the ssd.lock usage without explaining in detail which race and how it solved it. In particular, ui/spice-display.c usage seems safer before your change, since it takes the lock on display_refresh and display_mouse_define. It properly temporarily releases the lock before calling the dpy_mouse_set() and dpy_cursor_define() as well. To me, it looks like the only offender is qxl_spice_reset_cursor(), which lacks locking before unrefing. Could you confirm this hypothesis if you are able to reproduce the issue? thanks
Hi
During the test with logging, I found that there may be a conflict between the logic of updating the refcount in vnc_dpy_cursor_define() and QXL_CURSOR_SET action, same as dpy_cursor_define() after commit 385ac97f, and the atomic operation needs to be ensured;
The first thoughts are as follows，only lock cursor_unref/cursor_ref with ssd.lock，But it seems we can't get ssd.lock within dpy_cursor_define，so if we can't lock The top-level function qemu_spice_cursor_refresh_bh()？
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -336,6 +336,7 @@ int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
}
qemu_mutex_lock(&qxl->ssd.lock);
if (qxl->ssd.cursor) {
+ // other thread
cursor_unref(qxl->ssd.cursor);
}
qxl->ssd.cursor = c;
diff --git a/ui/console.c b/ui/console.c
index 43226c5c14..31dbd8fc6f 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -985,8 +985,10 @@ void dpy_cursor_define(QemuConsole *c, QEMUCursor *cursor)
DisplayState *s = c->ds;
DisplayChangeListener *dcl;
+ //lock, main thread
cursor_unref(con->cursor);
con->cursor = cursor_ref(cursor);
+ //unlock
if (!qemu_console_is_visible(c)) {
return;
}
------------------ 原始邮件 ------------------
发件人: "Marc-André Lureau" <marcandre.lureau@redhat.com>;
发送时间: 2024年4月10日(星期三) 晚上9:24
收件人: "ゞlym"<707242047@qq.com>;
抄送: "qemu-devel"<qemu-devel@nongnu.org>;"kraxel"<kraxel@redhat.com>;
主题: Re: [PATCH RFC v1]display: fix heap use after free in cursor_put
Hi
On Wed, Apr 10, 2024 at 2:06 PM ゞlym <707242047@qq.com> wrote:
>
>
Please send the patch as inline:
https://www.qemu.org/docs/master/devel/submitting-a-patch.html#do-not-send-as-an-attachment
The patch is doing too much changes to the ssd.lock usage without
explaining in detail which race and how it solved it.
In particular, ui/spice-display.c usage seems safer before your
change, since it takes the lock on display_refresh and
display_mouse_define. It properly temporarily releases the lock before
calling the dpy_mouse_set() and dpy_cursor_define() as well.
To me, it looks like the only offender is qxl_spice_reset_cursor(),
which lacks locking before unrefing.
Could you confirm this hypothesis if you are able to reproduce the issue?
thanks© 2016 - 2024 Red Hat, Inc.