All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
@ 2022-07-20  8:55 Wangjing(Hogan) via
  2022-07-20  9:00 ` Daniel P. Berrangé
  0 siblings, 1 reply; 6+ messages in thread
From: Wangjing(Hogan) via @ 2022-07-20  8:55 UTC (permalink / raw)
  To: Daniel P. Berrangé, Marc-André Lureau
  Cc: Markus Armbruster, QEMU, Paolo Bonzini, Wangxin (Alexander)


> On Wed, Jul 20, 2022 at 11:36:07AM +0400, Marc-André Lureau wrote:
> > Hi
> > 
> > On Wed, Jul 20, 2022 at 11:13 AM Hogan Wang via 
> > <qemu-devel@nongnu.org>
> > wrote:
> > 
> > > IOWatchPoll object did not hold the @ioc and @src objects reference, 
> > > then io_watch_poll_prepare execute in IO thread, if IOWatchPoll 
> > > removed by mian thread, then io_watch_poll_prepare access @ioc or
> > >
> > 
> > mian->main
> > 
> > 
> > > @src concurrently lead to coredump.
> > >
> > > In IO thread monitor scene, the IO thread used to accept client, 
> > > receive qmp request and handle hung-up event. Main thread used to 
> > > handle qmp request and send response, it will remove IOWatchPoll and 
> > > free @ioc when send response fail, then cause use-after-free
> > >
> > 
> > I wonder if we are misusing GSources in that case, by removing sources 
> > from different threads.. Could you be more specific about the code 
> > path that leads to that?
> 
> It is permitted, but unfortunately every version of glib prior to 2.64 has a race condition that means you'll periodically get a use-after-free and a crash:
> 
>   https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
> 
> Libvirt worked around this problem by not calling 'g_source_unref'
> directly, but instead have a helper that uses g_idle_add to delay the unref such that its guaranteed to happen inside the main event loop thread.
> 
> So I'd like to know what version of glib Hogan is using 

I am using glib2-2.62.5 in test environment, so it's looks like a glib2 known issue.

> 
> With regards,
> Daniel
> -- 
>|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
>|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
@ 2022-07-20 10:07 Wangjing(Hogan) via
  0 siblings, 0 replies; 6+ messages in thread
From: Wangjing(Hogan) via @ 2022-07-20 10:07 UTC (permalink / raw)
  To: Daniel P. Berrangé
  Cc: Marc-André Lureau, Markus Armbruster, QEMU, Paolo Bonzini,
	Wangxin (Alexander)

> On Wed, Jul 20, 2022 at 08:55:46AM +0000, Wangjing(Hogan) wrote:
> > 
> > > On Wed, Jul 20, 2022 at 11:36:07AM +0400, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Wed, Jul 20, 2022 at 11:13 AM Hogan Wang via 
> > > > <qemu-devel@nongnu.org>
> > > > wrote:
> > > > 
> > > > > IOWatchPoll object did not hold the @ioc and @src objects 
> > > > > reference, then io_watch_poll_prepare execute in IO thread, if 
> > > > > IOWatchPoll removed by main thread, then io_watch_poll_prepare 
> > > > > access @ioc or @src concurrently lead to coredump.
> > > > >
> > > > > In IO thread monitor scene, the IO thread used to accept client, 
> > > > > receive qmp request and handle hung-up event. Main thread used 
> > > > > to handle qmp request and send response, it will remove 
> > > > > IOWatchPoll and free @ioc when send response fail, then cause 
> > > > > use-after-free
> > > > >
> > > > 
> > > > I wonder if we are misusing GSources in that case, by removing 
> > > > sources from different threads.. Could you be more specific about 
> > > > the code path that leads to that?
> > > 
> > > It is permitted, but unfortunately every version of glib prior to 2.64 has a race condition that means you'll periodically get a use-after-free and a crash:
> > > 
> > >   https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1358
> > > 
> > > Libvirt worked around this problem by not calling 'g_source_unref'
> > > directly, but instead have a helper that uses g_idle_add to delay the unref such that its guaranteed to happen inside the main event loop thread.
> > > 
> > > So I'd like to know what version of glib Hogan is using
> > 
> > I am using glib2-2.62.5 in test environment, so it's looks like a glib2 known issue.
> 
> Hmm, actually the fix should have been backported into the 2.62.5 release according to this
> 
>   https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1361

According to the current source code:
Client connect to the monitor server, chr->gsource(IOWatchPoll) will be initialized by io_add_watch_poll.

According to io_watch_poll_prepare(Execute in IO Thread) function:
IOWatchPoll->src will be added to chr->gsource as a child source while the channel readable, but IOWatchPoll not hold the child source's reference because of g_source_unref(iwp->src), so left the only one reference hold by the child source list.

If client disconnect,then main thread will destroy the IOWatchPoll and unref all the child source by g_source_destroy.
Call trace:
tcp_chr_read
    tcp_chr_disconnect
        tcp_chr_disconnect_locked
            tcp_chr_free_connection
                remove_fd_in_watch
                    g_source_destroy(chr->gsource) /* unref all the child source */
                object_unref(OBJECT(s->ioc));      /* QIOChannel freed and IOWatchPoll->ioc is a wild pointer */
                s->ioc = NULL;

At this time, IOWatchPoll->src and IOWatchPoll->ioc freed, io_watch_poll_prepare called frequently and low-probability use-after-free.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2] chardev: avoid use-after-free when client disconnect
@ 2022-07-20  7:10 Hogan Wang via
  2022-07-20  7:36 ` Marc-André Lureau
  0 siblings, 1 reply; 6+ messages in thread
