Re: [PATCH v3 17/22] fuzz: add support for fork-based fuzzing.

From: Alexander Oleinik <alxndr@bu.edu>
To: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Cc: "pbonzini@redhat.com" <pbonzini@redhat.com>,
	"bsd@redhat.com" <bsd@redhat.com>,
	"stefanha@redhat.com" <stefanha@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [PATCH v3 17/22] fuzz: add support for fork-based fuzzing.
Date: Mon, 30 Sep 2019 11:17:59 -0400	[thread overview]
Message-ID: <b8d441a3-dc3b-8c91-7516-3c8162cc5a4d@bu.edu> (raw)
In-Reply-To: <20190918231846.22538-18-alxndr@bu.edu>

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