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

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

[Marc-André Lureau]

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

=?gb18030?B?u9i4tKO6IFtQQVRDSCBSRkMgdjFdZGlzcGxheTogZml4IGhlYXAgdXNlIGFmdGVyIGZyZWUgaW4gY3Vyc29yX3B1dA==?=

[=?gb18030?B?qWdseW0=?=]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030", Size: 3075 bytes --]

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&gt;;
·¢ËÍʱ¼ä:&nbsp;2024Äê4ÔÂ10ÈÕ(ÐÇÆÚÈý) ÍíÉÏ9:24
ÊÕ¼þÈË:&nbsp;"©glym"<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6§2PM ©glym <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

[-- Attachment #2: Type: text/html, Size: 4285 bytes --]