Hi, I've sent another patch to fix this UAF: "migration: Fix use-after-free during process exit"
It's more simple and fixes only the regression. Regards, Yury 05.04.2019, 12:07, "Dr. David Alan Gilbert" <[email protected]>: > * Yury Kotov ([email protected]) wrote: >> 03.04.2019, 22:06, "Dr. David Alan Gilbert" <[email protected]>: >> > * Yury Kotov ([email protected]) wrote: >> >> It fixes heap-use-after-free which was found by clang's ASAN. >> >> >> >> Control flow of this use-after-free: >> >> main_thread: >> >> * Got SIGTERM and completes main loop >> >> * Calls migration_shutdown >> >> - migrate_fd_cancel (so, migration_thread begins to complete) >> >> - object_unref(OBJECT(current_migration)); >> >> >> >> migration_thread: >> >> * migration_iteration_finish -> schedule cleanup bh >> >> * object_unref(OBJECT(s)); (Now, current_migration is freed) >> >> * exits >> >> >> >> main_thread: >> >> * Calls vm_shutdown -> drain bdrvs -> main loop >> >> -> cleanup_bh -> use after free >> >> >> >> If you want to reproduce, these couple of sleeps will help: >> >> vl.c:4613: >> >> migration_shutdown(); >> >> + sleep(2); >> >> migration.c:3269: >> >> + sleep(1); >> >> trace_migration_thread_after_loop(); >> >> migration_iteration_finish(s); >> > >> > Fun; I hadn't realised you could get more main loop after exiting >> > the main loop. >> > >> > Is this fallout from my 892ae715b6b or is this just another case it >> > didn't catch? >> > I guess it moved the shutdown early, so it probably is fallout, and >> > so probably does need to go to 4.0 ? >> > >> I think this is closer to another case than fallout. Because without >> 892ae715b6b, reproducing of UAF at exit was much easier. >> Another way to fix it - remove unref from migration_shutdown >> (but keep migrate_fd_cancel) and restore migration_object_finalize. > > OK, so in that case I'd rather leave this to 4.1 rather than rush it in > 4.0rc's - I'd rather only take regressions at this point. > > Dave > >> >> Original output: >> >> qemu-system-x86_64: terminating on signal 15 from pid 31980 (<unknown >> process>) >> >> ================================================================= >> >> ==31958==ERROR: AddressSanitizer: heap-use-after-free on address >> 0x61900001d210 >> >> at pc 0x555558a535ca bp 0x7fffffffb190 sp 0x7fffffffb188 >> >> READ of size 8 at 0x61900001d210 thread T0 (qemu-vm-0) >> >> #0 0x555558a535c9 in migrate_fd_cleanup >> migration/migration.c:1502:23 >> >> #1 0x5555594fde0a in aio_bh_call util/async.c:90:5 >> >> #2 0x5555594fe522 in aio_bh_poll util/async.c:118:13 >> >> #3 0x555559524783 in aio_poll util/aio-posix.c:725:17 >> >> #4 0x555559504fb3 in aio_wait_bh_oneshot util/aio-wait.c:71:5 >> >> #5 0x5555573bddf6 in virtio_blk_data_plane_stop >> >> hw/block/dataplane/virtio-blk.c:282:5 >> >> #6 0x5555589d5c09 in virtio_bus_stop_ioeventfd >> hw/virtio/virtio-bus.c:246:9 >> >> #7 0x5555589e9917 in virtio_pci_stop_ioeventfd >> hw/virtio/virtio-pci.c:287:5 >> >> #8 0x5555589e22bf in virtio_pci_vmstate_change >> hw/virtio/virtio-pci.c:1072:9 >> >> #9 0x555557628931 in virtio_vmstate_change hw/virtio/virtio.c:2257:9 >> >> #10 0x555557c36713 in vm_state_notify vl.c:1605:9 >> >> #11 0x55555716ef53 in do_vm_stop cpus.c:1074:9 >> >> #12 0x55555716eeff in vm_shutdown cpus.c:1092:12 >> >> #13 0x555557c4283e in main vl.c:4617:5 >> >> #14 0x7fffdfdb482f in __libc_start_main >> >> (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) >> >> #15 0x555556ecb118 in _start >> (x86_64-softmmu/qemu-system-x86_64+0x1977118) >> >> >> >> 0x61900001d210 is located 144 bytes inside of 952-byte region >> >> [0x61900001d180,0x61900001d538) >> >> freed by thread T6 (live_migration) here: >> >> #0 0x555556f76782 in __interceptor_free >> >> >> /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:124:3 >> >> #1 0x555558d5fa94 in object_finalize qom/object.c:618:9 >> >> #2 0x555558d57651 in object_unref qom/object.c:1068:9 >> >> #3 0x555558a55588 in migration_thread migration/migration.c:3272:5 >> >> #4 0x5555595393f2 in qemu_thread_start >> util/qemu-thread-posix.c:502:9 >> >> #5 0x7fffe057f6b9 in start_thread >> (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9) >> >> >> >> previously allocated by thread T0 (qemu-vm-0) here: >> >> #0 0x555556f76b03 in __interceptor_malloc >> >> >> /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3 >> >> #1 0x7ffff6ee37b8 in g_malloc >> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4f7b8) >> >> #2 0x555558d58031 in object_new qom/object.c:640:12 >> >> #3 0x555558a31f21 in migration_object_init >> migration/migration.c:139:25 >> >> #4 0x555557c41398 in main vl.c:4320:5 >> >> #5 0x7fffdfdb482f in __libc_start_main >> (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) >> >> >> >> Thread T6 (live_migration) created by T0 (qemu-vm-0) here: >> >> #0 0x555556f5f0dd in pthread_create >> >> >> /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_interceptors.cc:210:3 >> >> #1 0x555559538cf9 in qemu_thread_create >> util/qemu-thread-posix.c:539:11 >> >> #2 0x555558a53304 in migrate_fd_connect migration/migration.c:3332:5 >> >> #3 0x555558a72bd8 in migration_channel_connect >> migration/channel.c:92:5 >> >> #4 0x555558a6ef87 in exec_start_outgoing_migration >> migration/exec.c:42:5 >> >> #5 0x555558a4f3c2 in qmp_migrate migration/migration.c:1922:9 >> >> #6 0x555558bb4f6a in qmp_marshal_migrate >> qapi/qapi-commands-migration.c:607:5 >> >> #7 0x555559363738 in do_qmp_dispatch qapi/qmp-dispatch.c:131:5 >> >> #8 0x555559362a15 in qmp_dispatch qapi/qmp-dispatch.c:174:11 >> >> #9 0x5555571bac15 in monitor_qmp_dispatch monitor.c:4124:11 >> >> #10 0x55555719a22d in monitor_qmp_bh_dispatcher monitor.c:4207:9 >> >> #11 0x5555594fde0a in aio_bh_call util/async.c:90:5 >> >> #12 0x5555594fe522 in aio_bh_poll util/async.c:118:13 >> >> #13 0x5555595201e0 in aio_dispatch util/aio-posix.c:460:5 >> >> #14 0x555559503553 in aio_ctx_dispatch util/async.c:261:5 >> >> #15 0x7ffff6ede196 in g_main_context_dispatch >> >> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4a196) >> >> >> >> SUMMARY: AddressSanitizer: heap-use-after-free >> migration/migration.c:1502:23 >> >> in migrate_fd_cleanup >> >> Shadow bytes around the buggy address: >> >> 0x0c327fffb9f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa >> >> 0x0c327fffba00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa >> >> 0x0c327fffba10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa >> >> 0x0c327fffba20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa >> >> 0x0c327fffba30: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> >> =>0x0c327fffba40: fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd >> >> 0x0c327fffba50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> >> 0x0c327fffba60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> >> 0x0c327fffba70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> >> 0x0c327fffba80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> >> 0x0c327fffba90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd >> >> Shadow byte legend (one shadow byte represents 8 application bytes): >> >> Addressable: 00 >> >> Partially addressable: 01 02 03 04 05 06 07 >> >> Heap left redzone: fa >> >> Freed heap region: fd >> >> Stack left redzone: f1 >> >> Stack mid redzone: f2 >> >> Stack right redzone: f3 >> >> Stack after return: f5 >> >> Stack use after scope: f8 >> >> Global redzone: f9 >> >> Global init order: f6 >> >> Poisoned by user: f7 >> >> Container overflow: fc >> >> Array cookie: ac >> >> Intra object redzone: bb >> >> ASan internal: fe >> >> Left alloca redzone: ca >> >> Right alloca redzone: cb >> >> Shadow gap: cc >> >> ==31958==ABORTING >> >> >> >> Signed-off-by: Yury Kotov <[email protected]> >> >> --- >> >> migration/migration.c | 30 ++++++++++++++++++++++++------ >> >> 1 file changed, 24 insertions(+), 6 deletions(-) >> >> >> >> diff --git a/migration/migration.c b/migration/migration.c >> >> index 609e0df5d0..54d82bb88b 100644 >> >> --- a/migration/migration.c >> >> +++ b/migration/migration.c >> >> @@ -128,6 +128,8 @@ static int migration_maybe_pause(MigrationState *s, >> >> int *current_active_state, >> >> int new_state); >> >> static void migrate_fd_cancel(MigrationState *s); >> >> +static void migrate_join_thread(MigrationState *s); >> >> +static void migrate_fd_cleanup(void *opaque); >> >> >> >> void migration_object_init(void) >> >> { >> >> @@ -176,6 +178,17 @@ void migration_shutdown(void) >> >> * stop the migration using this structure >> >> */ >> >> migrate_fd_cancel(current_migration); >> >> + /* >> >> + * migration_thread schedules cleanup_bh, but migration_shutdown is >> called >> >> + * from the end of main, so cleanu_up may not be called because >> main-loop is >> >> + * complete. So, wait for the complition of migration_thread, cancel >> bh and >> >> + * call cleanup ourselves. >> >> + */ >> > >> > (Some typos) >> > >> Ok :) >> >> >> + migrate_join_thread(current_migration); >> > >> > I'll admit to being a bit nervous that something in here could be >> > hanging and thus we could hang here rather than die as SIGTERM is >> > expecting us to. >> > >> I think it's only way to be sure we havn't other races with >> migration_thread. >> And this joining is cheaper than vm_shutdown IMHO. >> >> >> + if (current_migration->cleanup_bh) { >> >> + qemu_bh_cancel(current_migration->cleanup_bh); >> > >> > Is that needed? Doesn't migrate_fd_cleanup start with a qemu_bh_delete? >> > >> Agree, it's odd. >> >> >> + migrate_fd_cleanup(current_migration); >> >> + } >> >> object_unref(OBJECT(current_migration)); >> >> } >> >> >> >> @@ -1495,6 +1508,16 @@ static void >> block_cleanup_parameters(MigrationState *s) >> >> } >> >> } >> >> >> >> +static void migrate_join_thread(MigrationState *s) >> >> +{ >> >> + qemu_mutex_unlock_iothread(); >> >> + if (s->migration_thread_running) { >> >> + qemu_thread_join(&s->thread); >> >> + s->migration_thread_running = false; >> >> + } >> >> + qemu_mutex_lock_iothread(); >> >> +} >> >> + >> >> static void migrate_fd_cleanup(void *opaque) >> >> { >> >> MigrationState *s = opaque; >> >> @@ -1508,12 +1531,7 @@ static void migrate_fd_cleanup(void *opaque) >> >> QEMUFile *tmp; >> >> >> >> trace_migrate_fd_cleanup(); >> >> - qemu_mutex_unlock_iothread(); >> >> - if (s->migration_thread_running) { >> >> - qemu_thread_join(&s->thread); >> >> - s->migration_thread_running = false; >> >> - } >> >> - qemu_mutex_lock_iothread(); >> >> + migrate_join_thread(s); >> >> >> >> multifd_save_cleanup(); >> >> qemu_mutex_lock(&s->qemu_file_lock); >> > >> > Dave >> > >> >> -- >> >> 2.21.0 >> > -- >> > Dr. David Alan Gilbert / [email protected] / Manchester, UK > -- > Dr. David Alan Gilbert / [email protected] / Manchester, UK
