Re: [PATCH] util: fix use-after-free in module_load_one

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5781 bytes --]

On Wed, Feb 17, 2021 at 2:48 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 17, 2021 at 12:24:03PM +0400, marcandre.lureau@redhat.com
> wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Spotted by ASAN:
> >
> > ==2407186==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x6020003ac4f0 at pc 0x7ffff766659c bp 0x7fffffffd1d0 sp 0x7fffffffc980
> > READ of size 1 at 0x6020003ac4f0 thread T0
> >     #0 0x7ffff766659b  (/lib64/libasan.so.6+0x8a59b)
> >     #1 0x7ffff6bfa843 in g_str_equal ../glib/ghash.c:2303
> >     #2 0x7ffff6bf8167 in g_hash_table_lookup_node ../glib/ghash.c:493
> >     #3 0x7ffff6bf9b78 in g_hash_table_insert_internal
> ../glib/ghash.c:1598
> >     #4 0x7ffff6bf9c32 in g_hash_table_add ../glib/ghash.c:1689
> >     #5 0x5555596caad4 in module_load_one ../util/module.c:233
> >     #6 0x5555596ca949 in module_load_one ../util/module.c:225
> >     #7 0x5555596ca949 in module_load_one ../util/module.c:225
> >     #8 0x5555596cbdf4 in module_load_qom_all ../util/module.c:349
> >     #9 0x5555593c6bbc in qmp_qom_list_types ../qom/qom-qmp-cmds.c:114
> >     #10 0x5555595576df in qmp_marshal_qom_list_types
> qapi/qapi-commands-qom.c:194
> >     #11 0x555559772868 in do_qmp_dispatch_bh ../qapi/qmp-dispatch.c:110
> >     #12 0x5555596f8786 in aio_bh_call ../util/async.c:136
> >     #13 0x5555596f8e9b in aio_bh_poll ../util/async.c:164
> >     #14 0x555559685803 in aio_dispatch ../util/aio-posix.c:381
> >     #15 0x5555596fa324 in aio_ctx_dispatch ../util/async.c:306
> >     #16 0x7ffff6c0deda in g_main_dispatch ../glib/gmain.c:3337
> >     #17 0x7ffff6c0edfd in g_main_context_dispatch ../glib/gmain.c:4055
> >     #18 0x555559726c66 in glib_pollfds_poll ../util/main-loop.c:232
> >     #19 0x555559726e43 in os_host_main_loop_wait ../util/main-loop.c:255
> >     #20 0x555559727139 in main_loop_wait ../util/main-loop.c:531
> >     #21 0x555558fb46fc in qemu_main_loop ../softmmu/runstate.c:722
> >     #22 0x555557d45065 in main ../softmmu/main.c:50
> >     #23 0x7ffff59611e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
> >     #24 0x555557d44f7d in _start
> (/home/elmarco/src/qemu/build/qemu-system-x86_64+0x27f0f7d)
> >
> > 0x6020003ac4f0 is located 0 bytes inside of 10-byte region
> [0x6020003ac4f0,0x6020003ac4fa)
> > freed by thread T0 here:
> >     #0 0x7ffff76870c7 in __interceptor_free (/lib64/libasan.so.6+0xab0c7)
> >     #1 0x7ffff6c16d94 in g_free ../glib/gmem.c:199
> >     #2 0x5555596caae7 in module_load_one ../util/module.c:234
> >     #3 0x5555596ca949 in module_load_one ../util/module.c:225
> >     #4 0x5555596ca949 in module_load_one ../util/module.c:225
> >     #5 0x5555596cbdf4 in module_load_qom_all ../util/module.c:349
> >     #6 0x5555593c6bbc in qmp_qom_list_types ../qom/qom-qmp-cmds.c:114
> >     #7 0x5555595576df in qmp_marshal_qom_list_types
> qapi/qapi-commands-qom.c:194
> >     #8 0x555559772868 in do_qmp_dispatch_bh ../qapi/qmp-dispatch.c:110
> >     #9 0x5555596f8786 in aio_bh_call ../util/async.c:136
> >     #10 0x5555596f8e9b in aio_bh_poll ../util/async.c:164
> >     #11 0x555559685803 in aio_dispatch ../util/aio-posix.c:381
> >     #12 0x5555596fa324 in aio_ctx_dispatch ../util/async.c:306
> >     #13 0x7ffff6c0deda in g_main_dispatch ../glib/gmain.c:3337
> >     #14 0x7ffff6c0edfd in g_main_context_dispatch ../glib/gmain.c:4055
> >     #15 0x555559726c66 in glib_pollfds_poll ../util/main-loop.c:232
> >     #16 0x555559726e43 in os_host_main_loop_wait ../util/main-loop.c:255
> >     #17 0x555559727139 in main_loop_wait ../util/main-loop.c:531
> >     #18 0x555558fb46fc in qemu_main_loop ../softmmu/runstate.c:722
> >     #19 0x555557d45065 in main ../softmmu/main.c:50
> >     #20 0x7ffff59611e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
> >
> > Typical C bug...
>
> This commit message isn't really helping explain the problem.
> Rather than this huge trace which readers have to then debug,
> can we actually say what is wrong.
>
> After reading the docs it seems the problem is as follows:
>
>   "g_hash_table_add always retains ownership of the pointer
>    passed in as the key. Its return status merely indicates
>    whether the added entry was new, or replaced an existing
>    entry. Thus key must never be freed after this method
>    returns."
>
>
Correct, thanks for spelling it out. I can include in the commit and
resend, unless the maintainer (who?.. hmm Paolo?) does it on picking?

thanks

>
> > Fixes: 90629122d2e ("module: use g_hash_table_add()")
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  util/module.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/module.c b/util/module.c
> > index c65060c167..a2ab0bcdbc 100644
> > --- a/util/module.c
> > +++ b/util/module.c
> > @@ -230,10 +230,11 @@ bool module_load_one(const char *prefix, const
> char *lib_name, bool mayfail)
> >          }
> >      }
> >
> > -    if (!g_hash_table_add(loaded_modules, module_name)) {
> > +    if (g_hash_table_contains(loaded_modules, module_name)) {
> >          g_free(module_name);
> >          return true;
> >      }
> > +    g_hash_table_add(loaded_modules, module_name);
> >
> >      search_dir = getenv("QEMU_MODULE_DIR");
> >      if (search_dir != NULL) {
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
>
> 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 :|
>
>

[-- Attachment #2: Type: text/html, Size: 7765 bytes --]