[v3,16/22] fuzz: add fuzzer skeleton

tests/fuzz/fuzz.c serves as the entry point for the virtual-device
fuzzer. Namely, libfuzzer invokes the LLVMFuzzerInitialize and
LLVMFuzzerTestOneInput functions, both of which are defined in this
file. This change adds a "FuzzTarget" struct, along with the
fuzz_add_target function, which should be used to define new fuzz
targets.

Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
---
 tests/fuzz/Makefile.include |   4 +-
 tests/fuzz/fuzz.c           | 179 ++++++++++++++++++++++++++++++++++++
 tests/fuzz/fuzz.h           |  30 ++++++
 3 files changed, 211 insertions(+), 2 deletions(-)
 create mode 100644 tests/fuzz/fuzz.c
 create mode 100644 tests/fuzz/fuzz.h

On Wed, Sep 18, 2019 at 11:19:43PM +0000, Oleinik, Alexander wrote:
> +void set_fuzz_target_args(int argc, char **argv)
> +{
> +    if (fuzz_target) {
> +        fuzz_target->main_argc = argc;
> +        fuzz_target->main_argv = argv;
> +    }
> +}

Why calls this and why?

> +
> +void reboot(QTestState *s)
> +{
> +    qemu_system_reset(SHUTDOWN_CAUSE_GUEST_RESET);
> +}

Why does reboot() take an unused argument?

