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

[Hogan Wang]

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