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

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

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