> +static void usage(char *path)
> +{
> +    printf("Usage: %s --FUZZ_TARGET [LIBFUZZER ARGUMENTS]\n", path);
> +    printf("where --FUZZ_TARGET is one of:\n");

Is the "--" prefix a libfuzzer requirement?  I would have expected
either FUZZ_TARGET by itself or --fuzz-target=FUZZ_TARGET (a properly
formatted long option) so that collisions with other command-line
options are not possible.

> +typedef struct FuzzTarget {
> +    GString *name;
> +    GString *description;
> +    void(*pre_main)(void);
> +    void(*pre_fuzz)(QTestState *);
> +    void(*fuzz)(QTestState *, const unsigned char *, size_t);
> +    int main_argc;
> +    char **main_argv;
> +} FuzzTarget;
> +
> +void set_fuzz_target_args(int argc, char **argv);
> +void reboot(QTestState *);
> +void fuzz_add_target(const char *name, const char *description, FuzzTarget
> +        *target);

This is a strange API.  I can't make sense of the struct,
fuzz_add_target(), and set_fuzz_target_args() without reading the
implementation:

1. set_fuzz_target_args() implies that there is global state but then
   FuzzTarget also has main_argc and main_argv fields.  I'm not sure
   what the relationship is.

2. fuzz_add_target() takes name and description as arguments but expects
   the caller to populate the struct arg's pre_main(), pre_fuzz(),
   fuzz() function pointers.  This is inconsistent and undocumented.

A cleaner API:

  /* Each fuzz target implements the following interface: */
  typedef struct {
      const char *name;        /* command-line option for this target */
      const char *description; /* human-readable help text */

      /* TODO documentation */
      void (*pre_main)(void);

      /* TODO documentation */
      void (*pre_fuzz)(QTestState *);

      /* TODO documentation */
      void (*fuzz)(QTestState *, const unsigned char *, size_t);
  } FuzzTarget;

  void fuzz_register_target(const FuzzTarget *target);

On Thu, 2019-09-19 at 13:48 +0100, Stefan Hajnoczi wrote:

> > +
> > +void reboot(QTestState *s)
> > +{
> > +    qemu_system_reset(SHUTDOWN_CAUSE_GUEST_RESET);
> > +}
> 
> Why does reboot() take an unused argument?
It was needed when I had a reset_state(s) pointer which was separate
from fuzz(). Since fuzz() is responsible for initializing and resetting
state now, I'll remove it.

> 
> > +static void usage(char *path)
> > +{
> > +    printf("Usage: %s --FUZZ_TARGET [LIBFUZZER ARGUMENTS]\n",
> > path);
> > +    printf("where --FUZZ_TARGET is one of:\n");
> 
> Is the "--" prefix a libfuzzer requirement?  I would have expected
> either FUZZ_TARGET by itself or --fuzz-target=FUZZ_TARGET (a properly
> formatted long option) so that collisions with other command-line
> options are not possible.
Yes libfuzzer will only pass arguments that start with "--". I can
replace it with --fuzz-target=FUZZ_TARGET. Alternatively, I can try to
build separate binaries for each target. It might waste disk space, but
we wouldn't need arguments (--trace could be replace with TRACE=1 in
ENV). With this design, I'm not sure what to do with code such as
i440fx_fuzz.c which re-purposes some functions for multiple different
fuzz targets.

> > +typedef struct FuzzTarget {
> > +    GString *name;
> > +    GString *description;
> > +    void(*pre_main)(void);
> > +    void(*pre_fuzz)(QTestState *);
> > +    void(*fuzz)(QTestState *, const unsigned char *, size_t);
> > +    int main_argc;
> > +    char **main_argv;
> > +} FuzzTarget;
> > +
> > +void set_fuzz_target_args(int argc, char **argv);
> > +void reboot(QTestState *);
> > +void fuzz_add_target(const char *name, const char *description,
> > FuzzTarget
> > +        *target);
> 
> This is a strange API.  I can't make sense of the struct,
> fuzz_add_target(), and set_fuzz_target_args() without reading the
> implementation:
> 
> 1. set_fuzz_target_args() implies that there is global state but then
>    FuzzTarget also has main_argc and main_argv fields.  I'm not sure
>    what the relationship is.
This is essentially there for the QOS support. For QOS targets, when
running fuzz_add_qos_target(), we don't yet know argc and argv, since
that requires walking the QOSGraph. When we have populated the
FuzzTargetList, QOSGraph and parsed the --FUZZ_TARGET, we set global
FuzzTarget and check against it while walking the QOSGraph. When we
find the matching path, we then know the argc/argv and can set them for
the global fuzz_target. Since qos_fuzz.c still needs major work due to
all of the duplicated code, I'll take another stab at this. Maybe we
can identify the argc/argv immediately when we add the fuzz target node
to the QOSGraph. This is another case for "each target has its own
binary", since that could avoid much of this complexity, since we
wouldn't need the fuzz_target_list.

> 2. fuzz_add_target() takes name and description as arguments but
> expects
>    the caller to populate the struct arg's pre_main(), pre_fuzz(),
>    fuzz() function pointers.  This is inconsistent and undocumented.
> 
> A cleaner API:
> 
>   /* Each fuzz target implements the following interface: */
>   typedef struct {
>       const char *name;        /* command-line option for this target
> */
>       const char *description; /* human-readable help text */
> 
>       /* TODO documentation */
>       void (*pre_main)(void);
> 
>       /* TODO documentation */
>       void (*pre_fuzz)(QTestState *);
> 
>       /* TODO documentation */
>       void (*fuzz)(QTestState *, const unsigned char *, size_t);
>   } FuzzTarget;

Sounds good. Should there also be argc and argv here? 

>   void fuzz_register_target(const FuzzTarget *target);

On Thu, Sep 19, 2019 at 01:49:09PM +0000, Oleinik, Alexander wrote:
> On Thu, 2019-09-19 at 13:48 +0100, Stefan Hajnoczi wrote:
> > > +static void usage(char *path)
> > > +{
> > > +    printf("Usage: %s --FUZZ_TARGET [LIBFUZZER ARGUMENTS]\n",
> > > path);
> > > +    printf("where --FUZZ_TARGET is one of:\n");
> > 
> > Is the "--" prefix a libfuzzer requirement?  I would have expected
> > either FUZZ_TARGET by itself or --fuzz-target=FUZZ_TARGET (a properly
> > formatted long option) so that collisions with other command-line
> > options are not possible.
> Yes libfuzzer will only pass arguments that start with "--". I can
> replace it with --fuzz-target=FUZZ_TARGET. Alternatively, I can try to
> build separate binaries for each target. It might waste disk space, but
> we wouldn't need arguments (--trace could be replace with TRACE=1 in
> ENV). With this design, I'm not sure what to do with code such as
> i440fx_fuzz.c which re-purposes some functions for multiple different
> fuzz targets.

Building a single fuzzing binary with all targets feels natural.  Please
support the --fuzz-target=TARGET syntax though.

> > A cleaner API:
> > 
> >   /* Each fuzz target implements the following interface: */
> >   typedef struct {
> >       const char *name;        /* command-line option for this target
> > */
> >       const char *description; /* human-readable help text */
> > 
> >       /* TODO documentation */
> >       void (*pre_main)(void);
> > 
> >       /* TODO documentation */
> >       void (*pre_fuzz)(QTestState *);
> > 
> >       /* TODO documentation */
> >       void (*fuzz)(QTestState *, const unsigned char *, size_t);
> >   } FuzzTarget;
> 
> Sounds good. Should there also be argc and argv here? 

If they are read-only and provided by the FuzzTarget, then yes.  The
reason I consider this "cleaner" is because the FuzzTarget struct is
stateless and just captures the information about the fuzz target
instead of mixing it with runtime state.  But like I said, I didn't
really understand the design of the struct so maybe I don't understand
the full problem :).

