* [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit
@ 2019-04-08 11:33 ` Yury Kotov
0 siblings, 0 replies; 7+ messages in thread
From: Yury Kotov @ 2019-04-08 11:33 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Juan Quintela
Cc: Alex Bennée, Evgeny Yakovlev, qemu-devel
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);
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 <yury-kotov@yandex-team.ru>
---
migration/migration.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 609e0df5d0..b9848d1030 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1495,10 +1495,8 @@ static void block_cleanup_parameters(MigrationState *s)
}
}
-static void migrate_fd_cleanup(void *opaque)
+static void migrate_fd_cleanup(MigrationState *s)
{
- MigrationState *s = opaque;
-
qemu_bh_delete(s->cleanup_bh);
s->cleanup_bh = NULL;
@@ -1543,6 +1541,21 @@ static void migrate_fd_cleanup(void *opaque)
block_cleanup_parameters(s);
}
+static void migrate_fd_cleanup_schedule(MigrationState *s)
+{
+ /* Ref the state for bh, because it may be called when
+ * there're already no other refs */
+ object_ref(OBJECT(s));
+ qemu_bh_schedule(s->cleanup_bh);
+}
+
+static void migrate_fd_cleanup_bh(void *opaque)
+{
+ MigrationState *s = opaque;
+ migrate_fd_cleanup(s);
+ object_unref(OBJECT(s));
+}
+
void migrate_set_error(MigrationState *s, const Error *error)
{
qemu_mutex_lock(&s->error_mutex);
@@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState *s)
error_report("%s: Unknown ending state %d", __func__, s->state);
break;
}
- qemu_bh_schedule(s->cleanup_bh);
+ migrate_fd_cleanup_schedule(s);
qemu_mutex_unlock_iothread();
}
@@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
s->expected_downtime = s->parameters.downtime_limit;
- s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
+ s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
if (error_in) {
migrate_fd_error(s, error_in);
migrate_fd_cleanup(s);
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit
@ 2019-04-08 11:33 ` Yury Kotov
0 siblings, 0 replies; 7+ messages in thread
From: Yury Kotov @ 2019-04-08 11:33 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Juan Quintela
Cc: Alex Bennée, qemu-devel, Evgeny Yakovlev
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);
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 <yury-kotov@yandex-team.ru>
---
migration/migration.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 609e0df5d0..b9848d1030 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1495,10 +1495,8 @@ static void block_cleanup_parameters(MigrationState *s)
}
}
-static void migrate_fd_cleanup(void *opaque)
+static void migrate_fd_cleanup(MigrationState *s)
{
- MigrationState *s = opaque;
-
qemu_bh_delete(s->cleanup_bh);
s->cleanup_bh = NULL;
@@ -1543,6 +1541,21 @@ static void migrate_fd_cleanup(void *opaque)
block_cleanup_parameters(s);
}
+static void migrate_fd_cleanup_schedule(MigrationState *s)
+{
+ /* Ref the state for bh, because it may be called when
+ * there're already no other refs */
+ object_ref(OBJECT(s));
+ qemu_bh_schedule(s->cleanup_bh);
+}
+
+static void migrate_fd_cleanup_bh(void *opaque)
+{
+ MigrationState *s = opaque;
+ migrate_fd_cleanup(s);
+ object_unref(OBJECT(s));
+}
+
void migrate_set_error(MigrationState *s, const Error *error)
{
qemu_mutex_lock(&s->error_mutex);
@@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState *s)
error_report("%s: Unknown ending state %d", __func__, s->state);
break;
}
- qemu_bh_schedule(s->cleanup_bh);
+ migrate_fd_cleanup_schedule(s);
qemu_mutex_unlock_iothread();
}
@@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
s->expected_downtime = s->parameters.downtime_limit;
- s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
+ s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
if (error_in) {
migrate_fd_error(s, error_in);
migrate_fd_cleanup(s);
--
2.21.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit
2019-04-08 11:33 ` Yury Kotov
(?)
@ 2019-04-17 12:43 ` Yury Kotov
2019-05-14 9:40 ` Yury Kotov
-1 siblings, 1 reply; 7+ messages in thread
From: Yury Kotov @ 2019-04-17 12:43 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Juan Quintela
Cc: Alex Bennée, qemu-devel,
yc-core
(рассылка)
Ping
08.04.2019, 14:34, "Yury Kotov" <yury-kotov@yandex-team.ru>:
> 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);
>
> 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 <yury-kotov@yandex-team.ru>
> ---
> migration/migration.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 609e0df5d0..b9848d1030 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1495,10 +1495,8 @@ static void block_cleanup_parameters(MigrationState *s)
> }
> }
>
> -static void migrate_fd_cleanup(void *opaque)
> +static void migrate_fd_cleanup(MigrationState *s)
> {
> - MigrationState *s = opaque;
> -
> qemu_bh_delete(s->cleanup_bh);
> s->cleanup_bh = NULL;
>
> @@ -1543,6 +1541,21 @@ static void migrate_fd_cleanup(void *opaque)
> block_cleanup_parameters(s);
> }
>
> +static void migrate_fd_cleanup_schedule(MigrationState *s)
> +{
> + /* Ref the state for bh, because it may be called when
> + * there're already no other refs */
> + object_ref(OBJECT(s));
> + qemu_bh_schedule(s->cleanup_bh);
> +}
> +
> +static void migrate_fd_cleanup_bh(void *opaque)
> +{
> + MigrationState *s = opaque;
> + migrate_fd_cleanup(s);
> + object_unref(OBJECT(s));
> +}
> +
> void migrate_set_error(MigrationState *s, const Error *error)
> {
> qemu_mutex_lock(&s->error_mutex);
> @@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState *s)
> error_report("%s: Unknown ending state %d", __func__, s->state);
> break;
> }
> - qemu_bh_schedule(s->cleanup_bh);
> + migrate_fd_cleanup_schedule(s);
> qemu_mutex_unlock_iothread();
> }
>
> @@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>
> s->expected_downtime = s->parameters.downtime_limit;
> - s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
> + s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
> if (error_in) {
> migrate_fd_error(s, error_in);
> migrate_fd_cleanup(s);
> --
> 2.21.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit
2019-04-17 12:43 ` Yury Kotov
@ 2019-05-14 9:40 ` Yury Kotov
0 siblings, 0 replies; 7+ messages in thread
From: Yury Kotov @ 2019-05-14 9:40 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Juan Quintela
Cc: Alex Bennée, qemu-devel,
yc-core
(рассылка)
Ping ping
17.04.2019, 15:44, "Yury Kotov" <yury-kotov@yandex-team.ru>:
> Ping
>
> 08.04.2019, 14:34, "Yury Kotov" <yury-kotov@yandex-team.ru>:
>> 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);
>>
>> 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 <yury-kotov@yandex-team.ru>
>> ---
>> migration/migration.c | 23 ++++++++++++++++++-----
>> 1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 609e0df5d0..b9848d1030 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1495,10 +1495,8 @@ static void block_cleanup_parameters(MigrationState *s)
>> }
>> }
>>
>> -static void migrate_fd_cleanup(void *opaque)
>> +static void migrate_fd_cleanup(MigrationState *s)
>> {
>> - MigrationState *s = opaque;
>> -
>> qemu_bh_delete(s->cleanup_bh);
>> s->cleanup_bh = NULL;
>>
>> @@ -1543,6 +1541,21 @@ static void migrate_fd_cleanup(void *opaque)
>> block_cleanup_parameters(s);
>> }
>>
>> +static void migrate_fd_cleanup_schedule(MigrationState *s)
>> +{
>> + /* Ref the state for bh, because it may be called when
>> + * there're already no other refs */
>> + object_ref(OBJECT(s));
>> + qemu_bh_schedule(s->cleanup_bh);
>> +}
>> +
>> +static void migrate_fd_cleanup_bh(void *opaque)
>> +{
>> + MigrationState *s = opaque;
>> + migrate_fd_cleanup(s);
>> + object_unref(OBJECT(s));
>> +}
>> +
>> void migrate_set_error(MigrationState *s, const Error *error)
>> {
>> qemu_mutex_lock(&s->error_mutex);
>> @@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState *s)
>> error_report("%s: Unknown ending state %d", __func__, s->state);
>> break;
>> }
>> - qemu_bh_schedule(s->cleanup_bh);
>> + migrate_fd_cleanup_schedule(s);
>> qemu_mutex_unlock_iothread();
>> }
>>
>> @@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>>
>> s->expected_downtime = s->parameters.downtime_limit;
>> - s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
>> + s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
>> if (error_in) {
>> migrate_fd_error(s, error_in);
>> migrate_fd_cleanup(s);
>> --
>> 2.21.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit
2019-04-08 11:33 ` Yury Kotov
(?)
(?)
@ 2019-05-14 16:04 ` Dr. David Alan Gilbert
-1 siblings, 0 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2019-05-14 16:04 UTC (permalink / raw)
To: Yury Kotov
Cc: qemu-devel, Alex Bennée, Evgeny Yakovlev, stefanha, Juan Quintela
* Yury Kotov (yury-kotov@yandex-team.ru) wrote:
> It fixes heap-use-after-free which was found by clang's ASAN.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
and queued.
(cc'ing in Stefan since aio crashes often get to him).
> 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);
>
> 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 <yury-kotov@yandex-team.ru>
> ---
> migration/migration.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 609e0df5d0..b9848d1030 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1495,10 +1495,8 @@ static void block_cleanup_parameters(MigrationState *s)
> }
> }
>
> -static void migrate_fd_cleanup(void *opaque)
> +static void migrate_fd_cleanup(MigrationState *s)
> {
> - MigrationState *s = opaque;
> -
> qemu_bh_delete(s->cleanup_bh);
> s->cleanup_bh = NULL;
>
> @@ -1543,6 +1541,21 @@ static void migrate_fd_cleanup(void *opaque)
> block_cleanup_parameters(s);
> }
>
> +static void migrate_fd_cleanup_schedule(MigrationState *s)
> +{
> + /* Ref the state for bh, because it may be called when
> + * there're already no other refs */
> + object_ref(OBJECT(s));
> + qemu_bh_schedule(s->cleanup_bh);
> +}
> +
> +static void migrate_fd_cleanup_bh(void *opaque)
> +{
> + MigrationState *s = opaque;
> + migrate_fd_cleanup(s);
> + object_unref(OBJECT(s));
> +}
> +
> void migrate_set_error(MigrationState *s, const Error *error)
> {
> qemu_mutex_lock(&s->error_mutex);
> @@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState *s)
> error_report("%s: Unknown ending state %d", __func__, s->state);
> break;
> }
> - qemu_bh_schedule(s->cleanup_bh);
> + migrate_fd_cleanup_schedule(s);
> qemu_mutex_unlock_iothread();
> }
>
> @@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>
> s->expected_downtime = s->parameters.downtime_limit;
> - s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
> + s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
> if (error_in) {
> migrate_fd_error(s, error_in);
> migrate_fd_cleanup(s);
> --
> 2.21.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit
2019-04-08 11:33 ` Yury Kotov
` (2 preceding siblings ...)
(?)
@ 2019-09-13 13:43 ` Vladimir Sementsov-Ogievskiy
2019-09-13 14:42 ` Yury Kotov
-1 siblings, 1 reply; 7+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-09-13 13:43 UTC (permalink / raw)
To: Yury Kotov, Dr. David Alan Gilbert, Juan Quintela
Cc: Alex Bennée, qemu-devel, Evgeny Yakovlev
Hi!
08.04.2019 14:33, Yury Kotov 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
>
[..]
>
> +static void migrate_fd_cleanup_schedule(MigrationState *s)
> +{
> + /* Ref the state for bh, because it may be called when
> + * there're already no other refs */
> + object_ref(OBJECT(s));
> + qemu_bh_schedule(s->cleanup_bh);
> +}
so you ref on scheduling
> +
> +static void migrate_fd_cleanup_bh(void *opaque)
> +{
> + MigrationState *s = opaque;
> + migrate_fd_cleanup(s);
> + object_unref(OBJECT(s));
> +}
and unref after execution of bh.
but migration code has also call to qemu_bh_delete(s->cleanup_bh) from
migrate_fd_cleanup(), in migrate_fd_cleanup() is called not only from
migrate_fd_cleanup_bh..
I mean, that if we call qemu_bh_delete after scheduling, we will not
unref our new reference.
Still, seems it's impossible, as all other calls to migrate_fd_cleanup
are done before migration thread creation.
Don't know, should something done around it or not, but decided to mention it.
> +
> void migrate_set_error(MigrationState *s, const Error *error)
> {
> qemu_mutex_lock(&s->error_mutex);
> @@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState *s)
> error_report("%s: Unknown ending state %d", __func__, s->state);
> break;
> }
> - qemu_bh_schedule(s->cleanup_bh);
> + migrate_fd_cleanup_schedule(s);
> qemu_mutex_unlock_iothread();
> }
>
> @@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
> bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>
> s->expected_downtime = s->parameters.downtime_limit;
> - s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
> + s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
> if (error_in) {
> migrate_fd_error(s, error_in);
> migrate_fd_cleanup(s);
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit
2019-09-13 13:43 ` Vladimir Sementsov-Ogievskiy
@ 2019-09-13 14:42 ` Yury Kotov
0 siblings, 0 replies; 7+ messages in thread
From: Yury Kotov @ 2019-09-13 14:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, Dr. David Alan Gilbert, Juan Quintela
Cc: Alex Bennée, qemu-devel, Evgeny Yakovlev
Hi Vladimir!
13.09.2019, 16:43, "Vladimir Sementsov-Ogievskiy" <vsementsov@virtuozzo.com>:
> Hi!
>
> 08.04.2019 14:33, Yury Kotov 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
>
> [..]
>
>> +static void migrate_fd_cleanup_schedule(MigrationState *s)
>> +{
>> + /* Ref the state for bh, because it may be called when
>> + * there're already no other refs */
>> + object_ref(OBJECT(s));
>> + qemu_bh_schedule(s->cleanup_bh);
>> +}
>
> so you ref on scheduling
>
>> +
>> +static void migrate_fd_cleanup_bh(void *opaque)
>> +{
>> + MigrationState *s = opaque;
>> + migrate_fd_cleanup(s);
>> + object_unref(OBJECT(s));
>> +}
>
> and unref after execution of bh.
>
> but migration code has also call to qemu_bh_delete(s->cleanup_bh) from
> migrate_fd_cleanup(), in migrate_fd_cleanup() is called not only from
> migrate_fd_cleanup_bh..
>
> I mean, that if we call qemu_bh_delete after scheduling, we will not
> unref our new reference.
>
> Still, seems it's impossible, as all other calls to migrate_fd_cleanup
> are done before migration thread creation.
>
> Don't know, should something done around it or not, but decided to mention it.
>
Hmm.. It's very interesting, thanks! I need more time to understand
is there a problem or not, but after quick look I see one possibility
to achieve a leak: qmp_migrate after cleanup bh schedule and before bh call...
>> +
>> void migrate_set_error(MigrationState *s, const Error *error)
>> {
>> qemu_mutex_lock(&s->error_mutex);
>> @@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState *s)
>> error_report("%s: Unknown ending state %d", __func__, s->state);
>> break;
>> }
>> - qemu_bh_schedule(s->cleanup_bh);
>> + migrate_fd_cleanup_schedule(s);
>> qemu_mutex_unlock_iothread();
>> }
>>
>> @@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>>
>> s->expected_downtime = s->parameters.downtime_limit;
>> - s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
>> + s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s);
>> if (error_in) {
>> migrate_fd_error(s, error_in);
>> migrate_fd_cleanup(s);
>
> --
> Best regards,
> Vladimir
Regards,
Yury
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-09-13 14:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-08 11:33 [Qemu-devel] [PATCH] migration: Fix use-after-free during process exit Yury Kotov
2019-04-08 11:33 ` Yury Kotov
2019-04-17 12:43 ` Yury Kotov
2019-05-14 9:40 ` Yury Kotov
2019-05-14 16:04 ` Dr. David Alan Gilbert
2019-09-13 13:43 ` Vladimir Sementsov-Ogievskiy
2019-09-13 14:42 ` Yury Kotov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.