| Message ID | 20190918231846.22538-18-alxndr@bu.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add virtual device fuzzing support | expand |
On Wed, Sep 18, 2019 at 11:19:44PM +0000, Oleinik, Alexander wrote: > diff --git a/exec.c b/exec.c > index 235d6bc883..d3838f4ea4 100644 > --- a/exec.c > +++ b/exec.c > @@ -2295,7 +2295,9 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared) > qemu_ram_setup_dump(new_block->host, new_block->max_length); > qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE); > /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU */ > +#ifndef CONFIG_FUZZ /* This conflicts with fork-based fuzzing */ > qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_DONTFORK); > +#endif > ram_block_notify_add(new_block->host, new_block->max_length); > } > } I didn't check Makefile changes but regular softmmu binaries should continue to work with --enable-fuzzing so we cannot use #ifdef here. Perhaps this should be a runtime check similar to qtest_enabled(): /* The fuzzer's fork child requires access to guest RAM */ if (!fuzz_enabled()) { qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_DONTFORK); }
On Thu, 2019-09-19 at 13:54 +0100, Stefan Hajnoczi wrote: > On Wed, Sep 18, 2019 at 11:19:44PM +0000, Oleinik, Alexander wrote: > > diff --git a/exec.c b/exec.c > > index 235d6bc883..d3838f4ea4 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -2295,7 +2295,9 @@ static void ram_block_add(RAMBlock > > *new_block, Error **errp, bool shared) > > qemu_ram_setup_dump(new_block->host, new_block- > > >max_length); > > qemu_madvise(new_block->host, new_block->max_length, > > QEMU_MADV_HUGEPAGE); > > /* MADV_DONTFORK is also needed by KVM in absence of > > synchronous MMU */ > > +#ifndef CONFIG_FUZZ /* This conflicts with fork-based fuzzing */ > > qemu_madvise(new_block->host, new_block->max_length, > > QEMU_MADV_DONTFORK); > > +#endif > > ram_block_notify_add(new_block->host, new_block- > > >max_length); > > } > > } > > I didn't check Makefile changes but regular softmmu binaries should > continue to work with --enable-fuzzing so we cannot use #ifdef here. > Perhaps this should be a runtime check similar to qtest_enabled() Yes - I'll add a runtime check. The makefile requires a make clean between softmmu and fuzzer builds, since the ".o"s for the fuzzer build are compiled with fsanitize=fuzzer(must be linked against libfuzzer). I can see that the #ifndef CONFIG_FUZZ is a proper softmmu builds with --enable-fuzzer are currently broken due to the ifndef CONFIG_FUZZ check.
On Thu, Sep 19, 2019 at 02:01:53PM +0000, Oleinik, Alexander wrote: > On Thu, 2019-09-19 at 13:54 +0100, Stefan Hajnoczi wrote: > > On Wed, Sep 18, 2019 at 11:19:44PM +0000, Oleinik, Alexander wrote: > > > diff --git a/exec.c b/exec.c > > > index 235d6bc883..d3838f4ea4 100644 > > > --- a/exec.c > > > +++ b/exec.c > > > @@ -2295,7 +2295,9 @@ static void ram_block_add(RAMBlock > > > *new_block, Error **errp, bool shared) > > > qemu_ram_setup_dump(new_block->host, new_block- > > > >max_length); > > > qemu_madvise(new_block->host, new_block->max_length, > > > QEMU_MADV_HUGEPAGE); > > > /* MADV_DONTFORK is also needed by KVM in absence of > > > synchronous MMU */ > > > +#ifndef CONFIG_FUZZ /* This conflicts with fork-based fuzzing */ > > > qemu_madvise(new_block->host, new_block->max_length, > > > QEMU_MADV_DONTFORK); > > > +#endif > > > ram_block_notify_add(new_block->host, new_block- > > > >max_length); > > > } > > > } > > > > I didn't check Makefile changes but regular softmmu binaries should > > continue to work with --enable-fuzzing so we cannot use #ifdef here. > > Perhaps this should be a runtime check similar to qtest_enabled() > > Yes - I'll add a runtime check. The makefile requires a make clean > between softmmu and fuzzer builds, since the ".o"s for the fuzzer build > are compiled with fsanitize=fuzzer(must be linked against libfuzzer). > I can see that the #ifndef CONFIG_FUZZ is a proper softmmu builds with > --enable-fuzzer are currently broken due to the ifndef CONFIG_FUZZ > check. In that case the fuzzer is a whole different make target and shouldn't share .o files with *-softmmu/ build directories. Stefan
On 9/18/19 7:19 PM, Oleinik, Alexander wrote: > fork() is a simple way to ensure that state does not leak in between > fuzzing runs. Unfortunately, the fuzzer mutation engine relies on > bitmaps which contain coverage information for each fuzzing run, and > these bitmaps should be copied from the child to the parent(where the > mutation occurs). These bitmaps are created through compile-time > instrumentation and there seems to be no simple way to re-map them as > shared memory. As a workaround, we use a linker script modification to > place all of the bitmaps together and add some markers around them which > we can observe from our code. Then, we map shared memory and copy the > bimaps to the SHM (in the child) and out of the SHM(in the parent) after > each fuzzing run. Ram blocks are marked as DONTFORK in exec.c, which > breaks this approach. For now, avoid this with an #ifdef. > > Signed-off-by: Alexander Oleinik <alxndr@bu.edu> > --- > exec.c | 2 ++ > tests/fuzz/Makefile.include | 3 +++ > tests/fuzz/fork_fuzz.c | 27 ++++++++++++++++++++++ > tests/fuzz/fork_fuzz.h | 12 ++++++++++ > tests/fuzz/fork_fuzz.ld | 46 +++++++++++++++++++++++++++++++++++++ > 5 files changed, 90 insertions(+) > create mode 100644 tests/fuzz/fork_fuzz.c > create mode 100644 tests/fuzz/fork_fuzz.h > create mode 100644 tests/fuzz/fork_fuzz.ld > > diff --git a/exec.c b/exec.c > index 235d6bc883..d3838f4ea4 100644 > --- a/exec.c > +++ b/exec.c > @@ -2295,7 +2295,9 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared) > qemu_ram_setup_dump(new_block->host, new_block->max_length); > qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE); > /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU */ > +#ifndef CONFIG_FUZZ /* This conflicts with fork-based fuzzing */ > qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_DONTFORK); > +#endif > ram_block_notify_add(new_block->host, new_block->max_length); > } > } > diff --git a/tests/fuzz/Makefile.include b/tests/fuzz/Makefile.include > index b415b056b0..687dacce04 100644 > --- a/tests/fuzz/Makefile.include > +++ b/tests/fuzz/Makefile.include > @@ -2,3 +2,6 @@ QEMU_PROG_FUZZ=qemu-fuzz-$(TARGET_NAME)$(EXESUF) > fuzz-obj-y = $(libqos-obj-y) > fuzz-obj-y += tests/libqtest.o > fuzz-obj-y += tests/fuzz/fuzz.o > +fuzz-obj-y += tests/fuzz/fork_fuzz.o > + > +FUZZ_LDFLAGS += -Xlinker -T$(SRC_PATH)/tests/fuzz/fork_fuzz.ld > diff --git a/tests/fuzz/fork_fuzz.c b/tests/fuzz/fork_fuzz.c > new file mode 100644 > index 0000000000..26d0b4b42e > --- /dev/null > +++ b/tests/fuzz/fork_fuzz.c > @@ -0,0 +1,27 @@ > +#include "qemu/osdep.h" > +#include "fork_fuzz.h" > + > +uintptr_t feature_shm; > + > +void counter_shm_init(void) > +{ > + feature_shm = (uintptr_t)mmap(NULL, > + &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START, > + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); > + return; > +} > + > +void counter_shm_store(void) > +{ > + memcpy((void *)feature_shm, > + &__FUZZ_COUNTERS_START, > + &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START); > +} > + > +void counter_shm_load(void) > +{ > + memcpy(&__FUZZ_COUNTERS_START, > + (void *)feature_shm, > + &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START); > +} > + > diff --git a/tests/fuzz/fork_fuzz.h b/tests/fuzz/fork_fuzz.h > new file mode 100644 > index 0000000000..b5f8b35015 > --- /dev/null > +++ b/tests/fuzz/fork_fuzz.h > @@ -0,0 +1,12 @@ > +#ifndef FORK_FUZZ_H > +#define FORK_FUZZ_H > + > +extern uint8_t __FUZZ_COUNTERS_START; > +extern uint8_t __FUZZ_COUNTERS_END; > + > +void counter_shm_init(void); > +void counter_shm_store(void); > +void counter_shm_load(void); > + > +#endif > + > diff --git a/tests/fuzz/fork_fuzz.ld b/tests/fuzz/fork_fuzz.ld > new file mode 100644 > index 0000000000..ba0ba79570 > --- /dev/null > +++ b/tests/fuzz/fork_fuzz.ld > @@ -0,0 +1,46 @@ > +/* We adjust linker script modification to place all of the stuff that needs to > + * persist across fuzzing runs into a contiguous seciton of memory. Then, it is > + * easy to copy it to and from shared memory. > + * > + * Total Size : A5A00 > + * Sancov counters: B26F > + * Coverage counters: 56D60 > + * TracePC Object: 43C00 > +*/ > + > +SECTIONS > +{ > + .data.fuzz_start : ALIGN(4K) > + { > + __FUZZ_COUNTERS_START = .; > + } > + .data.fuzz_ordered : > + { > + /* Internal Libfuzzer TracePC object which contains the ValueProfileMap. > + * Not optimal that we have to copy the rest of the TracePC object. > + * */ > + __start___sancov_cntrs = .; > + *(__sancov_cntrs*) > + __stop___sancov_cntrs = .; > + } > + .data.fuzz_unordered : > + { > + /* Coverage counters. They're not necessary for fuzzing, but are useful > + * for analyzing the fuzzing performance > + * */ > + __start___llvm_prf_cnts = .; > + *(*llvm_prf_cnts); > + __stop___llvm_prf_cnts = .; > + > + /* Lowest stack counter */ > + *(__sancov_lowest_stack); > + /* Internal Libfuzzer TracePC object which contains the ValueProfileMap. > + * Not optimal that we have to copy the rest of the TracePC object. > + * */ > + *FuzzerTracePC*(.bss._ZN6fuzzer*) > + __FUZZ_COUNTERS_END = .; > + } > +} > +/* Dont overwrite the SECTIONS in the default linker script. Instead insert the > + * above into the default script */ > +INSERT AFTER .data; > The obvious issue with this are that we lose the threads created prior to fork(). Over the past days I've been coverage of existing virtio-net tests with and without fork(). With fork(), virtio_net_tx_bh doesn't get scheduled, and never runs. Could this be due to the missing RCU thread? This is probably an even bigger problem for iothread devices, such as virtio-blk... Does anyone have suggestions for how to approach the thread problem, or at least the RCU part of it? I know Paolo made some fork-related changes in 21b7cf9e07e5991c57b461181cfb5bbb6fe7a9d6 rcu: handle forks safely and 2a96a552f9502ac34c29da2f3a39788db5ee5692 , but from what I can tell, this is mostly for qemu-user. -Alex
diff --git a/exec.c b/exec.c index 235d6bc883..d3838f4ea4 100644 --- a/exec.c +++ b/exec.c @@ -2295,7 +2295,9 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared) qemu_ram_setup_dump(new_block->host, new_block->max_length); qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_HUGEPAGE); /* MADV_DONTFORK is also needed by KVM in absence of synchronous MMU */ +#ifndef CONFIG_FUZZ /* This conflicts with fork-based fuzzing */ qemu_madvise(new_block->host, new_block->max_length, QEMU_MADV_DONTFORK); +#endif ram_block_notify_add(new_block->host, new_block->max_length); } } diff --git a/tests/fuzz/Makefile.include b/tests/fuzz/Makefile.include index b415b056b0..687dacce04 100644 --- a/tests/fuzz/Makefile.include +++ b/tests/fuzz/Makefile.include @@ -2,3 +2,6 @@ QEMU_PROG_FUZZ=qemu-fuzz-$(TARGET_NAME)$(EXESUF) fuzz-obj-y = $(libqos-obj-y) fuzz-obj-y += tests/libqtest.o fuzz-obj-y += tests/fuzz/fuzz.o +fuzz-obj-y += tests/fuzz/fork_fuzz.o + +FUZZ_LDFLAGS += -Xlinker -T$(SRC_PATH)/tests/fuzz/fork_fuzz.ld diff --git a/tests/fuzz/fork_fuzz.c b/tests/fuzz/fork_fuzz.c new file mode 100644 index 0000000000..26d0b4b42e --- /dev/null +++ b/tests/fuzz/fork_fuzz.c @@ -0,0 +1,27 @@ +#include "qemu/osdep.h" +#include "fork_fuzz.h" + +uintptr_t feature_shm; + +void counter_shm_init(void) +{ + feature_shm = (uintptr_t)mmap(NULL, + &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START, + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); + return; +} + +void counter_shm_store(void) +{ + memcpy((void *)feature_shm, + &__FUZZ_COUNTERS_START, + &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START); +} + +void counter_shm_load(void) +{ + memcpy(&__FUZZ_COUNTERS_START, + (void *)feature_shm, + &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START); +} + diff --git a/tests/fuzz/fork_fuzz.h b/tests/fuzz/fork_fuzz.h new file mode 100644 index 0000000000..b5f8b35015 --- /dev/null +++ b/tests/fuzz/fork_fuzz.h @@ -0,0 +1,12 @@ +#ifndef FORK_FUZZ_H +#define FORK_FUZZ_H + +extern uint8_t __FUZZ_COUNTERS_START; +extern uint8_t __FUZZ_COUNTERS_END; + +void counter_shm_init(void); +void counter_shm_store(void); +void counter_shm_load(void); + +#endif + diff --git a/tests/fuzz/fork_fuzz.ld b/tests/fuzz/fork_fuzz.ld new file mode 100644 index 0000000000..ba0ba79570 --- /dev/null +++ b/tests/fuzz/fork_fuzz.ld @@ -0,0 +1,46 @@ +/* We adjust linker script modification to place all of the stuff that needs to + * persist across fuzzing runs into a contiguous seciton of memory. Then, it is + * easy to copy it to and from shared memory. + * + * Total Size : A5A00 + * Sancov counters: B26F + * Coverage counters: 56D60 + * TracePC Object: 43C00 +*/ + +SECTIONS +{ + .data.fuzz_start : ALIGN(4K) + { + __FUZZ_COUNTERS_START = .; + } + .data.fuzz_ordered : + { + /* Internal Libfuzzer TracePC object which contains the ValueProfileMap. + * Not optimal that we have to copy the rest of the TracePC object. + * */ + __start___sancov_cntrs = .; + *(__sancov_cntrs*) + __stop___sancov_cntrs = .; + } + .data.fuzz_unordered : + { + /* Coverage counters. They're not necessary for fuzzing, but are useful + * for analyzing the fuzzing performance + * */ + __start___llvm_prf_cnts = .; + *(*llvm_prf_cnts); + __stop___llvm_prf_cnts = .; + + /* Lowest stack counter */ + *(__sancov_lowest_stack); + /* Internal Libfuzzer TracePC object which contains the ValueProfileMap. + * Not optimal that we have to copy the rest of the TracePC object. + * */ + *FuzzerTracePC*(.bss._ZN6fuzzer*) + __FUZZ_COUNTERS_END = .; + } +} +/* Dont overwrite the SECTIONS in the default linker script. Instead insert the + * above into the default script */ +INSERT AFTER .data;
fork() is a simple way to ensure that state does not leak in between fuzzing runs. Unfortunately, the fuzzer mutation engine relies on bitmaps which contain coverage information for each fuzzing run, and these bitmaps should be copied from the child to the parent(where the mutation occurs). These bitmaps are created through compile-time instrumentation and there seems to be no simple way to re-map them as shared memory. As a workaround, we use a linker script modification to place all of the bitmaps together and add some markers around them which we can observe from our code. Then, we map shared memory and copy the bimaps to the SHM (in the child) and out of the SHM(in the parent) after each fuzzing run. Ram blocks are marked as DONTFORK in exec.c, which breaks this approach. For now, avoid this with an #ifdef. Signed-off-by: Alexander Oleinik <alxndr@bu.edu> --- exec.c | 2 ++ tests/fuzz/Makefile.include | 3 +++ tests/fuzz/fork_fuzz.c | 27 ++++++++++++++++++++++ tests/fuzz/fork_fuzz.h | 12 ++++++++++ tests/fuzz/fork_fuzz.ld | 46 +++++++++++++++++++++++++++++++++++++ 5 files changed, 90 insertions(+) create mode 100644 tests/fuzz/fork_fuzz.c create mode 100644 tests/fuzz/fork_fuzz.h create mode 100644 tests/fuzz/fork_fuzz.ld