[PATCH RFC v1]display: fix heap use after free in cursor_put

ゞlym posted 1 patch 1 month, 2 weeks ago
hw/display/qxl.c   |  2 ++
ui/spice-display.c | 12 +-----------
2 files changed, 3 insertions(+), 11 deletions(-)
[PATCH RFC v1]display: fix heap use after free in cursor_put
Posted by ゞlym 1 month, 2 weeks ago
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

Re: [PATCH RFC v1]display: fix heap use after free in cursor_put
Posted by Marc-André Lureau 1 month, 2 weeks ago
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
回复: [PATCH RFC v1]display: fix heap use after free in cursor_put
Posted by ゞlym 1 month, 2 weeks ago
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&nbsp;action,&nbsp; same as dpy_cursor_define() after commit&nbsp;385ac97f,&nbsp;&nbsp;and the atomic operation needs to be ensured;


The first thoughts are as follows,only lock cursor_unref/cursor_ref with ssd.lock,But&nbsp;it seems we can't get ssd.lock within dpy_cursor_define,so if we can't lock The top-level function&nbsp;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)
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;}
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;qemu_mutex_lock(&amp;qxl-&gt;ssd.lock);
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;if (qxl-&gt;ssd.cursor) {
+&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; // other thread
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;cursor_unref(qxl-&gt;ssd.cursor);
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;}
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;qxl-&gt;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)
&nbsp; &nbsp; &nbsp;DisplayState *s = c-&gt;ds;
&nbsp; &nbsp; &nbsp;DisplayChangeListener *dcl;
&nbsp;
+&nbsp; &nbsp; //lock, main thread
&nbsp; &nbsp; &nbsp;cursor_unref(con-&gt;cursor);
&nbsp; &nbsp; &nbsp;con-&gt;cursor = cursor_ref(cursor);
+&nbsp; &nbsp; //unlock
&nbsp; &nbsp; &nbsp;if (!qemu_console_is_visible(c)) {
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;return;
&nbsp; &nbsp; &nbsp;}





------------------&nbsp;原始邮件&nbsp;------------------
发件人:                                                                                                                        "Marc-André Lureau"                                                                                    <marcandre.lureau@redhat.com&gt;;
发送时间:&nbsp;2024年4月10日(星期三) 晚上9:24
收件人:&nbsp;"ゞlym"<707242047@qq.com&gt;;
抄送:&nbsp;"qemu-devel"<qemu-devel@nongnu.org&gt;;"kraxel"<kraxel@redhat.com&gt;;
主题:&nbsp;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&gt; wrote:
&gt;
&gt;

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