[v3,20/22] fuzz: add i440fx fuzz targets

Commit Message

Alexander Bulekov Sept. 18, 2019, 11:19 p.m. UTC

These three targets should simply fuzz reads/writes to a couple ioports,
but they mostly serve as examples of different ways to write targets.
They demonstrate using qtest and qos for fuzzing, as well as using
rebooting and forking to reset state, or not resetting it at all.

Signed-off-by: Alexander Oleinik <[e-mail]>
---
 tests/fuzz/Makefile.include |   3 +
 tests/fuzz/i440fx_fuzz.c    | 158 ++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+)
 create mode 100644 tests/fuzz/i440fx_fuzz.c

Comments

Stefan Hajnoczi Sept. 19, 2019, 1:08 p.m. UTC | #1

On Wed, Sep 18, 2019 at 11:19:47PM +0000, Oleinik, Alexander wrote:
> +static void i440fx_fuzz_qtest(QTestState *s,
> +        const unsigned char *Data, size_t Size) {
> +
> +    typedef struct QTestFuzzAction {
> +        uint8_t id;
> +        uint8_t addr;
> +        uint32_t value;
> +    } QTestFuzzAction;

I'm concerned about padding within the struct and struct alignment
causing us to skip some bytes in Data[].  Also, on some architectures
unaligned memory accesses are unsupported so memcpy() is safer than
casting a pointer directly into Data[].

The question is whether skipping bytes in Data[] matters.
Feedback-directed fuzzing should realize that certain offsets in Data[]
are unused and do not affect the input space.  Still, I think we should
arrange things so that every bit in Data[] matters as much as possible.

The struct can be arranged to avoid struct field padding:

  uint32_t value;
  uint8_t id;
  uint8_t addr;

> +    QTestFuzzAction *a = (QTestFuzzAction *)Data;
> +    while (Size >= sizeof(QTestFuzzAction)) {

To avoid struct alignment issues:

  QTestFuzzAction a;
  while (Size >= sizeof(a)) {
      memcpy(&a, Data, sizeof(a));
      ...
      Data += sizeof(a);
  }

> +        uint16_t addr = a->addr % 2 ? 0xcf8 : 0xcfc;

Please define constants for these magic numbers (e.g.
I440FX_PCI_HOST_BRIDGE_CFG, I440FX_PCI_HOST_BRIDGE_DATA).

> +        switch (a->id) {

How about:

  switch (a->id % ACTION_MAX) {

This way we always exercise a useful code path instead of just skipping
the input.

Patch

diff --git a/tests/fuzz/Makefile.include b/tests/fuzz/Makefile.include
index 687dacce04..37d6821bee 100644
--- a/tests/fuzz/Makefile.include
+++ b/tests/fuzz/Makefile.include
@@ -3,5 +3,8 @@  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-obj-y += tests/fuzz/qos_fuzz.o
+
+fuzz-obj-y += tests/fuzz/i440fx_fuzz.o
 
 FUZZ_LDFLAGS += -Xlinker -T$(SRC_PATH)/tests/fuzz/fork_fuzz.ld
diff --git a/tests/fuzz/i440fx_fuzz.c b/tests/fuzz/i440fx_fuzz.c
new file mode 100644
index 0000000000..9079c40f55
--- /dev/null
+++ b/tests/fuzz/i440fx_fuzz.c
@@ -0,0 +1,158 @@ 
+#include "qemu/osdep.h"
+
+#include "fuzz.h"
+#include "tests/libqtest.h"
+#include "fuzz/qos_fuzz.h"
+#include "fuzz/fork_fuzz.h"
+#include "qemu/main-loop.h"
+#include "tests/libqos/pci.h"
+#include "tests/libqos/pci-pc.h"
+
+enum action_id {
+    WRITEB,
+    WRITEW,
+    WRITEL,
+    READB,
+    READW,
+    READL,
+};
+
+static void i440fx_fuzz_qtest(QTestState *s,
+        const unsigned char *Data, size_t Size) {
+
+    typedef struct QTestFuzzAction {
+        uint8_t id;
+        uint8_t addr;
+        uint32_t value;
+    } QTestFuzzAction;
+    QTestFuzzAction *a = (QTestFuzzAction *)Data;
+    while (Size >= sizeof(QTestFuzzAction)) {
+        uint16_t addr = a->addr % 2 ? 0xcf8 : 0xcfc;
+        switch (a->id) {
+        case WRITEB:
+            qtest_outb(s, addr, (uint8_t)a->value);
+            break;
+        case WRITEW:
+            qtest_outw(s, addr, (uint16_t)a->value);
+            break;
+        case WRITEL:
+            qtest_outl(s, addr, (uint32_t)a->value);
+            break;
+        case READB:
+            qtest_inb(s, addr);
+            break;
+        case READW:
+            qtest_inw(s, addr);
+            break;
+        case READL:
+            qtest_inl(s, addr);
+            break;
+        }
+        a++;
+        Size -= sizeof(QTestFuzzAction);
+    }
+    qtest_clock_step_next(s);
+    main_loop_wait(true);
+    reboot(s);
+}
+
+static void i440fx_fuzz_qos(QTestState *s,
+        const unsigned char *Data, size_t Size) {
+
+    typedef struct QOSFuzzAction {
+        uint8_t id;
+        int devfn;
+        uint8_t offset;
+        uint32_t value;
+    } QOSFuzzAction;
+
+    QOSFuzzAction *a = (QOSFuzzAction *)Data;
+    static QPCIBus *bus;
+    if (!bus) {
+        bus = qpci_new_pc(s, fuzz_qos_alloc);
+    }
+
+    while (Size >= sizeof(QOSFuzzAction)) {
+        switch (a->id) {
+        case WRITEB:
+            bus->config_writeb(bus, a->devfn, a->offset, (uint8_t)a->value);
+            break;
+        case WRITEW:
+            bus->config_writew(bus, a->devfn, a->offset, (uint16_t)a->value);
+            break;
+        case WRITEL:
+            bus->config_writel(bus, a->devfn, a->offset, (uint32_t)a->value);
+            break;
+        case READB:
+            bus->config_readb(bus, a->devfn, a->offset);
+            break;
+        case READW:
+            bus->config_readw(bus, a->devfn, a->offset);
+            break;
+        case READL:
+            bus->config_readl(bus, a->devfn, a->offset);
+            break;
+        }
+        a++;
+        Size -= sizeof(QOSFuzzAction);
+    }
+    qtest_clock_step_next(s);
+    main_loop_wait(true);
+}
+
+static void i440fx_fuzz_qos_fork(QTestState *s,
+        const unsigned char *Data, size_t Size) {
+    if (fork() == 0) {
+        i440fx_fuzz_qos(s, Data, Size);
+        counter_shm_store();
+        _Exit(0);
+    } else {
+        wait(NULL);
+        counter_shm_load();
+    }
+}
+
+static void fork_init(QTestState *s)
+{
+    counter_shm_init();
+}
+static const char *i440fx_qtest_argv[] = {"qemu_system_i386", "-machine", "accel=qtest"};
+
+static void register_pci_fuzz_targets(void)
+{
+    /* Uses simple qtest commands and reboots to reset state */
+    fuzz_add_target("i440fx-qtest-reboot-fuzz",
+            "Fuzz the i440fx using raw qtest commands and rebooting"
+            "after each run",
+            &(FuzzTarget){
+                .fuzz = i440fx_fuzz_qtest,
+                .main_argc = 3,
+                .main_argv = (char **)i440fx_qtest_argv,
+                });
+
+    /* Uses libqos and forks to prevent state leakage */
+    fuzz_add_qos_target("i440fx-qos-fork-fuzz",
+            "Fuzz the i440fx using qos helpers and forking"
+            "for each run",
+            "i440FX-pcihost",
+            &(QOSGraphTestOptions){},
+            &(FuzzTarget){
+            .pre_main = &qos_setup,
+            .pre_fuzz = &fork_init,
+            .fuzz = &i440fx_fuzz_qos_fork
+            });
+
+    /* Uses libqos. Doesn't do anything to reset state. Note that if we were to
+     reboot after each run, we would also have to redo the qos-related
+     initialization (qos_init_path) */
+    fuzz_add_qos_target("i440fx-qos-nocleanup-fuzz",
+            "Fuzz the i440fx using qos helpers. No cleanup done after each run",
+            "i440FX-pcihost",
+            &(QOSGraphTestOptions){},
+            &(FuzzTarget){
+            .pre_main = &qos_setup,
+            .fuzz = &i440fx_fuzz_qos
+            });
+}
+
+fuzz_target_init(register_pci_fuzz_targets);