| Message ID | 20190918231846.22538-17-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: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.
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); + + if (!fuzz_target) { + fprintf(stderr, "Error: Fuzz fuzz_target name %s not found\n", + target_name); + usage(**argv); + } + + if (fuzz_target->pre_main) { + fuzz_target->pre_main(); + } + + if (trace) { + printf("### cmd_line: "); + for (int i = 0; i < (fuzz_target->main_argc); i++) { + printf("%s ", ((fuzz_target->main_argv))[i]); + } + printf("\n"); + } + + /* Run QEMU's softmmu main with the calculated arguments*/ + qemu_init(fuzz_target->main_argc, fuzz_target->main_argv, NULL); + + /* If configured, this is a good spot to set up snapshotting */ + if (fuzz_target->pre_fuzz) { + fuzz_target->pre_fuzz(fuzz_qts); + } + + if (trace) { + printf("### END INITIALIZATION\n"); + } + + return 0; +} diff --git a/tests/fuzz/fuzz.h b/tests/fuzz/fuzz.h new file mode 100644 index 0000000000..73af029c82 --- /dev/null +++ b/tests/fuzz/fuzz.h @@ -0,0 +1,30 @@ +#ifndef FUZZER_H_ +#define FUZZER_H_ + +#include "qemu/osdep.h" +#include "qemu/units.h" +#include "qapi/error.h" +#include "exec/memory.h" +#include "tests/libqtest.h" + + +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); + +int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size); +int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp); + +#endif +
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