1. Patchew
  2. QEMU
  3. View series

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

Hogan Wang via posted 1 patch 3 years, 10 months ago
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220328082207.2525-1-hogan.wang@huawei.com
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
chardev/char-io.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
[PATCH] chardev: avoid use-after-free when client disconnect
Posted by Hogan Wang via 3 years, 10 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, io_watch_poll_prepare may execute at last
chance and access the freed @ioc or @src object.

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 | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index 8ced184160..3af388eeaa 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,23 @@ 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);
+    if (iwp->src) {
+        g_source_unref(iwp->src);
+        iwp->src = NULL;
+    }
+    if (iwp->ioc) {
+        object_unref(OBJECT(iwp->ioc));
+        iwp->ioc = NULL;
+    }
+}
+
 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,6 +102,7 @@ GSource *io_add_watch_poll(Chardev *chr,
                                        sizeof(IOWatchPoll));
     iwp->fd_can_read = fd_can_read;
     iwp->opaque = user_data;
+    object_ref(OBJECT(ioc));
     iwp->ioc = ioc;
     iwp->fd_read = (GSourceFunc) fd_read;
     iwp->src = NULL;
-- 
2.33.0
Re: [PATCH] chardev: avoid use-after-free when client disconnect
Posted by Marc-André Lureau 3 years, 10 months ago 
Hi

On Mon, Mar 28, 2022 at 12:22 PM 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, io_watch_poll_prepare may execute at last
> chance and access the freed @ioc or @src object.
>
> 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 | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 8ced184160..3af388eeaa 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);
>

This change looks unnecessary. According to g_source_add_child_source()
documentation: *"**source* will hold a reference on *child_source* while
*child_source* is attached to it."


>          iwp->src = NULL;
>      }
>      return FALSE;
> @@ -69,9 +69,23 @@ 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);
> +    if (iwp->src) {
> +        g_source_unref(iwp->src);
>

(see above)


> +        iwp->src = NULL;
> +    }
> +    if (iwp->ioc) {
> +        object_unref(OBJECT(iwp->ioc));
> +        iwp->ioc = NULL;
> +    }
>

replace with 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,6 +102,7 @@ GSource *io_add_watch_poll(Chardev *chr,
>                                         sizeof(IOWatchPoll));
>      iwp->fd_can_read = fd_can_read;
>      iwp->opaque = user_data;
> +    object_ref(OBJECT(ioc));
>      iwp->ioc = ioc;
>

please use "iwp->ioc = object_ref(ioc)"


>      iwp->fd_read = (GSourceFunc) fd_read;
>      iwp->src = NULL;
> --
> 2.33.0
>
>
>
Daniel, please review
thanks

-- 
Marc-André Lureau
Re: [PATCH] chardev: avoid use-after-free when client disconnect
Posted by Hogan Wang via 3 years, 9 months ago 
> Hi
> 
> On Mon, Mar 28, 2022 at 12:22 PM 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, io_watch_poll_prepare may execute at last
> > chance and access the freed @ioc or @src object.
> > 
> > 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 8ced184160..7c20a6027c 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;
> 
> This change looks unnecessary. According to g_source_add_child_source() documentation: 
> "source will hold a reference on child_source while child_source is attached to it."

Thansk for your good suggestions, I think it's necessary. Although parent source hold 
a reference for the child source, but parent source will dereference and release the
child source object while IOWatchPoll removed by mian thread, and then IOWatchPoll will
hold the wild pointer. IOThread may access the wild pointer cause sigfault at this time.

> 
> >      }
> >      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;
> >  
Accept your good suggestions, and I do fix it.

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.