On Wed, Sep 18, 2019 at 11:19:43PM +0000, Oleinik, Alexander wrote:
>tests/fuzz/fuzz.c serves as the entry point for the virtual-device
>fuzzer. Namely, libfuzzer invokes the LLVMFuzzerInitialize and
>LLVMFuzzerTestOneInput functions, both of which are defined in this
>file. This change adds a "FuzzTarget" struct, along with the
>fuzz_add_target function, which should be used to define new fuzz
>targets.
>
>Signed-off-by: Alexander Oleinik <alxndr@bu.edu>
>---
> tests/fuzz/Makefile.include |   4 +-
> tests/fuzz/fuzz.c           | 179 ++++++++++++++++++++++++++++++++++++
> tests/fuzz/fuzz.h           |  30 ++++++
> 3 files changed, 211 insertions(+), 2 deletions(-)
> create mode 100644 tests/fuzz/fuzz.c
> create mode 100644 tests/fuzz/fuzz.h
>
>diff --git a/tests/fuzz/Makefile.include b/tests/fuzz/Makefile.include
>index 324e6c1433..b415b056b0 100644
>--- a/tests/fuzz/Makefile.include
>+++ b/tests/fuzz/Makefile.include
>@@ -1,4 +1,4 @@
>-# QEMU_PROG_FUZZ=qemu-fuzz-$(TARGET_NAME)$(EXESUF)
>+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
>diff --git a/tests/fuzz/fuzz.c b/tests/fuzz/fuzz.c
>new file mode 100644
>index 0000000000..833f436731
>--- /dev/null
>+++ b/tests/fuzz/fuzz.c
>@@ -0,0 +1,179 @@
>+#include "qemu/osdep.h"
>+
>+#include <stdio.h>
>+#include <stdlib.h>
>+
>+
>+#include "tests/libqtest.h"
>+#include "sysemu/qtest.h"
>+#include "fuzz.h"
>+#include "tests/libqos/qgraph.h"
>+#include "sysemu/runstate.h"
>+#include "sysemu/sysemu.h"
>+
>+typedef struct FuzzTargetState {
>+        FuzzTarget *target;
>+        QSLIST_ENTRY(FuzzTargetState) target_list;
>+} FuzzTargetState;
>+
>+typedef QSLIST_HEAD(, FuzzTargetState) FuzzTargetList;
>+
>+static const char *fuzz_arch = TARGET_NAME;
>+
>+static FuzzTargetList *fuzz_target_list;
>+static FuzzTarget *fuzz_target;
>+static QTestState *fuzz_qts;
>+static bool trace;
>+
>+
>+void set_fuzz_target_args(int argc, char **argv)
>+{
>+    if (fuzz_target) {
>+        fuzz_target->main_argc = argc;
>+        fuzz_target->main_argv = argv;
>+    }
>+}
>+
>+void reboot(QTestState *s)
>+{
>+    qemu_system_reset(SHUTDOWN_CAUSE_GUEST_RESET);
>+}
>+
>+static QTestState *qtest_setup(void)
>+{
>+    qtest_server_set_tx_handler(&qtest_client_inproc_recv, NULL);
>+    return qtest_inproc_init(trace, fuzz_arch, &qtest_server_inproc_recv);
>+}
>+
>+void fuzz_add_target(const char *name, const char *description,
>+        FuzzTarget *target)
>+{
>+    FuzzTargetState *tmp;
>+    FuzzTargetState *target_state;
>+    if (!fuzz_target_list) {
>+        fuzz_target_list = g_new0(FuzzTargetList, 1);
>+    }
>+
>+    QSLIST_FOREACH(tmp, fuzz_target_list, target_list) {
>+        if (g_strcmp0(tmp->target->name->str, name) == 0) {
>+            fprintf(stderr, "Error: Fuzz target name %s already in use\n",
>+                    name);
>+            abort();
>+        }
>+    }
>+    target_state = g_new0(FuzzTargetState, 1);
>+    target_state->target = g_new0(FuzzTarget, 1);
>+    *(target_state->target) = *target;
>+    target_state->target->name = g_string_new(name);
>+    target_state->target->description = g_string_new(description);
>+    QSLIST_INSERT_HEAD(fuzz_target_list, target_state, target_list);
>+}
>+
>+
>+static FuzzTarget *fuzz_get_target(char* name)
>+{
>+    FuzzTargetState *tmp;
>+    if (!fuzz_target_list) {
>+        fprintf(stderr, "Fuzz target list not initialized\n");
>+        abort();
>+    }
>+
>+    QSLIST_FOREACH(tmp, fuzz_target_list, target_list) {
>+        if (g_strcmp0(tmp->target->name->str, name) == 0) {
>+            break;
>+        }
>+    }
>+    return tmp->target;
>+}
>+
>+
>+static void usage(char *path)
>+{
>+    printf("Usage: %s --FUZZ_TARGET [LIBFUZZER ARGUMENTS]\n", path);
>+    printf("where --FUZZ_TARGET is one of:\n");
>+    FuzzTargetState *tmp;
>+    if (!fuzz_target_list) {
>+        fprintf(stderr, "Fuzz target list not initialized\n");
>+        abort();
>+    }
>+    QSLIST_FOREACH(tmp, fuzz_target_list, target_list) {
>+        printf(" --%s  : %s\n", tmp->target->name->str,
>+                tmp->target->description->str);
>+    }
>+    exit(0);
>+}
>+
>+
>+/* Executed for each fuzzing-input */
>+int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size)
>+{
>+    if (fuzz_target->fuzz) {
>+        fuzz_target->fuzz(fuzz_qts, Data, Size);
>+    }
>+    return 0;
>+}
>+
>+/* Executed once, prior to fuzzing */
>+int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp)
>+{
>+
>+    char *target_name, *trace_qtest;
>+
>+    /* --trace is useful for outputting a log of qtest commands that trigger
>+     * a crash. The log can can then be replayed with a simple qtest script. */
>+    if (*argc > 2) {
>+        trace_qtest = (*argv)[2];
>+        if (strcmp(trace_qtest, "--trace") == 0) {
>+            trace = true;
>+        }
>+    }
>+
>+    /* Initialize qgraph and modules */
>+    qos_graph_init();
>+    module_call_init(MODULE_INIT_FUZZ_TARGET);
>+    module_call_init(MODULE_INIT_QOM);
>+    module_call_init(MODULE_INIT_LIBQOS);
>+
>+    if (*argc <= 1) {
>+        usage(**argv);
>+    }
>+
>+    /* Identify the fuzz target */
>+    target_name = (*argv)[1];
>+    target_name += 2;
>+    fuzz_target = fuzz_get_target(target_name);
>+
>+    fuzz_qts = qtest_setup(void);

Shouldn't be passing void as an argument here, causes a build
failure for me.

Thanks,

Darren.