[PATCH v2] chardev: avoid use-after-free when client disconnect

Hogan Wang via posted 1 patch 1 year, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220720071057.1745-1-hogan.wang@huawei.com
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
chardev/char-io.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
[PATCH v2] chardev: avoid use-after-free when client disconnect
Posted by Hogan Wang via 1 year, 8 months ago
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
Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
Posted by Marc-André Lureau 1 year, 8 months ago
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?


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

That backtrace isn't so useful. If you can produce an ASAN error though,
that would be more explicit. Not a blocker.

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

ok, if we are not misusing GSource, otherwise seems needless or misleading


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

Daniel, Markus, please take a look


-- 
Marc-André Lureau
Re: [PATCH v2] chardev: avoid use-after-free when client disconnect
Posted by Daniel P. Berrangé 1 year, 8 months ago
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 

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 :|