diff mbox series

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

Message ID tencent_EC82468ED075970213367AE7801FBA4C0405@qq.com
State New
Headers show
Series 回复： [PATCH RFC v1]display: fix heap use after free in cursor_put | expand

Commit Message

ゞlym April 11, 2024, 1:55 a.m. UTC 
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()？







------------------ 原始邮件 ------------------
发件人:                                                                                                                        "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
diff mbox series

Patch

--- 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;}