From: Hogan Wang via @ 2022-07-20  7:10 UTC (permalink / raw)
  To: marcandre.lureau, berrange, qemu-devel
  Cc: pbonzini, armbru, wangxinxin.wang, hogan.wang

IOWatchPoll object did not hold the @ioc and @src objects reference,
then io_watch_poll_prepare execute in IO thread, if IOWatchPoll
removed by mian thread, then io_watch_poll_prepare access @ioc or
@src concurrently lead to coredump.

In IO thread monitor scene, the IO thread used to accept client,
receive qmp request and handle hung-up event. Main thread used to
handle qmp request and send response, it will remove IOWatchPoll
and free @ioc when send response fail, then cause use-after-free
like this:

(gdb) bt
0  0x00007f4d121c8edf in g_source_remove_child_source (source=0x7f4c58003560, child_source=0x7f4c58009b10)
1  0x00007f4d11e0705c in io_watch_poll_prepare (source=0x7f4c58003560, timeout=timeout@entry=0x7f4c7fffed94
2  0x00007f4d121ca419 in g_main_context_prepare (context=context@entry=0x55a1463f8260, priority=priority@entry=0x7f4c7fffee20)
3  0x00007f4d121cadeb in g_main_context_iterate (context=0x55a1463f8260, block=block@entry=1, dispatch=dispatch@entry=1, self=self@entry=0x7f4c94002260)
4  0x00007f4d121cb21d in g_main_loop_run (loop=0x55a146c90920)
5  0x00007f4d11de3ea1 in iothread_run (opaque=0x55a146411820)
6  0x00007f4d11d77470 in qemu_thread_start (args=0x55a146b1f3c0)
7  0x00007f4d11f2ef3b in ?? () from /usr/lib64/libpthread.so.0
8  0x00007f4d120ba550 in clone () from /usr/lib64/libc.so.6
(gdb) p	iwp
$1 = (IOWatchPoll *) 0x7f4c58003560
(gdb) p	*iwp
$2 = {parent = {callback_data = 0x0,
                callback_funcs = 0x0,
                source_funcs = 0x7f4d11f10760 <io_watch_poll_funcs>,
                ref_count = 1,
                context = 0x55a1463f8260,
                priority = 0,
                flags = 0,
                source_id = 544,
                poll_fds = 0x0,
                prev = 0x55a147a47a30,
                next = 0x55a14712fb80,
                name = 0x7f4c580036d0 "chardev-iowatch-charmonitor",
                priv = 0x7f4c58003060},
       ioc = 0x7f4c580033f0,
       src = 0x7f4c58009b10,
       fd_can_read = 0x7f4d11e0a5d0 <tcp_chr_read_poll>,
       fd_read = 0x7f4d11e0a380 <tcp_chr_read>,
       opaque = 0x55a1463aeea0 }
(gdb) p	iwp->ioc
$3 = (QIOChannel *) 0x7f4c580033f0
(gdb) p	*iwp->ioc
$4 = {parent = {class = 0x55a14707f400,
                free = 0x55a146313010,
                properties = 0x55a147b37b60,
                ref = 0,
                parent = 0x0},
      features = 3,
      name = 0x7f4c580085f0 "\240F",
      ctx = 0x0,
      read_coroutine = 0x0,
      write_coroutine = 0x0}

Solution: IOWatchPoll object hold the @ioc and @src objects reference
and release the reference in GSource finalize callback function.

Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
---
 chardev/char-io.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index 4451128cba..6b10217a70 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -55,9 +55,9 @@ static gboolean io_watch_poll_prepare(GSource *source,
             iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
         g_source_set_callback(iwp->src, iwp->fd_read, iwp->opaque, NULL);
         g_source_add_child_source(source, iwp->src);
-        g_source_unref(iwp->src);
     } else {
         g_source_remove_child_source(source, iwp->src);
+        g_source_unref(iwp->src);
         iwp->src = NULL;
     }
     return FALSE;
@@ -69,9 +69,17 @@ static gboolean io_watch_poll_dispatch(GSource *source, GSourceFunc callback,
     return G_SOURCE_CONTINUE;
 }
 
+static void io_watch_poll_finalize(GSource *source)
+{
+    IOWatchPoll *iwp = io_watch_poll_from_source(source);
+    g_clear_pointer(&iwp->src, g_source_unref);
+    g_clear_pointer(&iwp->ioc, object_unref);
+}
+
 static GSourceFuncs io_watch_poll_funcs = {
     .prepare = io_watch_poll_prepare,
     .dispatch = io_watch_poll_dispatch,
+    .finalize = io_watch_poll_finalize,
 };
 
 GSource *io_add_watch_poll(Chardev *chr,
@@ -88,7 +96,7 @@ GSource *io_add_watch_poll(Chardev *chr,
                                        sizeof(IOWatchPoll));
     iwp->fd_can_read = fd_can_read;
     iwp->opaque = user_data;
-    iwp->ioc = ioc;
+    iwp->ioc = object_ref(ioc);
     iwp->fd_read = (GSourceFunc) fd_read;
     iwp->src = NULL;
 
-- 
2.33.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-20 10:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-07-20  8:55 [PATCH v2] chardev: avoid use-after-free when client disconnect Wangjing(Hogan) via
2022-07-20  9:00 ` Daniel P. Berrangé
  -- strict thread matches above, loose matches on Subject: below --
2022-07-20 10:07 Wangjing(Hogan) via
2022-07-20  7:10 Hogan Wang via
2022-07-20  7:36 ` Marc-André Lureau
2022-07-20  8:19   ` Daniel P. Berrangé

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.