[PATCH v2 00/15] Add a General Virtual Device Fuzzer

[PATCH v2 00/15] Add a General Virtual Device Fuzzer

[Alexander Bulekov]

v2:
	- Remove QOS dependency.
	- Add a custom crossover function
	- Fix broken minimization scripts
	- Fixes to the IO region and DMA handling code

This is a general virtual-device fuzzer, designed to fuzz devices over Port IO,
MMIO, and DMA.

To get started with this:
 1. Build the fuzzers (see docs/devel/fuzzing.txt)
    Note: Build with --enable-sanitizers, or create a "dictionary file":
    echo kw1=\"FUZZ\" > dict
    and pass it as an argument to libFuzzer with -dict=./dict
    This magic value is a command separator that lets the fuzzer perform
    multiple IO actions with a single input.

 2. Pick the qemu arguments you wish to fuzz:
    export QEMU_FUZZ_ARGS="-M q35 -device virtio-balloon"

 3. Tell the fuzzer which QOM objects or MemoryRegion names to fuzz. I find the
 "info qom-tree", "info qtree" and "info mtree" commands useful for identifying
 these. Supports globbing. Here I will try to simultaneously fuzz(for no good
 reason) virtio-balloon and e1000e, which is included by default in the q35:
    export QEMU_FUZZ_OBJECTS='virtio* e1000*'
    You can also try to fuzz the whole machine:
    export QEMU_FUZZ_OBJECTS='*'

 4. Run the fuzzer for 0 inputs. The fuzzer should output a list of
 MemoryRegions/PCI Devices it will try to fuzz. Confirm that these match your
 expectations.
    ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=general-fuzz -runs=0

 5. Run the fuzzer:
    ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=general-fuzz 


Basically, at the core, this fuzzer is an interpreter that splits the input
into a series of commands, such as mmio_write, pio_write, etc. We structure
these commands to hit only MemoryRegions that are associated with the devices
specified in QEMU_FUZZ_OBJECTS. Additionally, these patches add "hooks" to
functions that are typically used by virtual-devices to read from RAM (DMA).
These hooks attempt to populate these DMA regions with fuzzed data, just in
time.

Some of the issues I have found or reproduced with this fuzzer:
https://bugs.launchpad.net/bugs/1525123
https://bugs.launchpad.net/bugs/1681439
https://bugs.launchpad.net/bugs/1777315
https://bugs.launchpad.net/bugs/1878034
https://bugs.launchpad.net/bugs/1878043
https://bugs.launchpad.net/bugs/1878054
https://bugs.launchpad.net/bugs/1878057
https://bugs.launchpad.net/bugs/1878067
https://bugs.launchpad.net/bugs/1878134
https://bugs.launchpad.net/bugs/1878136
https://bugs.launchpad.net/bugs/1878253
https://bugs.launchpad.net/bugs/1878255
https://bugs.launchpad.net/bugs/1878259
https://bugs.launchpad.net/bugs/1878263
https://bugs.launchpad.net/bugs/1878323
https://bugs.launchpad.net/bugs/1878641
https://bugs.launchpad.net/bugs/1878642
https://bugs.launchpad.net/bugs/1878645
https://bugs.launchpad.net/bugs/1878651
https://bugs.launchpad.net/bugs/1879223
https://bugs.launchpad.net/bugs/1879227
https://bugs.launchpad.net/bugs/1879531
https://bugs.launchpad.net/bugs/1880355
https://bugs.launchpad.net/bugs/1880539
https://bugs.launchpad.net/bugs/1884693
https://bugs.launchpad.net/bugs/1886362
https://bugs.launchpad.net/bugs/1887303
https://bugs.launchpad.net/bugs/1887309
https://bugs.launchpad.net/bugs/697510

*** BLURB HERE ***

Alexander Bulekov (15):
  fuzz: Change the way we write qtest log to stderr
  fuzz: Add general virtual-device fuzzer
  fuzz: Add PCI features to the general fuzzer
  fuzz: Add DMA support to the generic-fuzzer
  fuzz: Declare DMA Read callback function
  fuzz: Add fuzzer callbacks to DMA-read functions
  fuzz: Add support for custom crossover functions
  fuzz: add a DISABLE_PCI op to general-fuzzer
  fuzz: add a crossover function to generic-fuzzer
  scripts/oss-fuzz: Add wrapper program for generic fuzzer
  scripts/oss-fuzz: Add general-fuzzer build script
  scripts/oss-fuzz: Add general-fuzzer configs for oss-fuzz
  scripts/oss-fuzz: build the general-fuzzer configs
  scripts/oss-fuzz: Add script to reorder a general-fuzzer trace
  scripts/oss-fuzz: Add crash trace minimization script

 exec.c                                        |   2 +
 include/exec/memory.h                         |  16 +
 include/exec/memory_ldst_cached.inc.h         |   3 +
 memory_ldst.inc.c                             |   4 +
 scripts/oss-fuzz/build.sh                     |   8 +-
 scripts/oss-fuzz/build_general_fuzzers.py     |  62 ++
 scripts/oss-fuzz/general_fuzzer_configs.yml   | 103 +++
 scripts/oss-fuzz/minimize_qtest_trace.py      | 118 +++
 .../oss-fuzz/reorder_fuzzer_qtest_trace.py    |  94 ++
 scripts/oss-fuzz/target.c                     |  40 +
 softmmu/memory.c                              |  14 +
 tests/qtest/fuzz/Makefile.include             |   1 +
 tests/qtest/fuzz/fuzz.c                       |  18 +-
 tests/qtest/fuzz/fuzz.h                       |  26 +
 tests/qtest/fuzz/general_fuzz.c               | 843 ++++++++++++++++++
 15 files changed, 1348 insertions(+), 4 deletions(-)
 create mode 100755 scripts/oss-fuzz/build_general_fuzzers.py
 create mode 100644 scripts/oss-fuzz/general_fuzzer_configs.yml
 create mode 100755 scripts/oss-fuzz/minimize_qtest_trace.py
 create mode 100755 scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
 create mode 100644 scripts/oss-fuzz/target.c
 create mode 100644 tests/qtest/fuzz/general_fuzz.c

-- 
2.27.0

[PATCH v2 01/15] fuzz: Change the way we write qtest log to stderr

[Alexander Bulekov]

Telling QTest to log to /dev/fd/2, essentially results in dup(2). This
is fine, if other code isn't logging to stderr. Otherwise, the order of
the logs is mixed due to buffering issues, since two file-descriptors
are used to write to the same file. We can avoid this, since just
specifying "-qtest" sets the log fd to stderr. If we want to disable
qtest logs, we can just add -qtest-log none.

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/fuzz.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index 031594a686..8234b68754 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -202,9 +202,8 @@ int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp)
 
     /* Run QEMU's softmmu main with the fuzz-target dependent arguments */
     GString *cmd_line = fuzz_target->get_init_cmdline(fuzz_target);
-    g_string_append_printf(cmd_line,
-                           " -qtest /dev/null -qtest-log %s",
-                           getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null");
+    g_string_append_printf(cmd_line, " %s -qtest /dev/null ",
+                           getenv("QTEST_LOG") ? "" : "-qtest-log none");
 
     /* Split the runcmd into an argv and argc */
     wordexp_t result;
-- 
2.27.0

[PATCH v2 02/15] fuzz: Add general virtual-device fuzzer

[Alexander Bulekov]

This is a generic fuzzer designed to fuzz a virtual device's
MemoryRegions, as long as they exist within the Memory or Port IO (if it
exists) AddressSpaces. The fuzzer's input is interpreted into a sequence
of qtest commands (outb, readw, etc). The interpreted commands are
separated by a magic seaparator, which should be easy for the fuzzer to
guess. Without ASan, the separator can be specified as a "dictionary
value" using the -dict argument (see libFuzzer documentation).

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/Makefile.include |   1 +
 tests/qtest/fuzz/general_fuzz.c   | 494 ++++++++++++++++++++++++++++++
 2 files changed, 495 insertions(+)
 create mode 100644 tests/qtest/fuzz/general_fuzz.c

diff --git a/tests/qtest/fuzz/Makefile.include b/tests/qtest/fuzz/Makefile.include
index 5bde793bf2..854322efb6 100644
--- a/tests/qtest/fuzz/Makefile.include
+++ b/tests/qtest/fuzz/Makefile.include
@@ -11,6 +11,7 @@ fuzz-obj-y += tests/qtest/fuzz/qtest_wrappers.o
 fuzz-obj-$(CONFIG_PCI_I440FX) += tests/qtest/fuzz/i440fx_fuzz.o
 fuzz-obj-$(CONFIG_VIRTIO_NET) += tests/qtest/fuzz/virtio_net_fuzz.o
 fuzz-obj-$(CONFIG_SCSI) += tests/qtest/fuzz/virtio_scsi_fuzz.o
+fuzz-obj-y += tests/qtest/fuzz/general_fuzz.o
 
 FUZZ_CFLAGS += -I$(SRC_PATH)/tests -I$(SRC_PATH)/tests/qtest
 
diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
new file mode 100644
index 0000000000..01bcb029b1
--- /dev/null
+++ b/tests/qtest/fuzz/general_fuzz.c
@@ -0,0 +1,494 @@
+/*
+ * General Virtual-Device Fuzzing Target
+ *
+ * Copyright Red Hat Inc., 2020
+ *
+ * Authors:
+ *  Alexander Bulekov   <alxndr@bu.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+
+#include <wordexp.h>
+
+#include "cpu.h"
+#include "tests/qtest/libqtest.h"
+#include "fuzz.h"
+#include "fork_fuzz.h"
+#include "exec/address-spaces.h"
+#include "string.h"
+#include "exec/memory.h"
+#include "exec/ramblock.h"
+#include "exec/address-spaces.h"
+#include "hw/qdev-core.h"
+
+/*
+ * SEPARATOR is used to separate "operations" in the fuzz input
+ */
+#define SEPARATOR "FUZZ"
+
+enum cmds{
+    OP_IN,
+    OP_OUT,
+    OP_READ,
+    OP_WRITE,
+    OP_CLOCK_STEP,
+};
+
+#define DEFAULT_TIMEOUT_US 100000
+#define USEC_IN_SEC 100000000
+
+typedef struct {
+    size_t addr;
+    size_t len; /* The number of bytes until the end of the I/O region */
+} address_range;
+
+static useconds_t timeout = 100000;
+/*
+ * List of memory regions that are children of QOM objects specified by the
+ * user for fuzzing.
+ */
+static GPtrArray *fuzzable_memoryregions;
+/*
+ * Here we want to convert a fuzzer-provided [io-region-index, offset] to
+ * a physical address. To do this, we iterate over all of the matched
+ * MemoryRegions. Check whether each region exists within the particular io
+ * space. Return the absolute address of the offset within the index'th region
+ * that is a subregion of the io_space and the distance until the end of the
+ * memory region.
+ */
+static bool get_io_address(address_range *result,
+                                    MemoryRegion *io_space,
+                                    uint8_t index,
+                                    uint32_t offset) {
+    MemoryRegion *mr, *root;
+    index = index % fuzzable_memoryregions->len;
+    int candidate_regions = 0;
+    int i = 0;
+    int ind = index;
+    size_t abs_addr;
+    AddressSpace *as = (io_space == get_system_memory()) ? &address_space_memory : &address_space_io;
+
+    while (ind >= 0 && fuzzable_memoryregions->len) {
+        *result = (address_range){0, 0};
+        mr = g_ptr_array_index(fuzzable_memoryregions, i);
+        if (mr->enabled) {
+            abs_addr = mr->addr;
+            for (root = mr; root->container; ) {
+                root = root->container;
+                abs_addr += root->addr;
+            }
+            /*
+             * Only consider the region if it is rooted at the io_space we want
+             */
+            if (root == io_space) {
+                hwaddr xlat, len;
+                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){
+                    ind--;
+                    candidate_regions++;
+                    result->addr = abs_addr;
+                    if(mr->size){
+                        result->addr += (offset % mr->size);
+                    }
+                    result->len = mr->size-(result->addr-abs_addr);
+                }
+            }
+        }
+        ++i;
+        /* Loop around */
+        if (i == fuzzable_memoryregions->len) {
+            /* No enabled regions in our io_space? */
+            if (candidate_regions == 0) {
+                break;
+            }
+            i = 0;
+        }
+    }
+    return candidate_regions != 0;
+}
+static bool get_pio_address(address_range *result,
+                                     uint8_t index, uint16_t offset)
+{
+    /*
+     * PIO BARs can be set past the maximum port address (0xFFFF). Thus, result
+     * can contain an addr that extends past the PIO space. When we pass this
+     * address to qtest_in/qtest_out, it is cast to a uint16_t, so we might end
+     * up fuzzing a completely different MemoryRegion/Device. Therefore, check
+     * that the address here is within the PIO space limits.
+     */
+
+    bool success = get_io_address(result, get_system_io(), index, offset);
+    return success && result->addr <= 0xFFFF;
+}
+static bool get_mmio_address(address_range *result,
+                                      uint8_t index, uint32_t offset)
+{
+    return get_io_address(result, get_system_memory(), index, offset);
+}
+
+static void op_in(QTestState *s, const unsigned char * data, size_t len)
+{
+    enum Sizes {Byte, Word, Long, end_sizes};
+    struct {
+        uint8_t size;
+        uint8_t base;
+        uint16_t offset;
+    } a;
+    address_range abs;
+
+    if (len < sizeof(a)) {
+        return;
+    }
+    memcpy(&a, data, sizeof(a));
+    if (get_pio_address(&abs, a.base, a.offset) == 0) {
+        return;
+    }
+
+    switch (a.size %= end_sizes) {
+    case Byte:
+        qtest_inb(s, abs.addr);
+        break;
+    case Word:
+        if (abs.len >= 2) {
+            qtest_inw(s, abs.addr);
+        }
+        break;
+    case Long:
+        if (abs.len >= 4) {
+            qtest_inl(s, abs.addr);
+        }
+        break;
+    }
+}
+
+static void op_out(QTestState *s, const unsigned char * data, size_t len)
+{
+    enum Sizes {Byte, Word, Long, end_sizes};
+    struct {
+        uint8_t size;
+        uint8_t base;
+        uint16_t offset;
+        uint32_t value;
+    } a;
+    address_range abs;
+
+    if (len < sizeof(a)) {
+        return;
+    }
+    memcpy(&a, data, sizeof(a));
+
+    if (get_pio_address(&abs, a.base, a.offset) == 0) {
+        return;
+    }
+
+    switch (a.size %= end_sizes) {
+    case Byte:
+        qtest_outb(s, abs.addr, a.value & 0xFF);
+        break;
+    case Word:
+        if (abs.len >= 2) {
+            qtest_outw(s, abs.addr, a.value & 0xFFFF);
+        }
+        break;
+    case Long:
+        if (abs.len >= 4) {
+            qtest_outl(s, abs.addr, a.value);
+        }
+        break;
+    }
+}
+
+static void op_read(QTestState *s, const unsigned char * data, size_t len)
+{
+    enum Sizes {Byte, Word, Long, Quad, end_sizes};
+    struct {
+        uint8_t size;
+        uint8_t base;
+        uint32_t offset;
+    } a;
+    address_range abs;
+
+    if (len < sizeof(a)) {
+        return;
+    }
+    memcpy(&a, data, sizeof(a));
+
+    if (get_mmio_address(&abs, a.base, a.offset) == 0) {
+        return;
+    }
+
+    switch (a.size %= end_sizes) {
+    case Byte:
+        qtest_readb(s, abs.addr);
+        break;
+    case Word:
+        if (abs.len >= 2) {
+            qtest_readw(s, abs.addr);
+        }
+        break;
+    case Long:
+        if (abs.len >= 4) {
+            qtest_readl(s, abs.addr);
+        }
+        break;
+    case Quad:
+        if (abs.len >= 8) {
+            qtest_readq(s, abs.addr);
+        }
+        break;
+    }
+}
+
+static void op_write(QTestState *s, const unsigned char * data, size_t len)
+{
+    enum Sizes {Byte, Word, Long, Quad, end_sizes};
+    struct {
+        uint8_t size;
+        uint8_t base;
+        uint32_t offset;
+        uint64_t value;
+    } a;
+    address_range abs;
+
+    if (len < sizeof(a)) {
+        return;
+    }
+    memcpy(&a, data, sizeof(a));
+
+    if (get_mmio_address(&abs, a.base, a.offset) == 0) {
+        return;
+    }
+
+    switch (a.size %= end_sizes) {
+    case Byte:
+            qtest_writeb(s, abs.addr, a.value & 0xFF);
+        break;
+    case Word:
+        if (abs.len >= 2) {
+            qtest_writew(s, abs.addr, a.value & 0xFFFF);
+        }
+        break;
+    case Long:
+        if (abs.len >= 4) {
+            qtest_writel(s, abs.addr, a.value & 0xFFFFFFFF);
+        }
+        break;
+    case Quad:
+        if (abs.len >= 8) {
+            qtest_writeq(s, abs.addr, a.value);
+        }
+        break;
+    }
+}
+static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
+{
+    qtest_clock_step_next(s);
+}
+
+static void handle_timeout(int sig)
+{
+    if (getenv("QTEST_LOG")) {
+        fprintf(stderr, "[Timeout]\n");
+        fflush(stderr);
+    }
+    _Exit(0);
+}
+
+/*
+ * Here, we interpret random bytes from the fuzzer, as a sequence of commands.
+ * Our commands are variable-width, so we use a separator, SEPARATOR, to specify
+ * the boundaries between commands. This is just a random 32-bit value, which
+ * is easily identified by libfuzzer+AddressSanitizer, as long as we use
+ * memmem. It can also be included in the fuzzer's dictionary. More details
+ * here:
+ * https://github.com/google/fuzzing/blob/master/docs/split-inputs.md
+ *
+ * As a result, the stream of bytes is converted into a sequence of commands.
+ * In a simplified example where SEPARATOR is 0xFF:
+ * 00 01 02 FF 03 04 05 06 FF 01 FF ...
+ * becomes this sequence of commands:
+ * 00 01 02    -> op00 (0102)   -> in (0102, 2)
+ * 03 04 05 06 -> op03 (040506) -> write (040506, 3)
+ * 01          -> op01 (-,0)    -> out (-,0)
+ * ...
+ *
+ * Note here that it is the job of the individual opcode functions to check
+ * that enough data was provided. I.e. in the last command out (,0), out needs
+ * to check that there is not enough data provided to select an address/value
+ * for the operation.
+ */
+static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
+{
+    void (*ops[]) (QTestState *s, const unsigned char* , size_t) = {
+        [OP_IN]                 = op_in,
+        [OP_OUT]                = op_out,
+        [OP_READ]               = op_read,
+        [OP_WRITE]              = op_write,
+        [OP_CLOCK_STEP]         = op_clock_step,
+    };
+    const unsigned char *cmd = Data;
+    const unsigned char *nextcmd;
+    size_t cmd_len;
+    uint8_t op;
+
+    if (fork() == 0) {
+        /*
+         * Sometimes the fuzzer will find inputs that take quite a long time to
+         * process. Often times, these inputs do not result in new coverage.
+         * Even if these inputs might be interesting, they can slow down the
+         * fuzzer, overall. Set a timeout to avoid hurting performance, too much
+         */
+        if (timeout) {
+            struct sigaction sact;
+            struct itimerval timer;
+
+            sigemptyset(&sact.sa_mask);
+            sact.sa_flags   = SA_NODEFER;
+            sact.sa_handler = handle_timeout;
+            sigaction(SIGALRM, &sact, NULL);
+
+            memset(&timer, 0, sizeof(timer));
+            timer.it_value.tv_sec = timeout / USEC_IN_SEC;
+            timer.it_value.tv_usec = timeout % USEC_IN_SEC;
+            setitimer(ITIMER_VIRTUAL, &timer, NULL);
+        }
+
+        while (cmd && Size) {
+            /* Get the length until the next command or end of input */
+            nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
+            cmd_len = nextcmd ? nextcmd - cmd : Size;
+
+            if (cmd_len > 0) {
+                /* Interpret the first byte of the command as an opcode */
+                op = *cmd % (sizeof(ops) / sizeof((ops)[0]));
+                ops[op](s, cmd + 1, cmd_len - 1);
+
+                /* Run the main loop */
+                flush_events(s);
+            }
+            /* Advance to the next command */
+            cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
+            Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
+        }
+        _Exit(0);
+    } else {
+        flush_events(s);
+        wait(0);
+    }
+}
+
+static void usage(void)
+{
+    printf("Please specify the following environment variables:\n");
+    printf("QEMU_FUZZ_ARGS= the command line arguments passed to qemu\n");
+    printf("QEMU_FUZZ_OBJECTS= "
+            "a space separated list of QOM type names for objects to fuzz\n");
+    printf("Optionally: QEMU_FUZZ_TIMEOUT= Specify a custom timeout (us). "
+            "0 to disable. %d by default\n", timeout);
+    exit(0);
+}
+
+static int locate_fuzz_memory_regions(Object *child, void *opaque)
+{
+    const char *name;
+    MemoryRegion *mr;
+    if (object_dynamic_cast(child, TYPE_MEMORY_REGION)) {
+        mr = MEMORY_REGION(child);
+        if ((memory_region_is_ram(mr) ||
+            memory_region_is_ram_device(mr) ||
+            memory_region_is_rom(mr) ||
+            memory_region_is_romd(mr)) == false) {
+            name = object_get_canonical_path_component(child);
+            /*
+             * We don't want duplicate pointers to the same MemoryRegion, so
+             * try to remove copies of the pointer, before adding it.
+             */
+            g_ptr_array_remove_fast(fuzzable_memoryregions, mr);
+            g_ptr_array_add(fuzzable_memoryregions, mr);
+        }
+    }
+    return 0;
+}
+static int locate_fuzz_objects(Object *child, void *opaque)
+{
+    char *pattern = opaque;
+    if (g_pattern_match_simple(pattern, object_get_typename(child))) {
+        /* Find and save ptrs to any child MemoryRegions */
+        object_child_foreach_recursive(child, locate_fuzz_memory_regions, NULL);
+    } else if (object_dynamic_cast(OBJECT(child), TYPE_MEMORY_REGION)) {
+        if (g_pattern_match_simple(pattern,
+            object_get_canonical_path_component(child))) {
+            MemoryRegion *mr;
+            mr = MEMORY_REGION(child);
+            if ((memory_region_is_ram(mr) ||
+                 memory_region_is_ram_device(mr) ||
+                 memory_region_is_rom(mr) ||
+                 memory_region_is_romd(mr)) == false) {
+                g_ptr_array_remove_fast(fuzzable_memoryregions, mr);
+                g_ptr_array_add(fuzzable_memoryregions, mr);
+            }
+        }
+    }
+    return 0;
+}
+
+static void general_pre_fuzz(QTestState *s)
+{
+    if (!getenv("QEMU_FUZZ_OBJECTS")) {
+        usage();
+    }
+    if (getenv("QEMU_FUZZ_TIMEOUT")) {
+        timeout = g_ascii_strtoll(getenv("QEMU_FUZZ_TIMEOUT"), NULL, 0);
+    }
+
+    fuzzable_memoryregions = g_ptr_array_new();
+    char **result = g_strsplit (getenv("QEMU_FUZZ_OBJECTS"), " ", -1);
+    for (int i = 0; result[i] != NULL; i++) {
+        printf("Matching objects by name %s\n", result[i]);
+        object_child_foreach_recursive(qdev_get_machine(),
+                                    locate_fuzz_objects,
+                                    result[i]);
+    }
+    g_strfreev(result);
+    printf("This process will try to fuzz the following MemoryRegions:\n");
+    for (int i = 0; i < fuzzable_memoryregions->len; i++) {
+        MemoryRegion *mr;
+        mr = g_ptr_array_index(fuzzable_memoryregions, i);
+        printf("  * %s (size %lx)\n",
+               object_get_canonical_path_component(&(mr->parent_obj)),
+               mr->addr);
+    }
+
+    if(!fuzzable_memoryregions->len){
+        printf("No fuzzable memory regions found...\n");
+        exit(0);
+    }
+
+    counter_shm_init();
+}
+static GString *general_fuzz_cmdline(FuzzTarget *t)
+{
+    GString *cmd_line = g_string_new(TARGET_NAME);
+    if (!getenv("QEMU_FUZZ_ARGS")) {
+        usage();
+    }
+    g_string_append_printf(cmd_line, " -display none \
+                                      -machine accel=qtest, \
+                                      -m 64 %s ", getenv("QEMU_FUZZ_ARGS"));
+    return cmd_line;
+}
+
+static void register_general_fuzz_targets(void)
+{
+    fuzz_add_target(&(FuzzTarget){
+            .name = "general-fuzz",
+            .description = "Fuzz based on any qemu command-line args. ",
+            .get_init_cmdline = general_fuzz_cmdline,
+            .pre_fuzz = general_pre_fuzz,
+            .fuzz = general_fuzz});
+}
+
+fuzz_target_init(register_general_fuzz_targets);
-- 
2.27.0

[PATCH v2 03/15] fuzz: Add PCI features to the general fuzzer

[Alexander Bulekov]

This patch compares TYPE_PCI_DEVICE objects against the user-provided
matching pattern. If there is a match, we use some hacks and leverage
QOS to map each possible BAR for that device. Now fuzzed inputs might be
converted to pci_read/write commands which target specific. This means
that we can fuzz a particular device's PCI configuration space,

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/general_fuzz.c | 83 +++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
index 01bcb029b1..17b572a439 100644
--- a/tests/qtest/fuzz/general_fuzz.c
+++ b/tests/qtest/fuzz/general_fuzz.c
@@ -24,6 +24,7 @@
 #include "exec/ramblock.h"
 #include "exec/address-spaces.h"
 #include "hw/qdev-core.h"
+#include "hw/pci/pci.h"
 
 /*
  * SEPARATOR is used to separate "operations" in the fuzz input
@@ -35,12 +36,17 @@ enum cmds{
     OP_OUT,
     OP_READ,
     OP_WRITE,
+    OP_PCI_READ,
+    OP_PCI_WRITE,
     OP_CLOCK_STEP,
 };
 
 #define DEFAULT_TIMEOUT_US 100000
 #define USEC_IN_SEC 100000000
 
+#define PCI_HOST_BRIDGE_CFG 0xcf8
+#define PCI_HOST_BRIDGE_DATA 0xcfc
+
 typedef struct {
     size_t addr;
     size_t len; /* The number of bytes until the end of the I/O region */
@@ -52,6 +58,8 @@ static useconds_t timeout = 100000;
  * user for fuzzing.
  */
 static GPtrArray *fuzzable_memoryregions;
+static GPtrArray *fuzzable_pci_devices;
+
 /*
  * Here we want to convert a fuzzer-provided [io-region-index, offset] to
  * a physical address. To do this, we iterate over all of the matched
@@ -283,6 +291,65 @@ static void op_write(QTestState *s, const unsigned char * data, size_t len)
         break;
     }
 }
+static void op_pci_read(QTestState *s, const unsigned char * data, size_t len)
+{
+    enum Sizes {Byte, Word, Long, end_sizes};
+    struct {
+        uint8_t size;
+        uint8_t base;
+        uint8_t offset;
+    } a;
+    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
+        return;
+    }
+    memcpy(&a, data, sizeof(a));
+    PCIDevice *dev = g_ptr_array_index(fuzzable_pci_devices,
+                                  a.base % fuzzable_pci_devices->len);
+    int devfn = dev->devfn;
+    qtest_outl(s, PCI_HOST_BRIDGE_CFG, (1U << 31) | (devfn << 8) | a.offset);
+    switch (a.size %= end_sizes) {
+    case Byte:
+        qtest_inb(s, PCI_HOST_BRIDGE_DATA);
+        break;
+    case Word:
+        qtest_inw(s, PCI_HOST_BRIDGE_DATA);
+        break;
+    case Long:
+        qtest_inl(s, PCI_HOST_BRIDGE_DATA);
+        break;
+    }
+}
+
+static void op_pci_write(QTestState *s, const unsigned char * data, size_t len)
+{
+    enum Sizes {Byte, Word, Long, end_sizes};
+    struct {
+        uint8_t size;
+        uint8_t base;
+        uint8_t offset;
+        uint32_t value;
+    } a;
+    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
+        return;
+    }
+    memcpy(&a, data, sizeof(a));
+    PCIDevice *dev = g_ptr_array_index(fuzzable_pci_devices,
+                                  a.base % fuzzable_pci_devices->len);
+    int devfn = dev->devfn;
+    qtest_outl(s, PCI_HOST_BRIDGE_CFG, (1U << 31) | (devfn << 8) | a.offset);
+    switch (a.size %= end_sizes) {
+    case Byte:
+        qtest_outb(s, PCI_HOST_BRIDGE_DATA, a.value & 0xFF);
+        break;
+    case Word:
+        qtest_outw(s, PCI_HOST_BRIDGE_DATA, a.value & 0xFFFF);
+        break;
+    case Long:
+        qtest_outl(s, PCI_HOST_BRIDGE_DATA, a.value & 0xFFFFFFFF);
+        break;
+    }
+}
+
 static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
 {
     qtest_clock_step_next(s);
@@ -327,6 +394,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
         [OP_OUT]                = op_out,
         [OP_READ]               = op_read,
         [OP_WRITE]              = op_write,
+        [OP_PCI_READ]           = op_pci_read,
+        [OP_PCI_WRITE]          = op_pci_write,
         [OP_CLOCK_STEP]         = op_clock_step,
     };
     const unsigned char *cmd = Data;
@@ -418,6 +487,19 @@ static int locate_fuzz_objects(Object *child, void *opaque)
     if (g_pattern_match_simple(pattern, object_get_typename(child))) {
         /* Find and save ptrs to any child MemoryRegions */
         object_child_foreach_recursive(child, locate_fuzz_memory_regions, NULL);
+
+        /*
+         * We matched an object. If its a PCI device, store a pointer to it so
+         * we can map BARs and fuzz its config space.
+         */
+        if (object_dynamic_cast(OBJECT(child), TYPE_PCI_DEVICE)) {
+            /*
+             * Don't want duplicate pointers to the same PCIDevice, so remove
+             * copies of the pointer, before adding it.
+             */
+            g_ptr_array_remove_fast(fuzzable_pci_devices, PCI_DEVICE(child));
+            g_ptr_array_add(fuzzable_pci_devices, PCI_DEVICE(child));
+        }
     } else if (object_dynamic_cast(OBJECT(child), TYPE_MEMORY_REGION)) {
         if (g_pattern_match_simple(pattern,
             object_get_canonical_path_component(child))) {
@@ -445,6 +527,7 @@ static void general_pre_fuzz(QTestState *s)
     }
 
     fuzzable_memoryregions = g_ptr_array_new();
+    fuzzable_pci_devices   = g_ptr_array_new();
     char **result = g_strsplit (getenv("QEMU_FUZZ_OBJECTS"), " ", -1);
     for (int i = 0; result[i] != NULL; i++) {
         printf("Matching objects by name %s\n", result[i]);
-- 
2.27.0

[PATCH v2 04/15] fuzz: Add DMA support to the generic-fuzzer

[Alexander Bulekov]

When a virtual-device tries to access some buffer in memory over DMA, we
add call-backs into the fuzzer(next commit). The fuzzer checks verifies
that the DMA request maps to a physical RAM address and fills the memory
with fuzzer-provided data. The patterns that we use to fill this memory
are specified using add_dma_pattern and clear_dma_patterns operations.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/general_fuzz.c | 178 ++++++++++++++++++++++++++++++++
 1 file changed, 178 insertions(+)

diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
index 17b572a439..36d41acea0 100644
--- a/tests/qtest/fuzz/general_fuzz.c
+++ b/tests/qtest/fuzz/general_fuzz.c
@@ -25,6 +25,8 @@
 #include "exec/address-spaces.h"
 #include "hw/qdev-core.h"
 #include "hw/pci/pci.h"
+#include "hw/boards.h"
+#include "exec/memory-internal.h"
 
 /*
  * SEPARATOR is used to separate "operations" in the fuzz input
@@ -38,12 +40,16 @@ enum cmds{
     OP_WRITE,
     OP_PCI_READ,
     OP_PCI_WRITE,
+    OP_ADD_DMA_PATTERN,
+    OP_CLEAR_DMA_PATTERNS,
     OP_CLOCK_STEP,
 };
 
 #define DEFAULT_TIMEOUT_US 100000
 #define USEC_IN_SEC 100000000
 
+#define MAX_DMA_FILL_SIZE 0x10000
+
 #define PCI_HOST_BRIDGE_CFG 0xcf8
 #define PCI_HOST_BRIDGE_DATA 0xcfc
 
@@ -53,6 +59,24 @@ typedef struct {
 } address_range;
 
 static useconds_t timeout = 100000;
+/*
+ * A pattern used to populate a DMA region or perform a memwrite. This is
+ * useful for e.g. populating tables of unique addresses.
+ * Example {.index = 1; .stride = 2; .len = 3; .data = "\x00\x01\x02"}
+ * Renders as: 00 01 02   00 03 03   00 05 03   00 07 03 ...
+ */
+typedef struct {
+    uint8_t index;      /* Index of a byte to increment by stride */
+    uint8_t stride;     /* Increment each index'th byte by this amount */
+    size_t len;
+    const uint8_t *data;
+} pattern;
+
+/* Avoid filling the same DMA region between MMIO/PIO commands ? */
+static bool avoid_double_fetches;
+
+static QTestState *qts_global; /* Need a global for the DMA callback */
+
 /*
  * List of memory regions that are children of QOM objects specified by the
  * user for fuzzing.
@@ -60,6 +84,116 @@ static useconds_t timeout = 100000;
 static GPtrArray *fuzzable_memoryregions;
 static GPtrArray *fuzzable_pci_devices;
 
+/*
+ * List of dma regions populated since the last fuzzing command. Used to ensure
+ * that we only write to each DMA address once, to avoid race conditions when
+ * building reproducers.
+ */
+static GArray *dma_regions;
+
+static GArray *dma_patterns;
+static int dma_pattern_index;
+
+void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write);
+
+/*
+ * Allocate a block of memory and populate it with a pattern.
+ */
+static void *pattern_alloc(pattern p, size_t len)
+{
+    int i;
+    uint8_t *buf = g_malloc(len);
+    uint8_t sum = 0;
+
+    for (i = 0; i < len; ++i) {
+        buf[i] = p.data[i % p.len];
+        if ((i % p.len) == p.index) {
+            buf[i] += sum;
+            sum += p.stride;
+        }
+    }
+    return buf;
+}
+
+/*
+ * Call-back for functions that perform DMA reads from guest memory. Confirm
+ * that the region has not already been populated since the last loop in
+ * general_fuzz(), avoiding potential race-conditions, which we don't have
+ * a good way for reproducing right now.
+ */
+void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write)
+{
+    /* Are we in the general-fuzzer or are we using another fuzz-target? */
+    if (!qts_global) {
+        return;
+    }
+
+    /*
+     * Return immediately if:
+     * - We have no DMA patterns defined
+     * - The length of the DMA read request is zero
+     * - The DMA read is hitting an MR other than the machine's main RAM
+     * - The DMA request is not a read (what happens for a address_space_map
+     *   with is_write=True? Can the device use the same pointer to do reads?)
+     * - The DMA request hits past the bounds of our RAM
+     */
+    if (dma_patterns->len == 0
+        || len == 0
+        || (mr != MACHINE(qdev_get_machine())->ram && !(mr->ops == &unassigned_mem_ops))
+        || is_write
+        || addr > current_machine->ram_size) {
+        return;
+    }
+
+    /*
+     * If we overlap with any existing dma_regions, split the range and only
+     * populate the non-overlapping parts.
+     */
+    for (int i = 0; i < dma_regions->len && avoid_double_fetches; ++i) {
+        address_range region = g_array_index(dma_regions, address_range, i);
+        if (addr < region.addr + region.len && addr + len > region.addr) {
+            if (addr < region.addr) {
+                fuzz_dma_read_cb(addr, region.addr - addr, mr, is_write);
+            }
+            if (addr + len > region.addr + region.len) {
+                fuzz_dma_read_cb(region.addr + region.len,
+                        addr + len - (region.addr + region.len), mr, is_write);
+            }
+            return;
+        }
+    }
+
+    /* Cap the length of the DMA access to something reasonable */
+    len = MIN(len, MAX_DMA_FILL_SIZE);
+
+    address_range ar = {addr, len};
+    g_array_append_val(dma_regions, ar);
+    pattern p = g_array_index(dma_patterns, pattern, dma_pattern_index);
+    void *buf = pattern_alloc(p, ar.len);
+    if (getenv("QTEST_LOG")) {
+        /*
+         * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log
+         * that will be written by qtest.c with a DMA tag, so we can reorder
+         * the resulting QTest trace so the DMA fills precede the last PIO/MMIO
+         * command.
+         */
+        fprintf(stderr, "[DMA] ");
+        fflush(stderr);
+        qtest_memwrite(qts_global, ar.addr, buf, ar.len);
+    } else {
+       /*
+        * Populate the region using address_space_write_rom to avoid writing to
+        * any IO MemoryRegions
+        */
+        address_space_write_rom(first_cpu->as, ar.addr, MEMTXATTRS_UNSPECIFIED,
+                buf, ar.len);
+    }
+    free(buf);
+
+    /* Increment the index of the pattern for the next DMA access */
+    dma_pattern_index = (dma_pattern_index + 1) % dma_patterns->len;
+}
+
 /*
  * Here we want to convert a fuzzer-provided [io-region-index, offset] to
  * a physical address. To do this, we iterate over all of the matched
@@ -350,6 +484,35 @@ static void op_pci_write(QTestState *s, const unsigned char * data, size_t len)
     }
 }
 
+static void op_add_dma_pattern(QTestState *s,
+                               const unsigned char *data, size_t len)
+{
+    struct {
+        /*
+         * index and stride can be used to increment the index-th byte of the
+         * pattern by the value stride, for each loop of the pattern.
+         */
+        uint8_t index;
+        uint8_t stride;
+    } a;
+
+    if (len < sizeof(a) + 1) {
+        return;
+    }
+    memcpy(&a, data, sizeof(a));
+    pattern p = {a.index, a.stride, len - sizeof(a), data + sizeof(a)};
+    p.index = a.index % p.len;
+    g_array_append_val(dma_patterns, p);
+    return;
+}
+
+static void op_clear_dma_patterns(QTestState *s,
+                                  const unsigned char *data, size_t len)
+{
+    g_array_set_size(dma_patterns, 0);
+    dma_pattern_index = 0;
+}
+
 static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
 {
     qtest_clock_step_next(s);
@@ -396,6 +559,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
         [OP_WRITE]              = op_write,
         [OP_PCI_READ]           = op_pci_read,
         [OP_PCI_WRITE]          = op_pci_write,
+        [OP_ADD_DMA_PATTERN]    = op_add_dma_pattern,
+        [OP_CLEAR_DMA_PATTERNS] = op_clear_dma_patterns,
         [OP_CLOCK_STEP]         = op_clock_step,
     };
     const unsigned char *cmd = Data;
@@ -425,6 +590,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
             setitimer(ITIMER_VIRTUAL, &timer, NULL);
         }
 
+        op_clear_dma_patterns(s, NULL, 0);
+
         while (cmd && Size) {
             /* Get the length until the next command or end of input */
             nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
@@ -441,6 +608,7 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
             /* Advance to the next command */
             cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
             Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
+            g_array_set_size(dma_regions, 0);
         }
         _Exit(0);
     } else {
@@ -455,6 +623,9 @@ static void usage(void)
     printf("QEMU_FUZZ_ARGS= the command line arguments passed to qemu\n");
     printf("QEMU_FUZZ_OBJECTS= "
             "a space separated list of QOM type names for objects to fuzz\n");
+    printf("Optionally: QEMU_AVOID_DOUBLE_FETCH= "
+            "Try to avoid racy DMA double fetch bugs? %d by default\n",
+            avoid_double_fetches);
     printf("Optionally: QEMU_FUZZ_TIMEOUT= Specify a custom timeout (us). "
             "0 to disable. %d by default\n", timeout);
     exit(0);
@@ -522,9 +693,16 @@ static void general_pre_fuzz(QTestState *s)
     if (!getenv("QEMU_FUZZ_OBJECTS")) {
         usage();
     }
+    if (getenv("QEMU_AVOID_DOUBLE_FETCH")) {
+        avoid_double_fetches = 1;
+    }
     if (getenv("QEMU_FUZZ_TIMEOUT")) {
         timeout = g_ascii_strtoll(getenv("QEMU_FUZZ_TIMEOUT"), NULL, 0);
     }
+    qts_global = s;
+
+    dma_regions = g_array_new(false, false, sizeof(address_range));
+    dma_patterns = g_array_new(false, false, sizeof(pattern));
 
     fuzzable_memoryregions = g_ptr_array_new();
     fuzzable_pci_devices   = g_ptr_array_new();
-- 
2.27.0

[PATCH v2 05/15] fuzz: Declare DMA Read callback function

[Alexander Bulekov]

This patch declares the fuzz_dma_read_cb function and uses the
preprocessor and linker(weak symbols) to handle these cases:

When we build softmmu/all with --enable-fuzzing, there should be no
strong symbol defined for fuzz_dma_read_cb, and we link against a weak
stub function.

When we build softmmu/fuzz with --enable-fuzzing, we link agains the
strong symbol in general_fuzz.c

When we build softmmu/all without --enable-fuzzing, fuzz_dma_read_cb is
an empty, inlined function. As long as we don't call any other functions
when building the arguments, there should be no overhead.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 include/exec/memory.h | 15 +++++++++++++++
 softmmu/memory.c      | 13 +++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 307e527835..2ec3b597f1 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -47,6 +47,21 @@
         OBJECT_GET_CLASS(IOMMUMemoryRegionClass, (obj), \
                          TYPE_IOMMU_MEMORY_REGION)
 
+#ifdef CONFIG_FUZZ
+void fuzz_dma_read_cb(size_t addr,
+                      size_t len,
+                      MemoryRegion *mr,
+                      bool is_write);
+#else
+static inline void fuzz_dma_read_cb(size_t addr,
+                                    size_t len,
+                                    MemoryRegion *mr,
+                                    bool is_write)
+{
+    /* Do Nothing */
+}
+#endif
+
 extern bool global_dirty_log;
 
 typedef struct MemoryRegionOps MemoryRegionOps;
diff --git a/softmmu/memory.c b/softmmu/memory.c
index af25987518..b0c2cf2535 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3223,6 +3223,19 @@ void memory_region_init_rom_device(MemoryRegion *mr,
     vmstate_register_ram(mr, owner_dev);
 }
 
+/*
+ * Support softmmu builds with CONFIG_FUZZ using a weak symbol and a stub for
+ * the fuzz_dma_read_cb callback
+ */
+#ifdef CONFIG_FUZZ
+void __attribute__((weak)) fuzz_dma_read_cb(size_t addr,
+                      size_t len,
+                      MemoryRegion *mr,
+                      bool is_write)
+{
+}
+#endif
+
 static const TypeInfo memory_region_info = {
     .parent             = TYPE_OBJECT,
     .name               = TYPE_MEMORY_REGION,
-- 
2.27.0

[PATCH v2 06/15] fuzz: Add fuzzer callbacks to DMA-read functions

[Alexander Bulekov]

We should be careful to not call any functions besides fuzz_dma_read_cb.
Without --enable-fuzzing, fuzz_dma_read_cb is an empty inlined function.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 exec.c                                | 2 ++
 include/exec/memory.h                 | 1 +
 include/exec/memory_ldst_cached.inc.h | 3 +++
 memory_ldst.inc.c                     | 4 ++++
 softmmu/memory.c                      | 1 +
 5 files changed, 11 insertions(+)

diff --git a/exec.c b/exec.c
index 6f381f98e2..c81f41514d 100644
--- a/exec.c
+++ b/exec.c
@@ -3241,6 +3241,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
             stn_he_p(buf, l, val);
         } else {
             /* RAM case */
+            fuzz_dma_read_cb(addr, len, mr, false);
             ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
             memcpy(buf, ram_ptr, l);
         }
@@ -3601,6 +3602,7 @@ void *address_space_map(AddressSpace *as,
     memory_region_ref(mr);
     *plen = flatview_extend_translation(fv, addr, len, mr, xlat,
                                         l, is_write, attrs);
+    fuzz_dma_read_cb(addr, *plen, mr, is_write);
     ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
 
     return ptr;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 2ec3b597f1..f8b943521a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2444,6 +2444,7 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
                           void *buf, hwaddr len)
 {
     assert(addr < cache->len && len <= cache->len - addr);
+    fuzz_dma_read_cb(cache->xlat + addr, len, cache->mrs.mr, false);
     if (likely(cache->ptr)) {
         memcpy(buf, cache->ptr + addr, len);
         return MEMTX_OK;
diff --git a/include/exec/memory_ldst_cached.inc.h b/include/exec/memory_ldst_cached.inc.h
index fd4bbb40e7..aff574039f 100644
--- a/include/exec/memory_ldst_cached.inc.h
+++ b/include/exec/memory_ldst_cached.inc.h
@@ -28,6 +28,7 @@ static inline uint32_t ADDRESS_SPACE_LD_CACHED(l)(MemoryRegionCache *cache,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     assert(addr < cache->len && 4 <= cache->len - addr);
+    fuzz_dma_read_cb(cache->xlat + addr, 4, cache->mrs.mr, false);
     if (likely(cache->ptr)) {
         return LD_P(l)(cache->ptr + addr);
     } else {
@@ -39,6 +40,7 @@ static inline uint64_t ADDRESS_SPACE_LD_CACHED(q)(MemoryRegionCache *cache,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     assert(addr < cache->len && 8 <= cache->len - addr);
+    fuzz_dma_read_cb(cache->xlat + addr, 8, cache->mrs.mr, false);
     if (likely(cache->ptr)) {
         return LD_P(q)(cache->ptr + addr);
     } else {
@@ -50,6 +52,7 @@ static inline uint32_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
     hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
 {
     assert(addr < cache->len && 2 <= cache->len - addr);
+    fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr, false);
     if (likely(cache->ptr)) {
         return LD_P(uw)(cache->ptr + addr);
     } else {
diff --git a/memory_ldst.inc.c b/memory_ldst.inc.c
index c54aee4a95..8d45d2eeff 100644
--- a/memory_ldst.inc.c
+++ b/memory_ldst.inc.c
@@ -42,6 +42,7 @@ static inline uint32_t glue(address_space_ldl_internal, SUFFIX)(ARG1_DECL,
                                         MO_32 | devend_memop(endian), attrs);
     } else {
         /* RAM case */
+        fuzz_dma_read_cb(addr, 4, mr, false);
         ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
@@ -110,6 +111,7 @@ static inline uint64_t glue(address_space_ldq_internal, SUFFIX)(ARG1_DECL,
                                         MO_64 | devend_memop(endian), attrs);
     } else {
         /* RAM case */
+        fuzz_dma_read_cb(addr, 8, mr, false);
         ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
@@ -175,6 +177,7 @@ uint32_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
         r = memory_region_dispatch_read(mr, addr1, &val, MO_8, attrs);
     } else {
         /* RAM case */
+        fuzz_dma_read_cb(addr, 1, mr, false);
         ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         val = ldub_p(ptr);
         r = MEMTX_OK;
@@ -212,6 +215,7 @@ static inline uint32_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL,
                                         MO_16 | devend_memop(endian), attrs);
     } else {
         /* RAM case */
+        fuzz_dma_read_cb(addr, 2, mr, false);
         ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
diff --git a/softmmu/memory.c b/softmmu/memory.c
index b0c2cf2535..be87044641 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1405,6 +1405,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
     unsigned size = memop_size(op);
     MemTxResult r;
 
+    fuzz_dma_read_cb(addr, size, mr, false);
     if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
         *pval = unassigned_mem_read(mr, addr, size);
         return MEMTX_DECODE_ERROR;
-- 
2.27.0

[PATCH v2 07/15] fuzz: Add support for custom crossover functions

[Alexander Bulekov]

libfuzzer supports a "custom crossover function". Libfuzzer often tries
to blend two inputs to create a new interesting input. Sometimes, we
have a better idea about how to blend inputs together. This change
allows fuzzers to specify a custom function for blending two inputs
together.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/fuzz.c | 13 +++++++++++++
 tests/qtest/fuzz/fuzz.h | 26 ++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
index 8234b68754..248fab5f37 100644
--- a/tests/qtest/fuzz/fuzz.c
+++ b/tests/qtest/fuzz/fuzz.c
@@ -118,6 +118,19 @@ static FuzzTarget *fuzz_get_target(char* name)
 }
 
 
+/* Sometimes called by libfuzzer to mutate two inputs into one */
+size_t LLVMFuzzerCustomCrossOver(const uint8_t *data1, size_t size1,
+                                 const uint8_t *data2, size_t size2,
+                                 uint8_t *out, size_t max_out_size,
+                                 unsigned int seed)
+{
+    if(fuzz_target->crossover) {
+        return fuzz_target->crossover(data1, size1, data2, size2, out,
+                                      max_out_size, seed);
+    }
+    return 0;
+}
+
 /* Executed for each fuzzing-input */
 int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size)
 {
diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h
index 9ca3d107c5..d36642b5ec 100644
--- a/tests/qtest/fuzz/fuzz.h
+++ b/tests/qtest/fuzz/fuzz.h
@@ -77,6 +77,28 @@ typedef struct FuzzTarget {
      */
     void(*fuzz)(QTestState *, const unsigned char *, size_t);
 
+    /*
+     * The fuzzer can specify a "Custom Crossover" function for combining two
+     * inputs from the corpus. This function is sometimes called by libfuzzer
+     * when mutating inputs.
+     *
+     * data1: location of first input
+     * size1: length of first input
+     * data1: location of second input
+     * size1: length of second input
+     * out: where to place the resulting, mutated input
+     * max_out_size: the maximum length of the input that can be placed in out
+     * seed: the seed that should be used to make mutations deterministic, when needed
+     *
+     * See libfuzzer's LLVMFuzzerCustomCrossOver API for more info.
+     *
+     * Can be NULL
+     */
+    size_t(*crossover)(const uint8_t *data1, size_t size1,
+                       const uint8_t *data2, size_t size2,
+                       uint8_t *out, size_t max_out_size,
+                       unsigned int seed);
+
 } FuzzTarget;
 
 void flush_events(QTestState *);
@@ -91,6 +113,10 @@ void fuzz_qtest_set_serialize(bool option);
  */
 void fuzz_add_target(const FuzzTarget *target);
 
+size_t LLVMFuzzerCustomCrossOver(const uint8_t *data1, size_t size1,
+                                 const uint8_t *data2, size_t size2,
+                                 uint8_t *out, size_t max_out_size,
+                                 unsigned int seed);
 int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size);
 int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp);
 
-- 
2.27.0

[PATCH v2 08/15] fuzz: add a DISABLE_PCI op to general-fuzzer

[Alexander Bulekov]

This new operation is used in the next commit, which concatenates two
fuzzer-generated inputs. With this operation, we can prevent the second
input from clobbering the PCI configuration performed by the first.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/general_fuzz.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
index 36d41acea0..26fcd69e45 100644
--- a/tests/qtest/fuzz/general_fuzz.c
+++ b/tests/qtest/fuzz/general_fuzz.c
@@ -40,6 +40,7 @@ enum cmds{
     OP_WRITE,
     OP_PCI_READ,
     OP_PCI_WRITE,
+    OP_DISABLE_PCI,
     OP_ADD_DMA_PATTERN,
     OP_CLEAR_DMA_PATTERNS,
     OP_CLOCK_STEP,
@@ -93,6 +94,7 @@ static GArray *dma_regions;
 
 static GArray *dma_patterns;
 static int dma_pattern_index;
+static bool pci_disabled = false;
 
 void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write);
 
@@ -433,7 +435,7 @@ static void op_pci_read(QTestState *s, const unsigned char * data, size_t len)
         uint8_t base;
         uint8_t offset;
     } a;
-    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
+    if (len < sizeof(a) || fuzzable_pci_devices->len == 0 || pci_disabled) {
         return;
     }
     memcpy(&a, data, sizeof(a));
@@ -463,7 +465,7 @@ static void op_pci_write(QTestState *s, const unsigned char * data, size_t len)
         uint8_t offset;
         uint32_t value;
     } a;
-    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
+    if (len < sizeof(a) || fuzzable_pci_devices->len == 0 || pci_disabled) {
         return;
     }
     memcpy(&a, data, sizeof(a));
@@ -518,6 +520,11 @@ static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
     qtest_clock_step_next(s);
 }
 
+static void op_disable_pci(QTestState *s, const unsigned char *data, size_t len)
+{
+    pci_disabled = true;
+}
+
 static void handle_timeout(int sig)
 {
     if (getenv("QTEST_LOG")) {
@@ -559,6 +566,7 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
         [OP_WRITE]              = op_write,
         [OP_PCI_READ]           = op_pci_read,
         [OP_PCI_WRITE]          = op_pci_write,
+        [OP_DISABLE_PCI]        = op_disable_pci,
         [OP_ADD_DMA_PATTERN]    = op_add_dma_pattern,
         [OP_CLEAR_DMA_PATTERNS] = op_clear_dma_patterns,
         [OP_CLOCK_STEP]         = op_clock_step,
@@ -591,6 +599,7 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
         }
 
         op_clear_dma_patterns(s, NULL, 0);
+        pci_disabled = false;
 
         while (cmd && Size) {
             /* Get the length until the next command or end of input */
-- 
2.27.0

[PATCH v2 09/15] fuzz: add a crossover function to generic-fuzzer

[Alexander Bulekov]

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/general_fuzz.c | 81 ++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
index 26fcd69e45..2c3716f8cc 100644
--- a/tests/qtest/fuzz/general_fuzz.c
+++ b/tests/qtest/fuzz/general_fuzz.c
@@ -739,6 +739,83 @@ static void general_pre_fuzz(QTestState *s)
 
     counter_shm_init();
 }
+
+/*
+ * When libfuzzer gives us two inputs to combine, return a new input with the
+ * following structure:
+ *
+ * Input 1 (data1)
+ * SEPARATOR
+ * Clear out the DMA Patterns
+ * SEPARATOR
+ * Disable the pci_read/write instructions
+ * SEPARATOR
+ * Input 2 (data2)
+ *
+ * The idea is to collate the core behaviors of the two inputs.
+ * For example:
+ * Input 1: maps a device's BARs, sets up three DMA patterns, and triggers
+ *          device functionality A
+ * Input 2: maps a device's BARs, sets up one DMA pattern, and triggers device
+ *          functionality B
+ *
+ * This function attempts to produce an input that:
+ * Ouptut: maps a device's BARs, set up three DMA patterns, triggers
+ *          functionality A device, replaces the DMA patterns with a single
+ *          patten, and triggers device functionality B.
+ */
+static size_t general_fuzz_crossover(const uint8_t *data1, size_t size1, const
+                                     uint8_t *data2, size_t size2, uint8_t *out,
+                                     size_t max_out_size, unsigned int seed)
+{
+    size_t copy = 0, size = 0;
+
+    // Copy in the first input
+    copy = MIN(size1, max_out_size);
+    memcpy(out+size, data1, copy);
+    size+= copy;
+    max_out_size-= copy;
+
+    // Append a separator
+    copy = MIN(strlen(SEPARATOR), max_out_size);
+    memcpy(out+size, SEPARATOR, copy);
+    size+= copy;
+    max_out_size-= copy;
+
+    // Clear out the
+    copy = MIN(1, max_out_size);
+    if (copy) {
+        out[size] = OP_CLEAR_DMA_PATTERNS;
+    }
+    size+= copy;
+    max_out_size-= copy;
+
+    copy = MIN(strlen(SEPARATOR), max_out_size);
+    memcpy(out+size, SEPARATOR, copy);
+    size+= copy;
+    max_out_size-= copy;
+
+    copy = MIN(1, max_out_size);
+    if (copy) {
+        out[size] = OP_DISABLE_PCI;
+    }
+    size+= copy;
+    max_out_size-= copy;
+
+    copy = MIN(strlen(SEPARATOR), max_out_size);
+    memcpy(out+size, SEPARATOR, copy);
+    size+= copy;
+    max_out_size-= copy;
+
+    copy = MIN(size2, max_out_size);
+    memcpy(out+size, data2, copy);
+    size+= copy;
+    max_out_size-= copy;
+
+    return  size;
+}
+
+
 static GString *general_fuzz_cmdline(FuzzTarget *t)
 {
     GString *cmd_line = g_string_new(TARGET_NAME);
@@ -758,7 +835,9 @@ static void register_general_fuzz_targets(void)
             .description = "Fuzz based on any qemu command-line args. ",
             .get_init_cmdline = general_fuzz_cmdline,
             .pre_fuzz = general_pre_fuzz,
-            .fuzz = general_fuzz});
+            .fuzz = general_fuzz,
+            .crossover = general_fuzz_crossover
+    });
 }
 
 fuzz_target_init(register_general_fuzz_targets);
-- 
2.27.0

[PATCH v2 10/15] scripts/oss-fuzz: Add wrapper program for generic fuzzer

[Alexander Bulekov]

On oss-fuzz we need some sort of wrapper to specify command-line
arguments or environment variables. When we had a similar problem with
other targets that I fixed with
05509c8e6d ("fuzz: select fuzz target using executable name")
by selecting the fuzz target based on the executable's name. In the
future should probably commit to one approach (wrapper binary or
argv0-based target selection).

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 scripts/oss-fuzz/target.c | 40 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 scripts/oss-fuzz/target.c

diff --git a/scripts/oss-fuzz/target.c b/scripts/oss-fuzz/target.c
new file mode 100644
index 0000000000..4a7257412a
--- /dev/null
+++ b/scripts/oss-fuzz/target.c
@@ -0,0 +1,40 @@
+/*
+ * Copyright Red Hat Inc., 2020
+ *
+ * Authors:
+ *  Alexander Bulekov   <alxndr@bu.edu>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <limits.h>
+#include <libgen.h>
+#include <string.h>
+#include <unistd.h>
+
+
+/* Required for oss-fuzz to consider the binary a target. */
+static const char *magic __attribute__((used)) = "LLVMFuzzerTestOneInput";
+static const char args[] = {QEMU_FUZZ_ARGS, 0x00};
+static const char objects[] = {QEMU_FUZZ_OBJECTS, 0x00};
+
+int main(int argc, char *argv[])
+{
+    char path[PATH_MAX] = {0};
+    char *dir = dirname(argv[0]);
+    strncpy(path, dir, PATH_MAX);
+    strcat(path, "/deps/qemu-fuzz-i386-target-general-fuzz");
+
+    setenv("QEMU_FUZZ_ARGS", args, 0);
+    setenv("QEMU_FUZZ_OBJECTS", objects, 0);
+
+    argv[0] = path;
+    int ret = execvp(path, argv);
+    if (ret) {
+        perror("execv");
+    }
+    return ret;
+}
-- 
2.27.0

[PATCH v2 11/15] scripts/oss-fuzz: Add general-fuzzer build script

[Alexander Bulekov]

This parses a yaml file containing general-fuzzer configs and builds a
separate oss-fuzz wrapper binary for each one, changing some
preprocessor macros for each configuration. To avoid dealing with
escaping and stringifying, convert each string into a byte-array
representation

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 scripts/oss-fuzz/build_general_fuzzers.py | 62 +++++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100755 scripts/oss-fuzz/build_general_fuzzers.py

diff --git a/scripts/oss-fuzz/build_general_fuzzers.py b/scripts/oss-fuzz/build_general_fuzzers.py
new file mode 100755
index 0000000000..79f4664117
--- /dev/null
+++ b/scripts/oss-fuzz/build_general_fuzzers.py
@@ -0,0 +1,62 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+"""
+This script creates wrapper binaries that invoke the general-device-fuzzer with
+configurations specified in a yaml config file.
+"""
+import sys
+import os
+import yaml
+import tempfile
+
+CC = ""
+TEMPLATE = ""
+
+
+def usage():
+    print("Usage: CC=COMPILER {} CONFIG_PATH \
+OUTPUT_PATH_PREFIX".format(sys.argv[0]))
+    sys.exit(0)
+
+
+def str_to_c_byte_array(s):
+    """
+    Convert strings to byte-arrays so we don't worry about formatting
+    strings to play nicely with cc -DQEMU_FUZZARGS etc
+    """
+    return ','.join('0x{:02x}'.format(ord(x)) for x in s)
+
+
+def compile_wrapper(cfg, path):
+    os.system('$CC -DQEMU_FUZZ_ARGS="{}" -DQEMU_FUZZ_OBJECTS="{}" \
+                {} -o {}'.format(
+                    str_to_c_byte_array(cfg["args"].replace("\n", " ")),
+                    str_to_c_byte_array(cfg["objects"].replace("\n", " ")),
+                    TEMPLATE, path))
+
+
+def main():
+    global CC
+    global TEMPLATE
+
+    if len(sys.argv) != 3:
+        usage()
+
+    cfg_path = sys.argv[1]
+    out_path = sys.argv[2]
+
+    CC = os.getenv("CC")
+    TEMPLATE = os.path.join(os.path.dirname(__file__), "target.c")
+
+    with open(cfg_path, "r") as f:
+        configs = yaml.load(f)["configs"]
+    for cfg in configs:
+        assert "name" in cfg
+        assert "args" in cfg
+        assert "objects" in cfg
+        compile_wrapper(cfg, out_path + cfg["name"])
+
+
+if __name__ == '__main__':
+    main()
-- 
2.27.0

[PATCH v2 12/15] scripts/oss-fuzz: Add general-fuzzer configs for oss-fuzz

[Alexander Bulekov]

Each of these entries is built into a wrapper binary that sets the
needed environment variables and executes the general virtual-device
fuzzer. In the future, we will need additional fields, such as arch=arm,
timeout_per_testcase=0, reset=reboot, etc...

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 scripts/oss-fuzz/general_fuzzer_configs.yml | 103 ++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 scripts/oss-fuzz/general_fuzzer_configs.yml

diff --git a/scripts/oss-fuzz/general_fuzzer_configs.yml b/scripts/oss-fuzz/general_fuzzer_configs.yml
new file mode 100644
index 0000000000..010e92a2a5
--- /dev/null
+++ b/scripts/oss-fuzz/general_fuzzer_configs.yml
@@ -0,0 +1,103 @@
+configs:
+    - name: virtio-net-pci-slirp
+      args: >
+        -M q35 -nodefaults
+        -device virtio-net,netdev=net0 -netdev user,id=net0
+      objects: virtio*
+
+    - name: virtio-blk
+      args: >
+        -machine q35 -device virtio-blk,drive=disk0
+        -drive file=null-co://,id=disk0,if=none,format=raw
+      objects: virtio*
+
+    - name: virtio-scsi
+      args: >
+        -machine q35 -device virtio-scsi,num_queues=8
+        -device scsi-hd,drive=disk0
+        -drive file=null-co://,id=disk0,if=none,format=raw
+      objects: scsi* virtio*
+
+    - name: virtio-gpu
+      args: -machine q35 -nodefaults -device virtio-gpu
+      objects: virtio*
+
+    - name: virtio-vga
+      args: -machine q35 -nodefaults -device virtio-vga
+      objects: virtio*
+
+    - name: virtio-rng
+      args: -machine q35 -nodefaults -device virtio-rng
+      objects: virtio*
+
+    - name: virtio-balloon
+      args: -machine q35 -nodefaults -device virtio-balloon
+      objects: virtio*
+
+    - name: virtio-serial
+      args: -machine q35 -nodefaults -device virtio-serial
+      objects: virtio*
+
+    - name: virtio-mouse
+      args: -machine q35 -nodefaults -device virtio-mouse
+      objects: virtio*
+
+    - name: e1000
+      args: >
+        -M q35 -nodefaults
+        -device e1000,netdev=net0 -netdev user,id=net0
+      objects: e1000
+
+    - name: e1000e
+      args: >
+        -M q35 -nodefaults
+        -device e1000e,netdev=net0 -netdev user,id=net0
+      objects: e1000e
+
+    - name: cirrus-vga
+      args: -machine q35 -nodefaults -device cirrus-vga
+      objects: cirrus*
+
+    - name: bochs-display
+      args: -machine q35 -nodefaults -device bochs-display
+      objects: bochs*
+
+    - name: intel-hda
+      args: >
+        -machine q35 -nodefaults -device intel-hda,id=hda0
+        -device hda-output,bus=hda0.0 -device hda-micro,bus=hda0.0
+        -device hda-duplex,bus=hda0.0
+      objects: intel-hda
+
+    - name: ide-hd
+      args: >
+        -machine q35 -nodefaults
+        -drive file=null-co://,if=none,format=raw,id=disk0
+        -device ide-hd,drive=disk0
+      objects: ahci*
+
+    - name: floppy
+      args: >
+        -machine pc -nodefaults -device floppy,id=floppy0
+        -drive id=disk0,file=null-co://,file.read-zeroes=on,if=none
+        -device floppy,drive=disk0,drive-type=288
+      objects: fd* floppy*
+
+    - name: xhci
+      args: >
+        -machine q35 -nodefaults
+        -drive file=null-co://,if=none,format=raw,id=disk0
+        -device qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0 -device usb-bot
+        -device usb-storage,drive=disk0 -chardev null,id=cd0 -chardev null,id=cd1
+        -device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid
+        -device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1
+        -device usb-tablet -device usb-wacom-tablet -device usb-audio
+      objects: "*usb* *uhci* *xhci*"
+
+    - name: pc-i440fx
+      args: -machine pc
+      objects: "*"
+
+    - name: pc-q35
+      args: -machine q35
+      objects: "*"
-- 
2.27.0

[PATCH v2 13/15] scripts/oss-fuzz: build the general-fuzzer configs

[Alexander Bulekov]

Build general-fuzzer wrappers for each configuration defined in
general_fuzzer_configs.yml and move the actual general-fuzzer to a
subdirectory, so oss-fuzz doesn't treat it as a standalone fuzzer.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 scripts/oss-fuzz/build.sh                   | 8 +++++++-
 scripts/oss-fuzz/general_fuzzer_configs.yml | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh
index a07b3022e8..2071e77ac2 100755
--- a/scripts/oss-fuzz/build.sh
+++ b/scripts/oss-fuzz/build.sh
@@ -38,7 +38,7 @@ OSS_FUZZ_BUILD_DIR="./build-oss-fuzz/"
 # remove it, resulting in an unresolved reference to qemu_build_not_reached
 # Undefine the __OPTIMIZE__ macro which compiler.h relies on to choose whether
 # to " #define qemu_build_not_reached()  g_assert_not_reached() "
-EXTRA_CFLAGS="$CFLAGS -U __OPTIMIZE__"
+EXTRA_CFLAGS="$CFLAGS -U __OPTIMIZE__ -DCONFIG_FUZZ=y"
 
 if ! { [ -e "./COPYING" ] &&
    [ -e "./MAINTAINERS" ] &&
@@ -101,5 +101,11 @@ do
     cp ./i386-softmmu/qemu-fuzz-i386 "$DEST_DIR/qemu-fuzz-i386-target-$target"
 done
 
+mkdir -p "$DEST_DIR/deps"
+mv "$DEST_DIR/qemu-fuzz-i386-target-general-fuzz" "$DEST_DIR/deps/"
+
+./scripts/oss-fuzz/build_general_fuzzers.py \
+    "./scripts/oss-fuzz/general_fuzzer_configs.yml" "$DEST_DIR/general-fuzz-"
+
 echo "Done. The fuzzers are located in $DEST_DIR"
 exit 0
diff --git a/scripts/oss-fuzz/general_fuzzer_configs.yml b/scripts/oss-fuzz/general_fuzzer_configs.yml
index 010e92a2a5..f70bacb243 100644
--- a/scripts/oss-fuzz/general_fuzzer_configs.yml
+++ b/scripts/oss-fuzz/general_fuzzer_configs.yml
@@ -92,7 +92,7 @@ configs:
         -device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid
         -device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1
         -device usb-tablet -device usb-wacom-tablet -device usb-audio
-      objects: "*usb* *uhci* *xhci*"
+      objects: "*usb* *xhci*"
 
     - name: pc-i440fx
       args: -machine pc
-- 
2.27.0

[PATCH v2 14/15] scripts/oss-fuzz: Add script to reorder a general-fuzzer trace

[Alexander Bulekov]

The general-fuzzer uses hooks to fulfill DMA requests just-in-time.
This means that if we try to use QTEST_LOG=1 to build a reproducer, the
DMA writes will be logged _after_ the in/out/read/write that triggered
the DMA read. To work work around this, the general-fuzzer annotates
these just-in time DMA fulfilments with a tag that we can use to
discern them. This script simply iterates over a raw qtest
trace (including log messages, errors, timestamps etc), filters it and
re-orders it so that DMA fulfillments are placed directly _before_ the
qtest command that will cause the DMA access.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 .../oss-fuzz/reorder_fuzzer_qtest_trace.py    | 94 +++++++++++++++++++
 1 file changed, 94 insertions(+)
 create mode 100755 scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py

diff --git a/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py b/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
new file mode 100755
index 0000000000..9fb7edb6ee
--- /dev/null
+++ b/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
@@ -0,0 +1,94 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+"""
+Use this to convert qtest log info from a generic fuzzer input into a qtest
+trace that you can feed into a standard qemu-system process. Example usage:
+
+QEMU_FUZZ_ARGS="-machine q35,accel=qtest" QEMU_FUZZ_OBJECTS="*" \
+        ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=general-pci-fuzz
+# .. Finds some crash
+QTEST_LOG=1 FUZZ_SERIALIZE_QTEST=1 \
+QEMU_FUZZ_ARGS="-machine q35,accel=qtest" QEMU_FUZZ_OBJECTS="*" \
+        ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=general-pci-fuzz
+        /path/to/crash 2> qtest_log_output
+scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py qtest_log_output > qtest_trace
+./i386-softmmu/qemu-fuzz-i386 -machine q35,accel=qtest \
+        -qtest stdin < qtest_trace
+
+### Details ###
+
+Some fuzzer make use of hooks that allow us to populate some memory range, just
+before a DMA read from that range. This means that the fuzzer can produce
+activity that looks like:
+    [start] read from mmio addr
+    [end]   read from mmio addr
+    [start] write to pio addr
+        [start] fill a DMA buffer just in time
+        [end]   fill a DMA buffer just in time
+        [start] fill a DMA buffer just in time
+        [end]   fill a DMA buffer just in time
+    [end]   write to pio addr
+    [start] read from mmio addr
+    [end]   read from mmio addr
+
+We annotate these "nested" DMA writes, so with QTEST_LOG=1 the QTest trace
+might look something like:
+[R +0.028431] readw 0x10000
+[R +0.028434] outl 0xc000 0xbeef  # Triggers a DMA read from 0xbeef and 0xbf00
+[DMA][R +0.034639] write 0xbeef 0x2 0xAAAA
+[DMA][R +0.034639] write 0xbf00 0x2 0xBBBB
+[R +0.028431] readw 0xfc000
+
+This script would reorder the above trace so it becomes:
+readw 0x10000
+write 0xbeef 0x2 0xAAAA
+write 0xbf00 0x2 0xBBBB
+outl 0xc000 0xbeef
+readw 0xfc000
+
+I.e. by the time, 0xc000 tries to read from DMA, those DMA buffers have already
+been set up, removing the need for the DMA hooks. We can simply provide this
+reordered trace via -qtest stdio to reproduce the input
+
+Note: this won't work for traces where the device tries to read from the same
+DMA region twice in between MMIO/PIO commands. E.g:
+    [R +0.028434] outl 0xc000 0xbeef
+    [DMA][R +0.034639] write 0xbeef 0x2 0xAAAA
+    [DMA][R +0.034639] write 0xbeef 0x2 0xBBBB
+"""
+
+import sys
+
+__author__     = "Alexander Bulekov <alxndr@bu.edu>"
+__copyright__  = "Copyright (C) 2020, Red Hat, Inc."
+__license__    = "GPL version 2 or (at your option) any later version"
+
+__maintainer__ = "Alexander Bulekov"
+__email__      = "alxndr@bu.edu"
+
+
+def usage():
+    sys.exit("Usage: {} /path/to/qtest_log_output".format((sys.argv[0])))
+
+
+def main(filename):
+    with open(filename, "r") as f:
+        trace = f.readlines()
+
+    # Leave only lines that look like logged qtest commands
+    trace[:] = [x.strip() for x in trace if "[R +" in x
+                or "[S +" in x and "CLOSED" not in x]
+
+    for i in range(len(trace)):
+        if i+1 < len(trace):
+            if "[DMA]" in trace[i+1]:
+                trace[i], trace[i+1] = trace[i+1], trace[i]
+    for line in trace:
+        print(line.split("]")[-1].strip())
+
+
+if __name__ == '__main__':
+    if len(sys.argv) == 1:
+        usage()
+    main(sys.argv[1])
-- 
2.27.0

[PATCH v2 15/15] scripts/oss-fuzz: Add crash trace minimization script

[Alexander Bulekov]

Once we find a crash, we can convert it into a QTest trace. Usually this
trace will contain many operations that are unneeded to reproduce the
crash. This script tries to minimize the crashing trace, by removing
operations and trimming QTest bufwrite(write addr len data...) commands.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 scripts/oss-fuzz/minimize_qtest_trace.py | 118 +++++++++++++++++++++++
 1 file changed, 118 insertions(+)
 create mode 100755 scripts/oss-fuzz/minimize_qtest_trace.py

diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py b/scripts/oss-fuzz/minimize_qtest_trace.py
new file mode 100755
index 0000000000..2f1f4f368e
--- /dev/null
+++ b/scripts/oss-fuzz/minimize_qtest_trace.py
@@ -0,0 +1,118 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+
+"""
+This takes a crashing qtest trace and tries to remove superflous operations
+"""
+
+import sys
+import os
+import subprocess
+import time
+
+QEMU_ARGS = None
+QEMU_PATH = None
+TIMEOUT = 5
+CRASH_TOKEN = None
+
+
+def usage():
+    sys.exit("""\
+Usage: QEMU_PATH="/path/to/qemu" QEMU_ARGS="args" {} input_trace output_trace
+By default, will try to use the second-to-last line in the output to identify
+whether the crash occred. Optionally, manually set a string that idenitifes the
+crash by setting CRASH_TOKEN=
+""".format((sys.argv[0])))
+
+
+def check_if_trace_crashes(trace, path):
+    global CRASH_TOKEN
+    with open(path, "w") as tracefile:
+        tracefile.write("".join(trace))
+    rc = subprocess.Popen("timeout -s 9 {}s {} {} 2>&1 < {}".format(TIMEOUT,
+                          QEMU_PATH, QEMU_ARGS, path),
+                          shell=True, stdin=subprocess.PIPE,
+                          stdout=subprocess.PIPE)
+    stdo = rc.communicate()[0]
+    output = stdo.decode('unicode_escape')
+    if rc.returncode == 137:    # Timed Out
+        return False
+    if len(output.splitlines()) < 2:
+        return False
+
+    if CRASH_TOKEN is None:
+        CRASH_TOKEN = output.splitlines()[-2]
+
+    return CRASH_TOKEN in output
+
+
+def minimize_trace(inpath, outpath):
+    global TIMEOUT
+    with open(inpath) as f:
+        trace = f.readlines()
+    start = time.time()
+    if not check_if_trace_crashes(trace, outpath):
+        sys.exit("The input qtest trace didn't cause a crash...")
+    end = time.time()
+    print("Crashed in {} seconds".format(end-start))
+    TIMEOUT = (end-start)*5
+    print("Setting the timeout for {} seconds".format(TIMEOUT))
+    print("Identifying Crashes by this string: {}".format(CRASH_TOKEN))
+
+    i = 0
+    newtrace = trace[:]
+    while i < len(newtrace):
+        prior = newtrace[i]
+        print("Trying to remove {}".format(newtrace[i]))
+        # Try to remove the line completely
+        newtrace[i] = ""
+        if check_if_trace_crashes(newtrace, outpath):
+            i += 1
+            continue
+        newtrace[i] = prior
+        # Try to split up writes into multiple commands, each of which can be
+        # removed.
+        if newtrace[i].startswith("write "):
+            addr = int(newtrace[i].split()[1], 16)
+            length = int(newtrace[i].split()[2], 16)
+            data = newtrace[i].split()[3][2:]
+            if length > 1:
+                leftlength = int(length/2)
+                rightlength = length - leftlength
+                newtrace.insert(i+1, "")
+                while leftlength > 0:
+                    newtrace[i] = "write {} {} 0x{}\n".format(
+                            hex(addr),
+                            hex(leftlength),
+                            data[:leftlength*2])
+                    newtrace[i+1] = "write {} {} 0x{}\n".format(
+                            hex(addr+leftlength),
+                            hex(rightlength),
+                            data[leftlength*2:])
+                    if check_if_trace_crashes(newtrace, outpath):
+                        break
+                    else:
+                        leftlength -= 1
+                        rightlength += 1
+                if check_if_trace_crashes(newtrace, outpath):
+                    i -= 1
+                else:
+                    newtrace[i] = prior
+                    del newtrace[i+1]
+        i += 1
+    check_if_trace_crashes(newtrace, outpath)
+
+
+if __name__ == '__main__':
+    if len(sys.argv) < 3:
+        usage()
+
+    QEMU_PATH = os.getenv("QEMU_PATH")
+    QEMU_ARGS = os.getenv("QEMU_ARGS")
+    if QEMU_PATH is None or QEMU_ARGS is None:
+        usage()
+    if "accel" not in QEMU_ARGS:
+        QEMU_ARGS += " -accel qtest"
+    CRASH_TOKEN = os.getenv("CRASH_TOKEN")
+    QEMU_ARGS += " -qtest stdio -monitor none -serial none "
+    minimize_trace(sys.argv[1], sys.argv[2])
-- 
2.27.0

Re: [PATCH v2 00/15] Add a General Virtual Device Fuzzer

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200819061110.1320568-1-alxndr@bu.edu/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20200819061110.1320568-1-alxndr@bu.edu
Subject: [PATCH v2 00/15] Add a General Virtual Device Fuzzer

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
a8e119d scripts/oss-fuzz: Add crash trace minimization script
ae04d9e scripts/oss-fuzz: Add script to reorder a general-fuzzer trace
565c5c5 scripts/oss-fuzz: build the general-fuzzer configs
559cd36 scripts/oss-fuzz: Add general-fuzzer configs for oss-fuzz
54db062 scripts/oss-fuzz: Add general-fuzzer build script
8973b6e scripts/oss-fuzz: Add wrapper program for generic fuzzer
3452c68 fuzz: add a crossover function to generic-fuzzer
5c579c9 fuzz: add a DISABLE_PCI op to general-fuzzer
4f50ecd fuzz: Add support for custom crossover functions
95bd76d fuzz: Add fuzzer callbacks to DMA-read functions
89e6484 fuzz: Declare DMA Read callback function
a5441b1 fuzz: Add DMA support to the generic-fuzzer
9bd3375 fuzz: Add PCI features to the general fuzzer
a2759f3 fuzz: Add general virtual-device fuzzer
f9c6ddd fuzz: Change the way we write qtest log to stderr

=== OUTPUT BEGIN ===
1/15 Checking commit f9c6ddda8115 (fuzz: Change the way we write qtest log to stderr)
2/15 Checking commit a2759f329ffa (fuzz: Add general virtual-device fuzzer)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#31: 
new file mode 100644

ERROR: missing space after enum definition
#68: FILE: tests/qtest/fuzz/general_fuzz.c:33:
+enum cmds{

ERROR: line over 90 characters
#108: FILE: tests/qtest/fuzz/general_fuzz.c:73:
+    AddressSpace *as = (io_space == get_system_memory()) ? &address_space_memory : &address_space_io;

ERROR: line over 90 characters
#124: FILE: tests/qtest/fuzz/general_fuzz.c:89:
+                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){

ERROR: space required before the open brace '{'
#124: FILE: tests/qtest/fuzz/general_fuzz.c:89:
+                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){

ERROR: space required before the open parenthesis '('
#124: FILE: tests/qtest/fuzz/general_fuzz.c:89:
+                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){

ERROR: space required before the open brace '{'
#128: FILE: tests/qtest/fuzz/general_fuzz.c:93:
+                    if(mr->size){

ERROR: space required before the open parenthesis '('
#128: FILE: tests/qtest/fuzz/general_fuzz.c:93:
+                    if(mr->size){

ERROR: spaces required around that '-' (ctx:VxV)
#131: FILE: tests/qtest/fuzz/general_fuzz.c:96:
+                    result->len = mr->size-(result->addr-abs_addr);
                                           ^

ERROR: spaces required around that '-' (ctx:VxV)
#131: FILE: tests/qtest/fuzz/general_fuzz.c:96:
+                    result->len = mr->size-(result->addr-abs_addr);
                                                         ^

ERROR: space prohibited between function name and open parenthesis '('
#483: FILE: tests/qtest/fuzz/general_fuzz.c:448:
+    char **result = g_strsplit (getenv("QEMU_FUZZ_OBJECTS"), " ", -1);

ERROR: space required before the open brace '{'
#500: FILE: tests/qtest/fuzz/general_fuzz.c:465:
+    if(!fuzzable_memoryregions->len){

ERROR: space required before the open parenthesis '('
#500: FILE: tests/qtest/fuzz/general_fuzz.c:465:
+    if(!fuzzable_memoryregions->len){

total: 12 errors, 1 warnings, 501 lines checked

Patch 2/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/15 Checking commit 9bd3375b88bc (fuzz: Add PCI features to the general fuzzer)
4/15 Checking commit a5441b1099c7 (fuzz: Add DMA support to the generic-fuzzer)
ERROR: externs should be avoided in .c files
#84: FILE: tests/qtest/fuzz/general_fuzz.c:97:
+void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write);

WARNING: line over 80 characters
#129: FILE: tests/qtest/fuzz/general_fuzz.c:142:
+        || (mr != MACHINE(qdev_get_machine())->ram && !(mr->ops == &unassigned_mem_ops))

total: 1 errors, 1 warnings, 247 lines checked

Patch 4/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/15 Checking commit 89e64845b1dd (fuzz: Declare DMA Read callback function)
6/15 Checking commit 95bd76d180c1 (fuzz: Add fuzzer callbacks to DMA-read functions)
7/15 Checking commit 4f50ecd4705c (fuzz: Add support for custom crossover functions)
ERROR: space required before the open parenthesis '('
#30: FILE: tests/qtest/fuzz/fuzz.c:127:
+    if(fuzz_target->crossover) {

WARNING: line over 80 characters
#59: FILE: tests/qtest/fuzz/fuzz.h:91:
+     * seed: the seed that should be used to make mutations deterministic, when needed

total: 1 errors, 1 warnings, 57 lines checked

Patch 7/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

8/15 Checking commit 5c579c959fec (fuzz: add a DISABLE_PCI op to general-fuzzer)
ERROR: do not initialise statics to 0 or NULL
#30: FILE: tests/qtest/fuzz/general_fuzz.c:97:
+static bool pci_disabled = false;

total: 1 errors, 0 warnings, 55 lines checked

Patch 8/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

9/15 Checking commit 3452c68ac630 (fuzz: add a crossover function to generic-fuzzer)
ERROR: do not use C99 // comments
#49: FILE: tests/qtest/fuzz/general_fuzz.c:773:
+    // Copy in the first input

ERROR: spaces required around that '+' (ctx:VxV)
#51: FILE: tests/qtest/fuzz/general_fuzz.c:775:
+    memcpy(out+size, data1, copy);
               ^

ERROR: spaces required around that '+=' (ctx:VxW)
#52: FILE: tests/qtest/fuzz/general_fuzz.c:776:
+    size+= copy;
         ^

ERROR: spaces required around that '-=' (ctx:VxW)
#53: FILE: tests/qtest/fuzz/general_fuzz.c:777:
+    max_out_size-= copy;
                 ^

ERROR: do not use C99 // comments
#55: FILE: tests/qtest/fuzz/general_fuzz.c:779:
+    // Append a separator

ERROR: spaces required around that '+' (ctx:VxV)
#57: FILE: tests/qtest/fuzz/general_fuzz.c:781:
+    memcpy(out+size, SEPARATOR, copy);
               ^

ERROR: spaces required around that '+=' (ctx:VxW)
#58: FILE: tests/qtest/fuzz/general_fuzz.c:782:
+    size+= copy;
         ^

ERROR: spaces required around that '-=' (ctx:VxW)
#59: FILE: tests/qtest/fuzz/general_fuzz.c:783:
+    max_out_size-= copy;
                 ^

ERROR: do not use C99 // comments
#61: FILE: tests/qtest/fuzz/general_fuzz.c:785:
+    // Clear out the

ERROR: spaces required around that '+=' (ctx:VxW)
#66: FILE: tests/qtest/fuzz/general_fuzz.c:790:
+    size+= copy;
         ^

ERROR: spaces required around that '-=' (ctx:VxW)
#67: FILE: tests/qtest/fuzz/general_fuzz.c:791:
+    max_out_size-= copy;
                 ^

ERROR: spaces required around that '+' (ctx:VxV)
#70: FILE: tests/qtest/fuzz/general_fuzz.c:794:
+    memcpy(out+size, SEPARATOR, copy);
               ^

ERROR: spaces required around that '+=' (ctx:VxW)
#71: FILE: tests/qtest/fuzz/general_fuzz.c:795:
+    size+= copy;
         ^

ERROR: spaces required around that '-=' (ctx:VxW)
#72: FILE: tests/qtest/fuzz/general_fuzz.c:796:
+    max_out_size-= copy;
                 ^

ERROR: spaces required around that '+=' (ctx:VxW)
#78: FILE: tests/qtest/fuzz/general_fuzz.c:802:
+    size+= copy;
         ^

ERROR: spaces required around that '-=' (ctx:VxW)
#79: FILE: tests/qtest/fuzz/general_fuzz.c:803:
+    max_out_size-= copy;
                 ^

ERROR: spaces required around that '+' (ctx:VxV)
#82: FILE: tests/qtest/fuzz/general_fuzz.c:806:
+    memcpy(out+size, SEPARATOR, copy);
               ^

ERROR: spaces required around that '+=' (ctx:VxW)
#83: FILE: tests/qtest/fuzz/general_fuzz.c:807:
+    size+= copy;
         ^

ERROR: spaces required around that '-=' (ctx:VxW)
#84: FILE: tests/qtest/fuzz/general_fuzz.c:808:
+    max_out_size-= copy;
                 ^

ERROR: spaces required around that '+' (ctx:VxV)
#87: FILE: tests/qtest/fuzz/general_fuzz.c:811:
+    memcpy(out+size, data2, copy);
               ^

ERROR: spaces required around that '+=' (ctx:VxW)
#88: FILE: tests/qtest/fuzz/general_fuzz.c:812:
+    size+= copy;
         ^

ERROR: spaces required around that '-=' (ctx:VxW)
#89: FILE: tests/qtest/fuzz/general_fuzz.c:813:
+    max_out_size-= copy;
                 ^

total: 22 errors, 0 warnings, 93 lines checked

Patch 9/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

10/15 Checking commit 8973b6e31476 (scripts/oss-fuzz: Add wrapper program for generic fuzzer)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#19: 
new file mode 100644

total: 0 errors, 1 warnings, 40 lines checked

Patch 10/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/15 Checking commit 54db062fafe0 (scripts/oss-fuzz: Add general-fuzzer build script)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#17: 
new file mode 100755

total: 0 errors, 1 warnings, 62 lines checked

Patch 11/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
12/15 Checking commit 559cd365394c (scripts/oss-fuzz: Add general-fuzzer configs for oss-fuzz)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#16: 
new file mode 100644

total: 0 errors, 1 warnings, 103 lines checked

Patch 12/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
13/15 Checking commit 565c5c5cec66 (scripts/oss-fuzz: build the general-fuzzer configs)
14/15 Checking commit ae04d9edfe56 (scripts/oss-fuzz: Add script to reorder a general-fuzzer trace)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#21: 
new file mode 100755

total: 0 errors, 1 warnings, 94 lines checked

Patch 14/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
15/15 Checking commit a8e119d529aa (scripts/oss-fuzz: Add crash trace minimization script)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#16: 
new file mode 100755

total: 0 errors, 1 warnings, 118 lines checked

Patch 15/15 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200819061110.1320568-1-alxndr@bu.edu/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 00/15] Add a General Virtual Device Fuzzer

[Alexander Bulekov]

Oops I forgot to do my checkpatch pass. I'll resend this, shortly.
-Alex

On 200818 2332, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200819061110.1320568-1-alxndr@bu.edu/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 20200819061110.1320568-1-alxndr@bu.edu
> Subject: [PATCH v2 00/15] Add a General Virtual Device Fuzzer
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Switched to a new branch 'test'
> a8e119d scripts/oss-fuzz: Add crash trace minimization script
> ae04d9e scripts/oss-fuzz: Add script to reorder a general-fuzzer trace
> 565c5c5 scripts/oss-fuzz: build the general-fuzzer configs
> 559cd36 scripts/oss-fuzz: Add general-fuzzer configs for oss-fuzz
> 54db062 scripts/oss-fuzz: Add general-fuzzer build script
> 8973b6e scripts/oss-fuzz: Add wrapper program for generic fuzzer
> 3452c68 fuzz: add a crossover function to generic-fuzzer
> 5c579c9 fuzz: add a DISABLE_PCI op to general-fuzzer
> 4f50ecd fuzz: Add support for custom crossover functions
> 95bd76d fuzz: Add fuzzer callbacks to DMA-read functions
> 89e6484 fuzz: Declare DMA Read callback function
> a5441b1 fuzz: Add DMA support to the generic-fuzzer
> 9bd3375 fuzz: Add PCI features to the general fuzzer
> a2759f3 fuzz: Add general virtual-device fuzzer
> f9c6ddd fuzz: Change the way we write qtest log to stderr
> 
> === OUTPUT BEGIN ===
> 1/15 Checking commit f9c6ddda8115 (fuzz: Change the way we write qtest log to stderr)
> 2/15 Checking commit a2759f329ffa (fuzz: Add general virtual-device fuzzer)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #31: 
> new file mode 100644
> 
> ERROR: missing space after enum definition
> #68: FILE: tests/qtest/fuzz/general_fuzz.c:33:
> +enum cmds{
> 
> ERROR: line over 90 characters
> #108: FILE: tests/qtest/fuzz/general_fuzz.c:73:
> +    AddressSpace *as = (io_space == get_system_memory()) ? &address_space_memory : &address_space_io;
> 
> ERROR: line over 90 characters
> #124: FILE: tests/qtest/fuzz/general_fuzz.c:89:
> +                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){
> 
> ERROR: space required before the open brace '{'
> #124: FILE: tests/qtest/fuzz/general_fuzz.c:89:
> +                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){
> 
> ERROR: space required before the open parenthesis '('
> #124: FILE: tests/qtest/fuzz/general_fuzz.c:89:
> +                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){
> 
> ERROR: space required before the open brace '{'
> #128: FILE: tests/qtest/fuzz/general_fuzz.c:93:
> +                    if(mr->size){
> 
> ERROR: space required before the open parenthesis '('
> #128: FILE: tests/qtest/fuzz/general_fuzz.c:93:
> +                    if(mr->size){
> 
> ERROR: spaces required around that '-' (ctx:VxV)
> #131: FILE: tests/qtest/fuzz/general_fuzz.c:96:
> +                    result->len = mr->size-(result->addr-abs_addr);
>                                            ^
> 
> ERROR: spaces required around that '-' (ctx:VxV)
> #131: FILE: tests/qtest/fuzz/general_fuzz.c:96:
> +                    result->len = mr->size-(result->addr-abs_addr);
>                                                          ^
> 
> ERROR: space prohibited between function name and open parenthesis '('
> #483: FILE: tests/qtest/fuzz/general_fuzz.c:448:
> +    char **result = g_strsplit (getenv("QEMU_FUZZ_OBJECTS"), " ", -1);
> 
> ERROR: space required before the open brace '{'
> #500: FILE: tests/qtest/fuzz/general_fuzz.c:465:
> +    if(!fuzzable_memoryregions->len){
> 
> ERROR: space required before the open parenthesis '('
> #500: FILE: tests/qtest/fuzz/general_fuzz.c:465:
> +    if(!fuzzable_memoryregions->len){
> 
> total: 12 errors, 1 warnings, 501 lines checked
> 
> Patch 2/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 3/15 Checking commit 9bd3375b88bc (fuzz: Add PCI features to the general fuzzer)
> 4/15 Checking commit a5441b1099c7 (fuzz: Add DMA support to the generic-fuzzer)
> ERROR: externs should be avoided in .c files
> #84: FILE: tests/qtest/fuzz/general_fuzz.c:97:
> +void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write);
> 
> WARNING: line over 80 characters
> #129: FILE: tests/qtest/fuzz/general_fuzz.c:142:
> +        || (mr != MACHINE(qdev_get_machine())->ram && !(mr->ops == &unassigned_mem_ops))
> 
> total: 1 errors, 1 warnings, 247 lines checked
> 
> Patch 4/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 5/15 Checking commit 89e64845b1dd (fuzz: Declare DMA Read callback function)
> 6/15 Checking commit 95bd76d180c1 (fuzz: Add fuzzer callbacks to DMA-read functions)
> 7/15 Checking commit 4f50ecd4705c (fuzz: Add support for custom crossover functions)
> ERROR: space required before the open parenthesis '('
> #30: FILE: tests/qtest/fuzz/fuzz.c:127:
> +    if(fuzz_target->crossover) {
> 
> WARNING: line over 80 characters
> #59: FILE: tests/qtest/fuzz/fuzz.h:91:
> +     * seed: the seed that should be used to make mutations deterministic, when needed
> 
> total: 1 errors, 1 warnings, 57 lines checked
> 
> Patch 7/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 8/15 Checking commit 5c579c959fec (fuzz: add a DISABLE_PCI op to general-fuzzer)
> ERROR: do not initialise statics to 0 or NULL
> #30: FILE: tests/qtest/fuzz/general_fuzz.c:97:
> +static bool pci_disabled = false;
> 
> total: 1 errors, 0 warnings, 55 lines checked
> 
> Patch 8/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 9/15 Checking commit 3452c68ac630 (fuzz: add a crossover function to generic-fuzzer)
> ERROR: do not use C99 // comments
> #49: FILE: tests/qtest/fuzz/general_fuzz.c:773:
> +    // Copy in the first input
> 
> ERROR: spaces required around that '+' (ctx:VxV)
> #51: FILE: tests/qtest/fuzz/general_fuzz.c:775:
> +    memcpy(out+size, data1, copy);
>                ^
> 
> ERROR: spaces required around that '+=' (ctx:VxW)
> #52: FILE: tests/qtest/fuzz/general_fuzz.c:776:
> +    size+= copy;
>          ^
> 
> ERROR: spaces required around that '-=' (ctx:VxW)
> #53: FILE: tests/qtest/fuzz/general_fuzz.c:777:
> +    max_out_size-= copy;
>                  ^
> 
> ERROR: do not use C99 // comments
> #55: FILE: tests/qtest/fuzz/general_fuzz.c:779:
> +    // Append a separator
> 
> ERROR: spaces required around that '+' (ctx:VxV)
> #57: FILE: tests/qtest/fuzz/general_fuzz.c:781:
> +    memcpy(out+size, SEPARATOR, copy);
>                ^
> 
> ERROR: spaces required around that '+=' (ctx:VxW)
> #58: FILE: tests/qtest/fuzz/general_fuzz.c:782:
> +    size+= copy;
>          ^
> 
> ERROR: spaces required around that '-=' (ctx:VxW)
> #59: FILE: tests/qtest/fuzz/general_fuzz.c:783:
> +    max_out_size-= copy;
>                  ^
> 
> ERROR: do not use C99 // comments
> #61: FILE: tests/qtest/fuzz/general_fuzz.c:785:
> +    // Clear out the
> 
> ERROR: spaces required around that '+=' (ctx:VxW)
> #66: FILE: tests/qtest/fuzz/general_fuzz.c:790:
> +    size+= copy;
>          ^
> 
> ERROR: spaces required around that '-=' (ctx:VxW)
> #67: FILE: tests/qtest/fuzz/general_fuzz.c:791:
> +    max_out_size-= copy;
>                  ^
> 
> ERROR: spaces required around that '+' (ctx:VxV)
> #70: FILE: tests/qtest/fuzz/general_fuzz.c:794:
> +    memcpy(out+size, SEPARATOR, copy);
>                ^
> 
> ERROR: spaces required around that '+=' (ctx:VxW)
> #71: FILE: tests/qtest/fuzz/general_fuzz.c:795:
> +    size+= copy;
>          ^
> 
> ERROR: spaces required around that '-=' (ctx:VxW)
> #72: FILE: tests/qtest/fuzz/general_fuzz.c:796:
> +    max_out_size-= copy;
>                  ^
> 
> ERROR: spaces required around that '+=' (ctx:VxW)
> #78: FILE: tests/qtest/fuzz/general_fuzz.c:802:
> +    size+= copy;
>          ^
> 
> ERROR: spaces required around that '-=' (ctx:VxW)
> #79: FILE: tests/qtest/fuzz/general_fuzz.c:803:
> +    max_out_size-= copy;
>                  ^
> 
> ERROR: spaces required around that '+' (ctx:VxV)
> #82: FILE: tests/qtest/fuzz/general_fuzz.c:806:
> +    memcpy(out+size, SEPARATOR, copy);
>                ^
> 
> ERROR: spaces required around that '+=' (ctx:VxW)
> #83: FILE: tests/qtest/fuzz/general_fuzz.c:807:
> +    size+= copy;
>          ^
> 
> ERROR: spaces required around that '-=' (ctx:VxW)
> #84: FILE: tests/qtest/fuzz/general_fuzz.c:808:
> +    max_out_size-= copy;
>                  ^
> 
> ERROR: spaces required around that '+' (ctx:VxV)
> #87: FILE: tests/qtest/fuzz/general_fuzz.c:811:
> +    memcpy(out+size, data2, copy);
>                ^
> 
> ERROR: spaces required around that '+=' (ctx:VxW)
> #88: FILE: tests/qtest/fuzz/general_fuzz.c:812:
> +    size+= copy;
>          ^
> 
> ERROR: spaces required around that '-=' (ctx:VxW)
> #89: FILE: tests/qtest/fuzz/general_fuzz.c:813:
> +    max_out_size-= copy;
>                  ^
> 
> total: 22 errors, 0 warnings, 93 lines checked
> 
> Patch 9/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 10/15 Checking commit 8973b6e31476 (scripts/oss-fuzz: Add wrapper program for generic fuzzer)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #19: 
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 40 lines checked
> 
> Patch 10/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 11/15 Checking commit 54db062fafe0 (scripts/oss-fuzz: Add general-fuzzer build script)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #17: 
> new file mode 100755
> 
> total: 0 errors, 1 warnings, 62 lines checked
> 
> Patch 11/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 12/15 Checking commit 559cd365394c (scripts/oss-fuzz: Add general-fuzzer configs for oss-fuzz)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #16: 
> new file mode 100644
> 
> total: 0 errors, 1 warnings, 103 lines checked
> 
> Patch 12/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 13/15 Checking commit 565c5c5cec66 (scripts/oss-fuzz: build the general-fuzzer configs)
> 14/15 Checking commit ae04d9edfe56 (scripts/oss-fuzz: Add script to reorder a general-fuzzer trace)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #21: 
> new file mode 100755
> 
> total: 0 errors, 1 warnings, 94 lines checked
> 
> Patch 14/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 15/15 Checking commit a8e119d529aa (scripts/oss-fuzz: Add crash trace minimization script)
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #16: 
> new file mode 100755
> 
> total: 0 errors, 1 warnings, 118 lines checked
> 
> Patch 15/15 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> The full log is available at
> http://patchew.org/logs/20200819061110.1320568-1-alxndr@bu.edu/testing.checkpatch/?type=message.
> ---
> Email generated automatically by Patchew [https://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2 02/15] fuzz: Add general virtual-device fuzzer

[Darren Kenny]

Hi Alex,

Apologies for not taking so long to get to this.

On Wednesday, 2020-08-19 at 02:10:57 -04, Alexander Bulekov wrote:
> This is a generic fuzzer designed to fuzz a virtual device's
> MemoryRegions, as long as they exist within the Memory or Port IO (if it
> exists) AddressSpaces. The fuzzer's input is interpreted into a sequence
> of qtest commands (outb, readw, etc). The interpreted commands are
> separated by a magic seaparator, which should be easy for the fuzzer to
> guess. Without ASan, the separator can be specified as a "dictionary
> value" using the -dict argument (see libFuzzer documentation).
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  tests/qtest/fuzz/Makefile.include |   1 +
>  tests/qtest/fuzz/general_fuzz.c   | 494 ++++++++++++++++++++++++++++++
>  2 files changed, 495 insertions(+)
>  create mode 100644 tests/qtest/fuzz/general_fuzz.c
>
> diff --git a/tests/qtest/fuzz/Makefile.include b/tests/qtest/fuzz/Makefile.include
> index 5bde793bf2..854322efb6 100644
> --- a/tests/qtest/fuzz/Makefile.include
> +++ b/tests/qtest/fuzz/Makefile.include
> @@ -11,6 +11,7 @@ fuzz-obj-y += tests/qtest/fuzz/qtest_wrappers.o
>  fuzz-obj-$(CONFIG_PCI_I440FX) += tests/qtest/fuzz/i440fx_fuzz.o
>  fuzz-obj-$(CONFIG_VIRTIO_NET) += tests/qtest/fuzz/virtio_net_fuzz.o
>  fuzz-obj-$(CONFIG_SCSI) += tests/qtest/fuzz/virtio_scsi_fuzz.o
> +fuzz-obj-y += tests/qtest/fuzz/general_fuzz.o
>  
>  FUZZ_CFLAGS += -I$(SRC_PATH)/tests -I$(SRC_PATH)/tests/qtest
>  
> diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
> new file mode 100644
> index 0000000000..01bcb029b1
> --- /dev/null
> +++ b/tests/qtest/fuzz/general_fuzz.c
> @@ -0,0 +1,494 @@
> +/*
> + * General Virtual-Device Fuzzing Target
> + *
> + * Copyright Red Hat Inc., 2020
> + *
> + * Authors:
> + *  Alexander Bulekov   <alxndr@bu.edu>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include <wordexp.h>
> +
> +#include "cpu.h"
> +#include "tests/qtest/libqtest.h"
> +#include "fuzz.h"
> +#include "fork_fuzz.h"
> +#include "exec/address-spaces.h"
> +#include "string.h"
> +#include "exec/memory.h"
> +#include "exec/ramblock.h"
> +#include "exec/address-spaces.h"
> +#include "hw/qdev-core.h"
> +
> +/*
> + * SEPARATOR is used to separate "operations" in the fuzz input
> + */
> +#define SEPARATOR "FUZZ"
> +
> +enum cmds{
> +    OP_IN,
> +    OP_OUT,
> +    OP_READ,
> +    OP_WRITE,
> +    OP_CLOCK_STEP,
> +};
> +
> +#define DEFAULT_TIMEOUT_US 100000
> +#define USEC_IN_SEC 100000000
> +
> +typedef struct {
> +    size_t addr;
> +    size_t len; /* The number of bytes until the end of the I/O region */
> +} address_range;
> +
> +static useconds_t timeout = 100000;
> +/*
> + * List of memory regions that are children of QOM objects specified by the
> + * user for fuzzing.
> + */
> +static GPtrArray *fuzzable_memoryregions;
> +/*
> + * Here we want to convert a fuzzer-provided [io-region-index, offset] to
> + * a physical address. To do this, we iterate over all of the matched
> + * MemoryRegions. Check whether each region exists within the particular io
> + * space. Return the absolute address of the offset within the index'th region
> + * that is a subregion of the io_space and the distance until the end of the
> + * memory region.
> + */
> +static bool get_io_address(address_range *result,
> +                                    MemoryRegion *io_space,
> +                                    uint8_t index,
> +                                    uint32_t offset) {
> +    MemoryRegion *mr, *root;
> +    index = index % fuzzable_memoryregions->len;
> +    int candidate_regions = 0;
> +    int i = 0;
> +    int ind = index;
> +    size_t abs_addr;
> +    AddressSpace *as = (io_space == get_system_memory()) ? &address_space_memory : &address_space_io;
> +
> +    while (ind >= 0 && fuzzable_memoryregions->len) {
> +        *result = (address_range){0, 0};
> +        mr = g_ptr_array_index(fuzzable_memoryregions, i);
> +        if (mr->enabled) {
> +            abs_addr = mr->addr;
> +            for (root = mr; root->container; ) {
> +                root = root->container;
> +                abs_addr += root->addr;
> +            }
> +            /*
> +             * Only consider the region if it is rooted at the io_space we want
> +             */
> +            if (root == io_space) {
> +                hwaddr xlat, len;
> +                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){
> +                    ind--;

I'm wondering what is the purpose of ind, we never really do anything
with it except possibly decrement it here and test in the while
condition.

With candidate_regions also only being incremented here, we could just
as easily compare that against index.

> +                    candidate_regions++;
> +                    result->addr = abs_addr;
> +                    if(mr->size){
> +                        result->addr += (offset % mr->size);
> +                    }
> +                    result->len = mr->size-(result->addr-abs_addr);

Should we have a break here? If we don't break the look now, *result
will be cleared again at the start of this while block, making this work
here pointless other than counting candidate_regions and decrementing
ind.

> +                }
> +            }
> +        }
> +        ++i;
> +        /* Loop around */
> +        if (i == fuzzable_memoryregions->len) {
> +            /* No enabled regions in our io_space? */
> +            if (candidate_regions == 0) {
> +                break;
> +            }
> +            i = 0;

I'm not really clear on what this block does - it resets i, only breaks
if we've not found any candidates after reaching the end of the
fuzzable_memoryregions array.

Overall, this while loop seems to lack a clear exit strategy, it looks
like where it is doing today is to lopp through  all the
fuzzable_memoryregions multiple times until ind < 0 or we simply don't
find any candidate_regions on our first pass.

Maybe I'm missing something?

> +        }
> +    }
> +    return candidate_regions != 0;
> +}
> +static bool get_pio_address(address_range *result,
> +                                     uint8_t index, uint16_t offset)
> +{
> +    /*
> +     * PIO BARs can be set past the maximum port address (0xFFFF). Thus, result
> +     * can contain an addr that extends past the PIO space. When we pass this
> +     * address to qtest_in/qtest_out, it is cast to a uint16_t, so we might end
> +     * up fuzzing a completely different MemoryRegion/Device. Therefore, check
> +     * that the address here is within the PIO space limits.
> +     */
> +
> +    bool success = get_io_address(result, get_system_io(), index, offset);
> +    return success && result->addr <= 0xFFFF;
> +}
> +static bool get_mmio_address(address_range *result,
> +                                      uint8_t index, uint32_t offset)
> +{
> +    return get_io_address(result, get_system_memory(), index, offset);
> +}
> +
> +static void op_in(QTestState *s, const unsigned char * data, size_t len)
> +{
> +    enum Sizes {Byte, Word, Long, end_sizes};
> +    struct {
> +        uint8_t size;
> +        uint8_t base;
> +        uint16_t offset;
> +    } a;
> +    address_range abs;
> +
> +    if (len < sizeof(a)) {
> +        return;
> +    }
> +    memcpy(&a, data, sizeof(a));
> +    if (get_pio_address(&abs, a.base, a.offset) == 0) {
> +        return;
> +    }
> +
> +    switch (a.size %= end_sizes) {
> +    case Byte:
> +        qtest_inb(s, abs.addr);
> +        break;
> +    case Word:
> +        if (abs.len >= 2) {
> +            qtest_inw(s, abs.addr);
> +        }
> +        break;
> +    case Long:
> +        if (abs.len >= 4) {
> +            qtest_inl(s, abs.addr);
> +        }
> +        break;
> +    }
> +}
> +
> +static void op_out(QTestState *s, const unsigned char * data, size_t len)
> +{
> +    enum Sizes {Byte, Word, Long, end_sizes};
> +    struct {
> +        uint8_t size;
> +        uint8_t base;
> +        uint16_t offset;
> +        uint32_t value;
> +    } a;
> +    address_range abs;
> +
> +    if (len < sizeof(a)) {
> +        return;
> +    }
> +    memcpy(&a, data, sizeof(a));
> +
> +    if (get_pio_address(&abs, a.base, a.offset) == 0) {
> +        return;
> +    }
> +
> +    switch (a.size %= end_sizes) {
> +    case Byte:
> +        qtest_outb(s, abs.addr, a.value & 0xFF);
> +        break;
> +    case Word:
> +        if (abs.len >= 2) {
> +            qtest_outw(s, abs.addr, a.value & 0xFFFF);
> +        }
> +        break;
> +    case Long:
> +        if (abs.len >= 4) {
> +            qtest_outl(s, abs.addr, a.value);
> +        }
> +        break;
> +    }
> +}
> +
> +static void op_read(QTestState *s, const unsigned char * data, size_t len)
> +{
> +    enum Sizes {Byte, Word, Long, Quad, end_sizes};
> +    struct {
> +        uint8_t size;
> +        uint8_t base;
> +        uint32_t offset;
> +    } a;
> +    address_range abs;
> +
> +    if (len < sizeof(a)) {
> +        return;
> +    }
> +    memcpy(&a, data, sizeof(a));
> +
> +    if (get_mmio_address(&abs, a.base, a.offset) == 0) {
> +        return;
> +    }
> +
> +    switch (a.size %= end_sizes) {
> +    case Byte:
> +        qtest_readb(s, abs.addr);
> +        break;
> +    case Word:
> +        if (abs.len >= 2) {
> +            qtest_readw(s, abs.addr);
> +        }
> +        break;
> +    case Long:
> +        if (abs.len >= 4) {
> +            qtest_readl(s, abs.addr);
> +        }
> +        break;
> +    case Quad:
> +        if (abs.len >= 8) {
> +            qtest_readq(s, abs.addr);
> +        }
> +        break;
> +    }
> +}
> +
> +static void op_write(QTestState *s, const unsigned char * data, size_t len)
> +{
> +    enum Sizes {Byte, Word, Long, Quad, end_sizes};
> +    struct {
> +        uint8_t size;
> +        uint8_t base;
> +        uint32_t offset;
> +        uint64_t value;
> +    } a;
> +    address_range abs;
> +
> +    if (len < sizeof(a)) {
> +        return;
> +    }
> +    memcpy(&a, data, sizeof(a));
> +
> +    if (get_mmio_address(&abs, a.base, a.offset) == 0) {
> +        return;
> +    }
> +
> +    switch (a.size %= end_sizes) {
> +    case Byte:
> +            qtest_writeb(s, abs.addr, a.value & 0xFF);
> +        break;
> +    case Word:
> +        if (abs.len >= 2) {
> +            qtest_writew(s, abs.addr, a.value & 0xFFFF);
> +        }
> +        break;
> +    case Long:
> +        if (abs.len >= 4) {
> +            qtest_writel(s, abs.addr, a.value & 0xFFFFFFFF);
> +        }
> +        break;
> +    case Quad:
> +        if (abs.len >= 8) {
> +            qtest_writeq(s, abs.addr, a.value);
> +        }
> +        break;
> +    }
> +}
> +static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
> +{
> +    qtest_clock_step_next(s);
> +}
> +
> +static void handle_timeout(int sig)
> +{
> +    if (getenv("QTEST_LOG")) {
> +        fprintf(stderr, "[Timeout]\n");
> +        fflush(stderr);
> +    }
> +    _Exit(0);
> +}
> +
> +/*
> + * Here, we interpret random bytes from the fuzzer, as a sequence of commands.
> + * Our commands are variable-width, so we use a separator, SEPARATOR, to specify
> + * the boundaries between commands. This is just a random 32-bit value, which
> + * is easily identified by libfuzzer+AddressSanitizer, as long as we use
> + * memmem. It can also be included in the fuzzer's dictionary. More details
> + * here:
> + * https://github.com/google/fuzzing/blob/master/docs/split-inputs.md
> + *
> + * As a result, the stream of bytes is converted into a sequence of commands.
> + * In a simplified example where SEPARATOR is 0xFF:
> + * 00 01 02 FF 03 04 05 06 FF 01 FF ...
> + * becomes this sequence of commands:
> + * 00 01 02    -> op00 (0102)   -> in (0102, 2)
> + * 03 04 05 06 -> op03 (040506) -> write (040506, 3)
> + * 01          -> op01 (-,0)    -> out (-,0)
> + * ...
> + *
> + * Note here that it is the job of the individual opcode functions to check
> + * that enough data was provided. I.e. in the last command out (,0), out needs
> + * to check that there is not enough data provided to select an address/value
> + * for the operation.
> + */

Out if curiosity, do any of our corpus actually make use of the FUZZ string, or are we
just falling back to always using the full size of the provided input?

> +static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
> +{
> +    void (*ops[]) (QTestState *s, const unsigned char* , size_t) = {
> +        [OP_IN]                 = op_in,
> +        [OP_OUT]                = op_out,
> +        [OP_READ]               = op_read,
> +        [OP_WRITE]              = op_write,
> +        [OP_CLOCK_STEP]         = op_clock_step,
> +    };
> +    const unsigned char *cmd = Data;
> +    const unsigned char *nextcmd;
> +    size_t cmd_len;
> +    uint8_t op;
> +
> +    if (fork() == 0) {
> +        /*
> +         * Sometimes the fuzzer will find inputs that take quite a long time to
> +         * process. Often times, these inputs do not result in new coverage.
> +         * Even if these inputs might be interesting, they can slow down the
> +         * fuzzer, overall. Set a timeout to avoid hurting performance, too much
> +         */
> +        if (timeout) {
> +            struct sigaction sact;
> +            struct itimerval timer;
> +
> +            sigemptyset(&sact.sa_mask);
> +            sact.sa_flags   = SA_NODEFER;
> +            sact.sa_handler = handle_timeout;
> +            sigaction(SIGALRM, &sact, NULL);
> +
> +            memset(&timer, 0, sizeof(timer));
> +            timer.it_value.tv_sec = timeout / USEC_IN_SEC;
> +            timer.it_value.tv_usec = timeout % USEC_IN_SEC;
> +            setitimer(ITIMER_VIRTUAL, &timer, NULL);
> +        }
> +
> +        while (cmd && Size) {
> +            /* Get the length until the next command or end of input */
> +            nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
> +            cmd_len = nextcmd ? nextcmd - cmd : Size;
> +
> +            if (cmd_len > 0) {
> +                /* Interpret the first byte of the command as an opcode */
> +                op = *cmd % (sizeof(ops) / sizeof((ops)[0]));
> +                ops[op](s, cmd + 1, cmd_len - 1);
> +
> +                /* Run the main loop */
> +                flush_events(s);
> +            }
> +            /* Advance to the next command */
> +            cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
> +            Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
> +        }
> +        _Exit(0);
> +    } else {
> +        flush_events(s);
> +        wait(0);
> +    }
> +}
> +
> +static void usage(void)
> +{
> +    printf("Please specify the following environment variables:\n");
> +    printf("QEMU_FUZZ_ARGS= the command line arguments passed to qemu\n");
> +    printf("QEMU_FUZZ_OBJECTS= "
> +            "a space separated list of QOM type names for objects to fuzz\n");
> +    printf("Optionally: QEMU_FUZZ_TIMEOUT= Specify a custom timeout (us). "
> +            "0 to disable. %d by default\n", timeout);
> +    exit(0);
> +}
> +
> +static int locate_fuzz_memory_regions(Object *child, void *opaque)
> +{
> +    const char *name;
> +    MemoryRegion *mr;
> +    if (object_dynamic_cast(child, TYPE_MEMORY_REGION)) {
> +        mr = MEMORY_REGION(child);
> +        if ((memory_region_is_ram(mr) ||
> +            memory_region_is_ram_device(mr) ||
> +            memory_region_is_rom(mr) ||
> +            memory_region_is_romd(mr)) == false) {
> +            name = object_get_canonical_path_component(child);
> +            /*
> +             * We don't want duplicate pointers to the same MemoryRegion, so
> +             * try to remove copies of the pointer, before adding it.
> +             */
> +            g_ptr_array_remove_fast(fuzzable_memoryregions, mr);
> +            g_ptr_array_add(fuzzable_memoryregions, mr);
> +        }
> +    }
> +    return 0;
> +}
> +static int locate_fuzz_objects(Object *child, void *opaque)
> +{
> +    char *pattern = opaque;
> +    if (g_pattern_match_simple(pattern, object_get_typename(child))) {
> +        /* Find and save ptrs to any child MemoryRegions */
> +        object_child_foreach_recursive(child, locate_fuzz_memory_regions, NULL);
> +    } else if (object_dynamic_cast(OBJECT(child), TYPE_MEMORY_REGION)) {
> +        if (g_pattern_match_simple(pattern,
> +            object_get_canonical_path_component(child))) {
> +            MemoryRegion *mr;
> +            mr = MEMORY_REGION(child);
> +            if ((memory_region_is_ram(mr) ||
> +                 memory_region_is_ram_device(mr) ||
> +                 memory_region_is_rom(mr) ||
> +                 memory_region_is_romd(mr)) == false) {
> +                g_ptr_array_remove_fast(fuzzable_memoryregions, mr);
> +                g_ptr_array_add(fuzzable_memoryregions, mr);
> +            }
> +        }
> +    }
> +    return 0;
> +}
> +
> +static void general_pre_fuzz(QTestState *s)
> +{
> +    if (!getenv("QEMU_FUZZ_OBJECTS")) {
> +        usage();
> +    }
> +    if (getenv("QEMU_FUZZ_TIMEOUT")) {
> +        timeout = g_ascii_strtoll(getenv("QEMU_FUZZ_TIMEOUT"), NULL, 0);
> +    }
> +
> +    fuzzable_memoryregions = g_ptr_array_new();
> +    char **result = g_strsplit (getenv("QEMU_FUZZ_OBJECTS"), " ", -1);
> +    for (int i = 0; result[i] != NULL; i++) {
> +        printf("Matching objects by name %s\n", result[i]);
> +        object_child_foreach_recursive(qdev_get_machine(),
> +                                    locate_fuzz_objects,
> +                                    result[i]);
> +    }
> +    g_strfreev(result);
> +    printf("This process will try to fuzz the following MemoryRegions:\n");
> +    for (int i = 0; i < fuzzable_memoryregions->len; i++) {
> +        MemoryRegion *mr;
> +        mr = g_ptr_array_index(fuzzable_memoryregions, i);
> +        printf("  * %s (size %lx)\n",
> +               object_get_canonical_path_component(&(mr->parent_obj)),
> +               mr->addr);
> +    }
> +
> +    if(!fuzzable_memoryregions->len){
> +        printf("No fuzzable memory regions found...\n");
> +        exit(0);

Possibly this should exit with a non-zero value since it probably is an
error case?

Thanks,

Darren.

Re: [PATCH v2 03/15] fuzz: Add PCI features to the general fuzzer

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:10:58 -04, Alexander Bulekov wrote:
> This patch compares TYPE_PCI_DEVICE objects against the user-provided
> matching pattern. If there is a match, we use some hacks and leverage
> QOS to map each possible BAR for that device. Now fuzzed inputs might be
> converted to pci_read/write commands which target specific. This means
> that we can fuzz a particular device's PCI configuration space,
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

> ---
>  tests/qtest/fuzz/general_fuzz.c | 83 +++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
>
> diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
> index 01bcb029b1..17b572a439 100644
> --- a/tests/qtest/fuzz/general_fuzz.c
> +++ b/tests/qtest/fuzz/general_fuzz.c
> @@ -24,6 +24,7 @@
>  #include "exec/ramblock.h"
>  #include "exec/address-spaces.h"
>  #include "hw/qdev-core.h"
> +#include "hw/pci/pci.h"
>  
>  /*
>   * SEPARATOR is used to separate "operations" in the fuzz input
> @@ -35,12 +36,17 @@ enum cmds{
>      OP_OUT,
>      OP_READ,
>      OP_WRITE,
> +    OP_PCI_READ,
> +    OP_PCI_WRITE,
>      OP_CLOCK_STEP,
>  };
>  
>  #define DEFAULT_TIMEOUT_US 100000
>  #define USEC_IN_SEC 100000000
>  
> +#define PCI_HOST_BRIDGE_CFG 0xcf8
> +#define PCI_HOST_BRIDGE_DATA 0xcfc
> +
>  typedef struct {
>      size_t addr;
>      size_t len; /* The number of bytes until the end of the I/O region */
> @@ -52,6 +58,8 @@ static useconds_t timeout = 100000;
>   * user for fuzzing.
>   */
>  static GPtrArray *fuzzable_memoryregions;
> +static GPtrArray *fuzzable_pci_devices;
> +
>  /*
>   * Here we want to convert a fuzzer-provided [io-region-index, offset] to
>   * a physical address. To do this, we iterate over all of the matched
> @@ -283,6 +291,65 @@ static void op_write(QTestState *s, const unsigned char * data, size_t len)
>          break;
>      }
>  }
> +static void op_pci_read(QTestState *s, const unsigned char * data, size_t len)
> +{
> +    enum Sizes {Byte, Word, Long, end_sizes};
> +    struct {
> +        uint8_t size;
> +        uint8_t base;
> +        uint8_t offset;
> +    } a;
> +    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
> +        return;
> +    }
> +    memcpy(&a, data, sizeof(a));
> +    PCIDevice *dev = g_ptr_array_index(fuzzable_pci_devices,
> +                                  a.base % fuzzable_pci_devices->len);
> +    int devfn = dev->devfn;
> +    qtest_outl(s, PCI_HOST_BRIDGE_CFG, (1U << 31) | (devfn << 8) | a.offset);
> +    switch (a.size %= end_sizes) {
> +    case Byte:
> +        qtest_inb(s, PCI_HOST_BRIDGE_DATA);
> +        break;
> +    case Word:
> +        qtest_inw(s, PCI_HOST_BRIDGE_DATA);
> +        break;
> +    case Long:
> +        qtest_inl(s, PCI_HOST_BRIDGE_DATA);
> +        break;
> +    }
> +}
> +
> +static void op_pci_write(QTestState *s, const unsigned char * data, size_t len)
> +{
> +    enum Sizes {Byte, Word, Long, end_sizes};
> +    struct {
> +        uint8_t size;
> +        uint8_t base;
> +        uint8_t offset;
> +        uint32_t value;
> +    } a;
> +    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
> +        return;
> +    }
> +    memcpy(&a, data, sizeof(a));
> +    PCIDevice *dev = g_ptr_array_index(fuzzable_pci_devices,
> +                                  a.base % fuzzable_pci_devices->len);
> +    int devfn = dev->devfn;
> +    qtest_outl(s, PCI_HOST_BRIDGE_CFG, (1U << 31) | (devfn << 8) | a.offset);
> +    switch (a.size %= end_sizes) {
> +    case Byte:
> +        qtest_outb(s, PCI_HOST_BRIDGE_DATA, a.value & 0xFF);
> +        break;
> +    case Word:
> +        qtest_outw(s, PCI_HOST_BRIDGE_DATA, a.value & 0xFFFF);
> +        break;
> +    case Long:
> +        qtest_outl(s, PCI_HOST_BRIDGE_DATA, a.value & 0xFFFFFFFF);
> +        break;
> +    }
> +}
> +
>  static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
>  {
>      qtest_clock_step_next(s);
> @@ -327,6 +394,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>          [OP_OUT]                = op_out,
>          [OP_READ]               = op_read,
>          [OP_WRITE]              = op_write,
> +        [OP_PCI_READ]           = op_pci_read,
> +        [OP_PCI_WRITE]          = op_pci_write,
>          [OP_CLOCK_STEP]         = op_clock_step,
>      };
>      const unsigned char *cmd = Data;
> @@ -418,6 +487,19 @@ static int locate_fuzz_objects(Object *child, void *opaque)
>      if (g_pattern_match_simple(pattern, object_get_typename(child))) {
>          /* Find and save ptrs to any child MemoryRegions */
>          object_child_foreach_recursive(child, locate_fuzz_memory_regions, NULL);
> +
> +        /*
> +         * We matched an object. If its a PCI device, store a pointer to it so
> +         * we can map BARs and fuzz its config space.
> +         */
> +        if (object_dynamic_cast(OBJECT(child), TYPE_PCI_DEVICE)) {
> +            /*
> +             * Don't want duplicate pointers to the same PCIDevice, so remove
> +             * copies of the pointer, before adding it.
> +             */
> +            g_ptr_array_remove_fast(fuzzable_pci_devices, PCI_DEVICE(child));
> +            g_ptr_array_add(fuzzable_pci_devices, PCI_DEVICE(child));
> +        }
>      } else if (object_dynamic_cast(OBJECT(child), TYPE_MEMORY_REGION)) {
>          if (g_pattern_match_simple(pattern,
>              object_get_canonical_path_component(child))) {
> @@ -445,6 +527,7 @@ static void general_pre_fuzz(QTestState *s)
>      }
>  
>      fuzzable_memoryregions = g_ptr_array_new();
> +    fuzzable_pci_devices   = g_ptr_array_new();
>      char **result = g_strsplit (getenv("QEMU_FUZZ_OBJECTS"), " ", -1);
>      for (int i = 0; result[i] != NULL; i++) {
>          printf("Matching objects by name %s\n", result[i]);
> -- 
> 2.27.0

Re: [PATCH v2 04/15] fuzz: Add DMA support to the generic-fuzzer

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:10:59 -04, Alexander Bulekov wrote:
> When a virtual-device tries to access some buffer in memory over DMA, we
> add call-backs into the fuzzer(next commit). The fuzzer checks verifies
> that the DMA request maps to a physical RAM address and fills the memory
> with fuzzer-provided data. The patterns that we use to fill this memory
> are specified using add_dma_pattern and clear_dma_patterns operations.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  tests/qtest/fuzz/general_fuzz.c | 178 ++++++++++++++++++++++++++++++++
>  1 file changed, 178 insertions(+)
>
> diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
> index 17b572a439..36d41acea0 100644
> --- a/tests/qtest/fuzz/general_fuzz.c
> +++ b/tests/qtest/fuzz/general_fuzz.c
> @@ -25,6 +25,8 @@
>  #include "exec/address-spaces.h"
>  #include "hw/qdev-core.h"
>  #include "hw/pci/pci.h"
> +#include "hw/boards.h"
> +#include "exec/memory-internal.h"
>  
>  /*
>   * SEPARATOR is used to separate "operations" in the fuzz input
> @@ -38,12 +40,16 @@ enum cmds{
>      OP_WRITE,
>      OP_PCI_READ,
>      OP_PCI_WRITE,
> +    OP_ADD_DMA_PATTERN,
> +    OP_CLEAR_DMA_PATTERNS,
>      OP_CLOCK_STEP,
>  };
>  
>  #define DEFAULT_TIMEOUT_US 100000
>  #define USEC_IN_SEC 100000000
>  
> +#define MAX_DMA_FILL_SIZE 0x10000
> +
>  #define PCI_HOST_BRIDGE_CFG 0xcf8
>  #define PCI_HOST_BRIDGE_DATA 0xcfc
>  
> @@ -53,6 +59,24 @@ typedef struct {
>  } address_range;
>  
>  static useconds_t timeout = 100000;
> +/*
> + * A pattern used to populate a DMA region or perform a memwrite. This is
> + * useful for e.g. populating tables of unique addresses.
> + * Example {.index = 1; .stride = 2; .len = 3; .data = "\x00\x01\x02"}
> + * Renders as: 00 01 02   00 03 03   00 05 03   00 07 03 ...

TYPO: I think this wrong, and that the last digit should be 02 not 03 in
      each group.

> + */
> +typedef struct {
> +    uint8_t index;      /* Index of a byte to increment by stride */
> +    uint8_t stride;     /* Increment each index'th byte by this amount */
> +    size_t len;
> +    const uint8_t *data;
> +} pattern;
> +
> +/* Avoid filling the same DMA region between MMIO/PIO commands ? */
> +static bool avoid_double_fetches;
> +
> +static QTestState *qts_global; /* Need a global for the DMA callback */
> +
>  /*
>   * List of memory regions that are children of QOM objects specified by the
>   * user for fuzzing.
> @@ -60,6 +84,116 @@ static useconds_t timeout = 100000;
>  static GPtrArray *fuzzable_memoryregions;
>  static GPtrArray *fuzzable_pci_devices;
>  
> +/*
> + * List of dma regions populated since the last fuzzing command. Used to ensure
> + * that we only write to each DMA address once, to avoid race conditions when
> + * building reproducers.
> + */
> +static GArray *dma_regions;
> +
> +static GArray *dma_patterns;
> +static int dma_pattern_index;
> +
> +void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write);
> +
> +/*
> + * Allocate a block of memory and populate it with a pattern.
> + */
> +static void *pattern_alloc(pattern p, size_t len)
> +{
> +    int i;
> +    uint8_t *buf = g_malloc(len);
> +    uint8_t sum = 0;
> +
> +    for (i = 0; i < len; ++i) {
> +        buf[i] = p.data[i % p.len];
> +        if ((i % p.len) == p.index) {
> +            buf[i] += sum;
> +            sum += p.stride;
> +        }
> +    }
> +    return buf;
> +}
> +
> +/*
> + * Call-back for functions that perform DMA reads from guest memory. Confirm
> + * that the region has not already been populated since the last loop in
> + * general_fuzz(), avoiding potential race-conditions, which we don't have
> + * a good way for reproducing right now.
> + */
> +void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write)
> +{
> +    /* Are we in the general-fuzzer or are we using another fuzz-target? */
> +    if (!qts_global) {
> +        return;
> +    }
> +
> +    /*
> +     * Return immediately if:
> +     * - We have no DMA patterns defined
> +     * - The length of the DMA read request is zero
> +     * - The DMA read is hitting an MR other than the machine's main RAM
> +     * - The DMA request is not a read (what happens for a address_space_map
> +     *   with is_write=True? Can the device use the same pointer to do reads?)
> +     * - The DMA request hits past the bounds of our RAM
> +     */
> +    if (dma_patterns->len == 0
> +        || len == 0
> +        || (mr != MACHINE(qdev_get_machine())->ram && !(mr->ops == &unassigned_mem_ops))
> +        || is_write
> +        || addr > current_machine->ram_size) {
> +        return;
> +    }
> +
> +    /*
> +     * If we overlap with any existing dma_regions, split the range and only
> +     * populate the non-overlapping parts.
> +     */
> +    for (int i = 0; i < dma_regions->len && avoid_double_fetches; ++i) {
> +        address_range region = g_array_index(dma_regions, address_range, i);

NIT: Can be slightly more expensive to declare a variable on each
iteration, but also tends to be cleaner not to do this.

> +        if (addr < region.addr + region.len && addr + len > region.addr) {
> +            if (addr < region.addr) {
> +                fuzz_dma_read_cb(addr, region.addr - addr, mr, is_write);
> +            }
> +            if (addr + len > region.addr + region.len) {
> +                fuzz_dma_read_cb(region.addr + region.len,
> +                        addr + len - (region.addr + region.len), mr, is_write);
> +            }
> +            return;
> +        }
> +    }
> +
> +    /* Cap the length of the DMA access to something reasonable */
> +    len = MIN(len, MAX_DMA_FILL_SIZE);
> +
> +    address_range ar = {addr, len};
> +    g_array_append_val(dma_regions, ar);
> +    pattern p = g_array_index(dma_patterns, pattern, dma_pattern_index);
> +    void *buf = pattern_alloc(p, ar.len);
> +    if (getenv("QTEST_LOG")) {

NIT: It might be cleaner to put any testing of env vars in to
the code in general_fuzz() where most others are being tested, and
instead set a static global boolean which can be used here instead.
Depending on how many times this is called, it may also be slightly
faster since getenv() has to search an array of strings, etc. to get the
value.

> +        /*
> +         * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log
> +         * that will be written by qtest.c with a DMA tag, so we can reorder
> +         * the resulting QTest trace so the DMA fills precede the last PIO/MMIO
> +         * command.
> +         */
> +        fprintf(stderr, "[DMA] ");
> +        fflush(stderr);
> +        qtest_memwrite(qts_global, ar.addr, buf, ar.len);
> +    } else {
> +       /*
> +        * Populate the region using address_space_write_rom to avoid writing to
> +        * any IO MemoryRegions
> +        */
> +        address_space_write_rom(first_cpu->as, ar.addr, MEMTXATTRS_UNSPECIFIED,
> +                buf, ar.len);
> +    }
> +    free(buf);

NIT: For consistency, this probably should be a g_free(), since the memory
was allocated using g_malloc().

> +
> +    /* Increment the index of the pattern for the next DMA access */
> +    dma_pattern_index = (dma_pattern_index + 1) % dma_patterns->len;
> +}
> +
>  /*
>   * Here we want to convert a fuzzer-provided [io-region-index, offset] to
>   * a physical address. To do this, we iterate over all of the matched
> @@ -350,6 +484,35 @@ static void op_pci_write(QTestState *s, const unsigned char * data, size_t len)
>      }
>  }
>  
> +static void op_add_dma_pattern(QTestState *s,
> +                               const unsigned char *data, size_t len)
> +{
> +    struct {
> +        /*
> +         * index and stride can be used to increment the index-th byte of the
> +         * pattern by the value stride, for each loop of the pattern.
> +         */
> +        uint8_t index;
> +        uint8_t stride;
> +    } a;
> +
> +    if (len < sizeof(a) + 1) {
> +        return;
> +    }
> +    memcpy(&a, data, sizeof(a));
> +    pattern p = {a.index, a.stride, len - sizeof(a), data + sizeof(a)};
> +    p.index = a.index % p.len;
> +    g_array_append_val(dma_patterns, p);
> +    return;
> +}
> +
> +static void op_clear_dma_patterns(QTestState *s,
> +                                  const unsigned char *data, size_t len)
> +{
> +    g_array_set_size(dma_patterns, 0);
> +    dma_pattern_index = 0;
> +}
> +
>  static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
>  {
>      qtest_clock_step_next(s);
> @@ -396,6 +559,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>          [OP_WRITE]              = op_write,
>          [OP_PCI_READ]           = op_pci_read,
>          [OP_PCI_WRITE]          = op_pci_write,
> +        [OP_ADD_DMA_PATTERN]    = op_add_dma_pattern,
> +        [OP_CLEAR_DMA_PATTERNS] = op_clear_dma_patterns,
>          [OP_CLOCK_STEP]         = op_clock_step,
>      };
>      const unsigned char *cmd = Data;
> @@ -425,6 +590,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>              setitimer(ITIMER_VIRTUAL, &timer, NULL);
>          }
>  
> +        op_clear_dma_patterns(s, NULL, 0);
> +
>          while (cmd && Size) {
>              /* Get the length until the next command or end of input */
>              nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
> @@ -441,6 +608,7 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>              /* Advance to the next command */
>              cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
>              Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
> +            g_array_set_size(dma_regions, 0);
>          }
>          _Exit(0);
>      } else {
> @@ -455,6 +623,9 @@ static void usage(void)
>      printf("QEMU_FUZZ_ARGS= the command line arguments passed to qemu\n");
>      printf("QEMU_FUZZ_OBJECTS= "
>              "a space separated list of QOM type names for objects to fuzz\n");
> +    printf("Optionally: QEMU_AVOID_DOUBLE_FETCH= "
> +            "Try to avoid racy DMA double fetch bugs? %d by default\n",
> +            avoid_double_fetches);
>      printf("Optionally: QEMU_FUZZ_TIMEOUT= Specify a custom timeout (us). "
>              "0 to disable. %d by default\n", timeout);
>      exit(0);
> @@ -522,9 +693,16 @@ static void general_pre_fuzz(QTestState *s)
>      if (!getenv("QEMU_FUZZ_OBJECTS")) {
>          usage();
>      }
> +    if (getenv("QEMU_AVOID_DOUBLE_FETCH")) {
> +        avoid_double_fetches = 1;
> +    }
>      if (getenv("QEMU_FUZZ_TIMEOUT")) {
>          timeout = g_ascii_strtoll(getenv("QEMU_FUZZ_TIMEOUT"), NULL, 0);
>      }
> +    qts_global = s;
> +
> +    dma_regions = g_array_new(false, false, sizeof(address_range));
> +    dma_patterns = g_array_new(false, false, sizeof(pattern));
>  
>      fuzzable_memoryregions = g_ptr_array_new();
>      fuzzable_pci_devices   = g_ptr_array_new();

Since mostly nits and typos:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

Re: [PATCH v2 05/15] fuzz: Declare DMA Read callback function

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:00 -04, Alexander Bulekov wrote:
> This patch declares the fuzz_dma_read_cb function and uses the
> preprocessor and linker(weak symbols) to handle these cases:
>
> When we build softmmu/all with --enable-fuzzing, there should be no
> strong symbol defined for fuzz_dma_read_cb, and we link against a weak
> stub function.
>
> When we build softmmu/fuzz with --enable-fuzzing, we link agains the

TYPO: s/agains/against/

> strong symbol in general_fuzz.c
>
> When we build softmmu/all without --enable-fuzzing, fuzz_dma_read_cb is
> an empty, inlined function. As long as we don't call any other functions
> when building the arguments, there should be no overhead.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  include/exec/memory.h | 15 +++++++++++++++
>  softmmu/memory.c      | 13 +++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 307e527835..2ec3b597f1 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -47,6 +47,21 @@
>          OBJECT_GET_CLASS(IOMMUMemoryRegionClass, (obj), \
>                           TYPE_IOMMU_MEMORY_REGION)
>  
> +#ifdef CONFIG_FUZZ
> +void fuzz_dma_read_cb(size_t addr,
> +                      size_t len,
> +                      MemoryRegion *mr,
> +                      bool is_write);
> +#else
> +static inline void fuzz_dma_read_cb(size_t addr,
> +                                    size_t len,
> +                                    MemoryRegion *mr,
> +                                    bool is_write)
> +{
> +    /* Do Nothing */
> +}
> +#endif
> +
>  extern bool global_dirty_log;
>  
>  typedef struct MemoryRegionOps MemoryRegionOps;
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index af25987518..b0c2cf2535 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3223,6 +3223,19 @@ void memory_region_init_rom_device(MemoryRegion *mr,
>      vmstate_register_ram(mr, owner_dev);
>  }
>  
> +/*
> + * Support softmmu builds with CONFIG_FUZZ using a weak symbol and a stub for
> + * the fuzz_dma_read_cb callback
> + */
> +#ifdef CONFIG_FUZZ
> +void __attribute__((weak)) fuzz_dma_read_cb(size_t addr,
> +                      size_t len,
> +                      MemoryRegion *mr,
> +                      bool is_write)
> +{
> +}
> +#endif
> +
>  static const TypeInfo memory_region_info = {
>      .parent             = TYPE_OBJECT,
>      .name               = TYPE_MEMORY_REGION,
> -- 
> 2.27.0

Re: [PATCH v2 06/15] fuzz: Add fuzzer callbacks to DMA-read functions

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:01 -04, Alexander Bulekov wrote:
> We should be careful to not call any functions besides fuzz_dma_read_cb.
> Without --enable-fuzzing, fuzz_dma_read_cb is an empty inlined function.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  exec.c                                | 2 ++
>  include/exec/memory.h                 | 1 +
>  include/exec/memory_ldst_cached.inc.h | 3 +++
>  memory_ldst.inc.c                     | 4 ++++
>  softmmu/memory.c                      | 1 +
>  5 files changed, 11 insertions(+)
>
> diff --git a/exec.c b/exec.c
> index 6f381f98e2..c81f41514d 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3241,6 +3241,7 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
>              stn_he_p(buf, l, val);
>          } else {
>              /* RAM case */
> +            fuzz_dma_read_cb(addr, len, mr, false);
>              ram_ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
>              memcpy(buf, ram_ptr, l);
>          }
> @@ -3601,6 +3602,7 @@ void *address_space_map(AddressSpace *as,
>      memory_region_ref(mr);
>      *plen = flatview_extend_translation(fv, addr, len, mr, xlat,
>                                          l, is_write, attrs);
> +    fuzz_dma_read_cb(addr, *plen, mr, is_write);
>      ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
>  
>      return ptr;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 2ec3b597f1..f8b943521a 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2444,6 +2444,7 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
>                            void *buf, hwaddr len)
>  {
>      assert(addr < cache->len && len <= cache->len - addr);
> +    fuzz_dma_read_cb(cache->xlat + addr, len, cache->mrs.mr, false);
>      if (likely(cache->ptr)) {
>          memcpy(buf, cache->ptr + addr, len);
>          return MEMTX_OK;
> diff --git a/include/exec/memory_ldst_cached.inc.h b/include/exec/memory_ldst_cached.inc.h
> index fd4bbb40e7..aff574039f 100644
> --- a/include/exec/memory_ldst_cached.inc.h
> +++ b/include/exec/memory_ldst_cached.inc.h
> @@ -28,6 +28,7 @@ static inline uint32_t ADDRESS_SPACE_LD_CACHED(l)(MemoryRegionCache *cache,
>      hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
>  {
>      assert(addr < cache->len && 4 <= cache->len - addr);
> +    fuzz_dma_read_cb(cache->xlat + addr, 4, cache->mrs.mr, false);
>      if (likely(cache->ptr)) {
>          return LD_P(l)(cache->ptr + addr);
>      } else {
> @@ -39,6 +40,7 @@ static inline uint64_t ADDRESS_SPACE_LD_CACHED(q)(MemoryRegionCache *cache,
>      hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
>  {
>      assert(addr < cache->len && 8 <= cache->len - addr);
> +    fuzz_dma_read_cb(cache->xlat + addr, 8, cache->mrs.mr, false);
>      if (likely(cache->ptr)) {
>          return LD_P(q)(cache->ptr + addr);
>      } else {
> @@ -50,6 +52,7 @@ static inline uint32_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
>      hwaddr addr, MemTxAttrs attrs, MemTxResult *result)
>  {
>      assert(addr < cache->len && 2 <= cache->len - addr);
> +    fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr, false);
>      if (likely(cache->ptr)) {
>          return LD_P(uw)(cache->ptr + addr);
>      } else {
> diff --git a/memory_ldst.inc.c b/memory_ldst.inc.c
> index c54aee4a95..8d45d2eeff 100644
> --- a/memory_ldst.inc.c
> +++ b/memory_ldst.inc.c
> @@ -42,6 +42,7 @@ static inline uint32_t glue(address_space_ldl_internal, SUFFIX)(ARG1_DECL,
>                                          MO_32 | devend_memop(endian), attrs);
>      } else {
>          /* RAM case */
> +        fuzz_dma_read_cb(addr, 4, mr, false);
>          ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
> @@ -110,6 +111,7 @@ static inline uint64_t glue(address_space_ldq_internal, SUFFIX)(ARG1_DECL,
>                                          MO_64 | devend_memop(endian), attrs);
>      } else {
>          /* RAM case */
> +        fuzz_dma_read_cb(addr, 8, mr, false);
>          ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
> @@ -175,6 +177,7 @@ uint32_t glue(address_space_ldub, SUFFIX)(ARG1_DECL,
>          r = memory_region_dispatch_read(mr, addr1, &val, MO_8, attrs);
>      } else {
>          /* RAM case */
> +        fuzz_dma_read_cb(addr, 1, mr, false);
>          ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
>          val = ldub_p(ptr);
>          r = MEMTX_OK;
> @@ -212,6 +215,7 @@ static inline uint32_t glue(address_space_lduw_internal, SUFFIX)(ARG1_DECL,
>                                          MO_16 | devend_memop(endian), attrs);
>      } else {
>          /* RAM case */
> +        fuzz_dma_read_cb(addr, 2, mr, false);
>          ptr = qemu_map_ram_ptr(mr->ram_block, addr1);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index b0c2cf2535..be87044641 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1405,6 +1405,7 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
>      unsigned size = memop_size(op);
>      MemTxResult r;
>  
> +    fuzz_dma_read_cb(addr, size, mr, false);
>      if (!memory_region_access_valid(mr, addr, size, false, attrs)) {
>          *pval = unassigned_mem_read(mr, addr, size);
>          return MEMTX_DECODE_ERROR;
> -- 
> 2.27.0

Re: [PATCH v2 08/15] fuzz: add a DISABLE_PCI op to general-fuzzer

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:03 -04, Alexander Bulekov wrote:
> This new operation is used in the next commit, which concatenates two
> fuzzer-generated inputs. With this operation, we can prevent the second
> input from clobbering the PCI configuration performed by the first.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  tests/qtest/fuzz/general_fuzz.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
> index 36d41acea0..26fcd69e45 100644
> --- a/tests/qtest/fuzz/general_fuzz.c
> +++ b/tests/qtest/fuzz/general_fuzz.c
> @@ -40,6 +40,7 @@ enum cmds{
>      OP_WRITE,
>      OP_PCI_READ,
>      OP_PCI_WRITE,
> +    OP_DISABLE_PCI,
>      OP_ADD_DMA_PATTERN,
>      OP_CLEAR_DMA_PATTERNS,
>      OP_CLOCK_STEP,
> @@ -93,6 +94,7 @@ static GArray *dma_regions;
>  
>  static GArray *dma_patterns;
>  static int dma_pattern_index;
> +static bool pci_disabled = false;
>  
>  void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write);
>  
> @@ -433,7 +435,7 @@ static void op_pci_read(QTestState *s, const unsigned char * data, size_t len)
>          uint8_t base;
>          uint8_t offset;
>      } a;
> -    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
> +    if (len < sizeof(a) || fuzzable_pci_devices->len == 0 || pci_disabled) {
>          return;
>      }
>      memcpy(&a, data, sizeof(a));
> @@ -463,7 +465,7 @@ static void op_pci_write(QTestState *s, const unsigned char * data, size_t len)
>          uint8_t offset;
>          uint32_t value;
>      } a;
> -    if (len < sizeof(a) || fuzzable_pci_devices->len == 0) {
> +    if (len < sizeof(a) || fuzzable_pci_devices->len == 0 || pci_disabled) {
>          return;
>      }
>      memcpy(&a, data, sizeof(a));
> @@ -518,6 +520,11 @@ static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
>      qtest_clock_step_next(s);
>  }
>  
> +static void op_disable_pci(QTestState *s, const unsigned char *data, size_t len)
> +{
> +    pci_disabled = true;
> +}
> +
>  static void handle_timeout(int sig)
>  {
>      if (getenv("QTEST_LOG")) {
> @@ -559,6 +566,7 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>          [OP_WRITE]              = op_write,
>          [OP_PCI_READ]           = op_pci_read,
>          [OP_PCI_WRITE]          = op_pci_write,
> +        [OP_DISABLE_PCI]        = op_disable_pci,
>          [OP_ADD_DMA_PATTERN]    = op_add_dma_pattern,
>          [OP_CLEAR_DMA_PATTERNS] = op_clear_dma_patterns,
>          [OP_CLOCK_STEP]         = op_clock_step,
> @@ -591,6 +599,7 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>          }
>  
>          op_clear_dma_patterns(s, NULL, 0);
> +        pci_disabled = false;
>  
>          while (cmd && Size) {
>              /* Get the length until the next command or end of input */
> -- 
> 2.27.0

Re: [PATCH v2 07/15] fuzz: Add support for custom crossover functions

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:02 -04, Alexander Bulekov wrote:
> libfuzzer supports a "custom crossover function". Libfuzzer often tries
> to blend two inputs to create a new interesting input. Sometimes, we
> have a better idea about how to blend inputs together. This change
> allows fuzzers to specify a custom function for blending two inputs
> together.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  tests/qtest/fuzz/fuzz.c | 13 +++++++++++++
>  tests/qtest/fuzz/fuzz.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/tests/qtest/fuzz/fuzz.c b/tests/qtest/fuzz/fuzz.c
> index 8234b68754..248fab5f37 100644
> --- a/tests/qtest/fuzz/fuzz.c
> +++ b/tests/qtest/fuzz/fuzz.c
> @@ -118,6 +118,19 @@ static FuzzTarget *fuzz_get_target(char* name)
>  }
>  
>  
> +/* Sometimes called by libfuzzer to mutate two inputs into one */
> +size_t LLVMFuzzerCustomCrossOver(const uint8_t *data1, size_t size1,
> +                                 const uint8_t *data2, size_t size2,
> +                                 uint8_t *out, size_t max_out_size,
> +                                 unsigned int seed)
> +{
> +    if(fuzz_target->crossover) {
> +        return fuzz_target->crossover(data1, size1, data2, size2, out,
> +                                      max_out_size, seed);
> +    }
> +    return 0;
> +}
> +
>  /* Executed for each fuzzing-input */
>  int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size)
>  {
> diff --git a/tests/qtest/fuzz/fuzz.h b/tests/qtest/fuzz/fuzz.h
> index 9ca3d107c5..d36642b5ec 100644
> --- a/tests/qtest/fuzz/fuzz.h
> +++ b/tests/qtest/fuzz/fuzz.h
> @@ -77,6 +77,28 @@ typedef struct FuzzTarget {
>       */
>      void(*fuzz)(QTestState *, const unsigned char *, size_t);
>  
> +    /*
> +     * The fuzzer can specify a "Custom Crossover" function for combining two
> +     * inputs from the corpus. This function is sometimes called by libfuzzer
> +     * when mutating inputs.
> +     *
> +     * data1: location of first input
> +     * size1: length of first input
> +     * data1: location of second input
> +     * size1: length of second input
> +     * out: where to place the resulting, mutated input
> +     * max_out_size: the maximum length of the input that can be placed in out
> +     * seed: the seed that should be used to make mutations deterministic, when needed
> +     *
> +     * See libfuzzer's LLVMFuzzerCustomCrossOver API for more info.
> +     *
> +     * Can be NULL
> +     */
> +    size_t(*crossover)(const uint8_t *data1, size_t size1,
> +                       const uint8_t *data2, size_t size2,
> +                       uint8_t *out, size_t max_out_size,
> +                       unsigned int seed);
> +
>  } FuzzTarget;
>  
>  void flush_events(QTestState *);
> @@ -91,6 +113,10 @@ void fuzz_qtest_set_serialize(bool option);
>   */
>  void fuzz_add_target(const FuzzTarget *target);
>  
> +size_t LLVMFuzzerCustomCrossOver(const uint8_t *data1, size_t size1,
> +                                 const uint8_t *data2, size_t size2,
> +                                 uint8_t *out, size_t max_out_size,
> +                                 unsigned int seed);
>  int LLVMFuzzerTestOneInput(const unsigned char *Data, size_t Size);
>  int LLVMFuzzerInitialize(int *argc, char ***argv, char ***envp);
>  
> -- 
> 2.27.0

Re: [PATCH v2 09/15] fuzz: add a crossover function to generic-fuzzer

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:04 -04, Alexander Bulekov wrote:
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  tests/qtest/fuzz/general_fuzz.c | 81 ++++++++++++++++++++++++++++++++-
>  1 file changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
> index 26fcd69e45..2c3716f8cc 100644
> --- a/tests/qtest/fuzz/general_fuzz.c
> +++ b/tests/qtest/fuzz/general_fuzz.c
> @@ -739,6 +739,83 @@ static void general_pre_fuzz(QTestState *s)
>  
>      counter_shm_init();
>  }
> +
> +/*
> + * When libfuzzer gives us two inputs to combine, return a new input with the
> + * following structure:
> + *
> + * Input 1 (data1)
> + * SEPARATOR
> + * Clear out the DMA Patterns
> + * SEPARATOR
> + * Disable the pci_read/write instructions
> + * SEPARATOR
> + * Input 2 (data2)
> + *
> + * The idea is to collate the core behaviors of the two inputs.
> + * For example:
> + * Input 1: maps a device's BARs, sets up three DMA patterns, and triggers
> + *          device functionality A
> + * Input 2: maps a device's BARs, sets up one DMA pattern, and triggers device
> + *          functionality B
> + *
> + * This function attempts to produce an input that:
> + * Ouptut: maps a device's BARs, set up three DMA patterns, triggers
> + *          functionality A device, replaces the DMA patterns with a single
> + *          patten, and triggers device functionality B.
> + */
> +static size_t general_fuzz_crossover(const uint8_t *data1, size_t size1, const
> +                                     uint8_t *data2, size_t size2, uint8_t *out,
> +                                     size_t max_out_size, unsigned int seed)
> +{

I don't see this function as well documented, but it might be a good
idea to check up front whether out is not NULL and that the max_out_size
is capable of holding what you're likely to consume, i.e. approx:

  size1 + size2 + (SEPARATOR * 3) + 2 /* Ops */

If nothing else means you can be sure you won't have to call MIN() all
the time, but also that you won't end up only partially filling it too.

> +    size_t copy = 0, size = 0;

NIT: Maybe copy should be copy_len or something rather than a verb?

> +
> +    // Copy in the first input
> +    copy = MIN(size1, max_out_size);
> +    memcpy(out+size, data1, copy);
> +    size+= copy;
> +    max_out_size-= copy;
> +
> +    // Append a separator
> +    copy = MIN(strlen(SEPARATOR), max_out_size);
> +    memcpy(out+size, SEPARATOR, copy);
> +    size+= copy;
> +    max_out_size-= copy;
> +
> +    // Clear out the
> +    copy = MIN(1, max_out_size);
> +    if (copy) {
> +        out[size] = OP_CLEAR_DMA_PATTERNS;
> +    }
> +    size+= copy;
> +    max_out_size-= copy;
> +
> +    copy = MIN(strlen(SEPARATOR), max_out_size);
> +    memcpy(out+size, SEPARATOR, copy);
> +    size+= copy;
> +    max_out_size-= copy;
> +
> +    copy = MIN(1, max_out_size);
> +    if (copy) {
> +        out[size] = OP_DISABLE_PCI;
> +    }
> +    size+= copy;
> +    max_out_size-= copy;
> +
> +    copy = MIN(strlen(SEPARATOR), max_out_size);
> +    memcpy(out+size, SEPARATOR, copy);
> +    size+= copy;
> +    max_out_size-= copy;
> +
> +    copy = MIN(size2, max_out_size);
> +    memcpy(out+size, data2, copy);
> +    size+= copy;
> +    max_out_size-= copy;
> +
> +    return  size;
> +}
> +
> +
>  static GString *general_fuzz_cmdline(FuzzTarget *t)
>  {
>      GString *cmd_line = g_string_new(TARGET_NAME);
> @@ -758,7 +835,9 @@ static void register_general_fuzz_targets(void)
>              .description = "Fuzz based on any qemu command-line args. ",
>              .get_init_cmdline = general_fuzz_cmdline,
>              .pre_fuzz = general_pre_fuzz,
> -            .fuzz = general_fuzz});
> +            .fuzz = general_fuzz,
> +            .crossover = general_fuzz_crossover
> +    });
>  }
>  
>  fuzz_target_init(register_general_fuzz_targets);

Thanks,

Darren.

Re: [PATCH v2 10/15] scripts/oss-fuzz: Add wrapper program for generic fuzzer

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:05 -04, Alexander Bulekov wrote:
> On oss-fuzz we need some sort of wrapper to specify command-line
> arguments or environment variables. When we had a similar problem with
> other targets that I fixed with
> 05509c8e6d ("fuzz: select fuzz target using executable name")
> by selecting the fuzz target based on the executable's name. In the
> future should probably commit to one approach (wrapper binary or
> argv0-based target selection).
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  scripts/oss-fuzz/target.c | 40 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 scripts/oss-fuzz/target.c
>
> diff --git a/scripts/oss-fuzz/target.c b/scripts/oss-fuzz/target.c
> new file mode 100644
> index 0000000000..4a7257412a
> --- /dev/null
> +++ b/scripts/oss-fuzz/target.c
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright Red Hat Inc., 2020
> + *
> + * Authors:
> + *  Alexander Bulekov   <alxndr@bu.edu>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <libgen.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +
> +/* Required for oss-fuzz to consider the binary a target. */
> +static const char *magic __attribute__((used)) = "LLVMFuzzerTestOneInput";
> +static const char args[] = {QEMU_FUZZ_ARGS, 0x00};
> +static const char objects[] = {QEMU_FUZZ_OBJECTS, 0x00};
> +
> +int main(int argc, char *argv[])
> +{
> +    char path[PATH_MAX] = {0};
> +    char *dir = dirname(argv[0]);
> +    strncpy(path, dir, PATH_MAX);
> +    strcat(path, "/deps/qemu-fuzz-i386-target-general-fuzz");
> +
> +    setenv("QEMU_FUZZ_ARGS", args, 0);
> +    setenv("QEMU_FUZZ_OBJECTS", objects, 0);
> +
> +    argv[0] = path;
> +    int ret = execvp(path, argv);
> +    if (ret) {
> +        perror("execv");
> +    }
> +    return ret;
> +}
> -- 
> 2.27.0

Re: [PATCH v2 10/15] scripts/oss-fuzz: Add wrapper program for generic fuzzer

[Darren Kenny]

Just thinking after reading later code, that maybe this should be
renamed to something like target.c.tmpl, target_template.c, or similar
so that it is clearer that this is not used directly, but as a template
for generation of other targets.

Thanks,

Darren.


On Wednesday, 2020-08-19 at 02:11:05 -04, Alexander Bulekov wrote:
> On oss-fuzz we need some sort of wrapper to specify command-line
> arguments or environment variables. When we had a similar problem with
> other targets that I fixed with
> 05509c8e6d ("fuzz: select fuzz target using executable name")
> by selecting the fuzz target based on the executable's name. In the
> future should probably commit to one approach (wrapper binary or
> argv0-based target selection).
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  scripts/oss-fuzz/target.c | 40 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 scripts/oss-fuzz/target.c
>
> diff --git a/scripts/oss-fuzz/target.c b/scripts/oss-fuzz/target.c
> new file mode 100644
> index 0000000000..4a7257412a
> --- /dev/null
> +++ b/scripts/oss-fuzz/target.c
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright Red Hat Inc., 2020
> + *
> + * Authors:
> + *  Alexander Bulekov   <alxndr@bu.edu>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <libgen.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +
> +/* Required for oss-fuzz to consider the binary a target. */
> +static const char *magic __attribute__((used)) = "LLVMFuzzerTestOneInput";
> +static const char args[] = {QEMU_FUZZ_ARGS, 0x00};
> +static const char objects[] = {QEMU_FUZZ_OBJECTS, 0x00};
> +
> +int main(int argc, char *argv[])
> +{
> +    char path[PATH_MAX] = {0};
> +    char *dir = dirname(argv[0]);
> +    strncpy(path, dir, PATH_MAX);
> +    strcat(path, "/deps/qemu-fuzz-i386-target-general-fuzz");
> +
> +    setenv("QEMU_FUZZ_ARGS", args, 0);
> +    setenv("QEMU_FUZZ_OBJECTS", objects, 0);
> +
> +    argv[0] = path;
> +    int ret = execvp(path, argv);
> +    if (ret) {
> +        perror("execv");
> +    }
> +    return ret;
> +}
> -- 
> 2.27.0

Re: [PATCH v2 11/15] scripts/oss-fuzz: Add general-fuzzer build script

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:06 -04, Alexander Bulekov wrote:
> This parses a yaml file containing general-fuzzer configs and builds a
> separate oss-fuzz wrapper binary for each one, changing some
> preprocessor macros for each configuration. To avoid dealing with
> escaping and stringifying, convert each string into a byte-array
> representation
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  scripts/oss-fuzz/build_general_fuzzers.py | 62 +++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100755 scripts/oss-fuzz/build_general_fuzzers.py
>
> diff --git a/scripts/oss-fuzz/build_general_fuzzers.py b/scripts/oss-fuzz/build_general_fuzzers.py
> new file mode 100755
> index 0000000000..79f4664117
> --- /dev/null
> +++ b/scripts/oss-fuzz/build_general_fuzzers.py
> @@ -0,0 +1,62 @@
> +#!/usr/bin/env python3
> +# -*- coding: utf-8 -*-
> +
> +"""
> +This script creates wrapper binaries that invoke the general-device-fuzzer with
> +configurations specified in a yaml config file.
> +"""
> +import sys
> +import os
> +import yaml
> +import tempfile
> +
> +CC = ""
> +TEMPLATE = ""
> +
> +
> +def usage():
> +    print("Usage: CC=COMPILER {} CONFIG_PATH \
> +OUTPUT_PATH_PREFIX".format(sys.argv[0]))
> +    sys.exit(0)
> +
> +
> +def str_to_c_byte_array(s):
> +    """
> +    Convert strings to byte-arrays so we don't worry about formatting
> +    strings to play nicely with cc -DQEMU_FUZZARGS etc
> +    """
> +    return ','.join('0x{:02x}'.format(ord(x)) for x in s)
> +
> +
> +def compile_wrapper(cfg, path):
> +    os.system('$CC -DQEMU_FUZZ_ARGS="{}" -DQEMU_FUZZ_OBJECTS="{}" \
> +                {} -o {}'.format(
> +                    str_to_c_byte_array(cfg["args"].replace("\n", " ")),
> +                    str_to_c_byte_array(cfg["objects"].replace("\n", " ")),
> +                    TEMPLATE, path))

NIT: When using multiple placeholders, it is nicer to use names for
them, so that reordering, or adding new ones is easier too.

> +
> +
> +def main():
> +    global CC
> +    global TEMPLATE
> +
> +    if len(sys.argv) != 3:
> +        usage()
> +
> +    cfg_path = sys.argv[1]
> +    out_path = sys.argv[2]
> +
> +    CC = os.getenv("CC")

Maybe provide a fall-back/default value if someone is calling it directly?

> +    TEMPLATE = os.path.join(os.path.dirname(__file__), "target.c")

No harm to double-check this exists, but also I would suggest that the
string "target.c" should be defined as a global value.

> +
> +    with open(cfg_path, "r") as f:
> +        configs = yaml.load(f)["configs"]
> +    for cfg in configs:
> +        assert "name" in cfg
> +        assert "args" in cfg
> +        assert "objects" in cfg
> +        compile_wrapper(cfg, out_path + cfg["name"])
> +
> +
> +if __name__ == '__main__':
> +    main()
> -- 
> 2.27.0

Thanks,

Darren.

Re: [PATCH v2 12/15] scripts/oss-fuzz: Add general-fuzzer configs for oss-fuzz

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:07 -04, Alexander Bulekov wrote:
> Each of these entries is built into a wrapper binary that sets the
> needed environment variables and executes the general virtual-device
> fuzzer. In the future, we will need additional fields, such as arch=arm,
> timeout_per_testcase=0, reset=reboot, etc...
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  scripts/oss-fuzz/general_fuzzer_configs.yml | 103 ++++++++++++++++++++
>  1 file changed, 103 insertions(+)
>  create mode 100644 scripts/oss-fuzz/general_fuzzer_configs.yml
>
> diff --git a/scripts/oss-fuzz/general_fuzzer_configs.yml b/scripts/oss-fuzz/general_fuzzer_configs.yml
> new file mode 100644
> index 0000000000..010e92a2a5
> --- /dev/null
> +++ b/scripts/oss-fuzz/general_fuzzer_configs.yml
> @@ -0,0 +1,103 @@
> +configs:
> +    - name: virtio-net-pci-slirp
> +      args: >
> +        -M q35 -nodefaults
> +        -device virtio-net,netdev=net0 -netdev user,id=net0
> +      objects: virtio*
> +
> +    - name: virtio-blk
> +      args: >
> +        -machine q35 -device virtio-blk,drive=disk0
> +        -drive file=null-co://,id=disk0,if=none,format=raw
> +      objects: virtio*
> +
> +    - name: virtio-scsi
> +      args: >
> +        -machine q35 -device virtio-scsi,num_queues=8
> +        -device scsi-hd,drive=disk0
> +        -drive file=null-co://,id=disk0,if=none,format=raw
> +      objects: scsi* virtio*
> +
> +    - name: virtio-gpu
> +      args: -machine q35 -nodefaults -device virtio-gpu
> +      objects: virtio*
> +
> +    - name: virtio-vga
> +      args: -machine q35 -nodefaults -device virtio-vga
> +      objects: virtio*
> +
> +    - name: virtio-rng
> +      args: -machine q35 -nodefaults -device virtio-rng
> +      objects: virtio*
> +
> +    - name: virtio-balloon
> +      args: -machine q35 -nodefaults -device virtio-balloon
> +      objects: virtio*
> +
> +    - name: virtio-serial
> +      args: -machine q35 -nodefaults -device virtio-serial
> +      objects: virtio*
> +
> +    - name: virtio-mouse
> +      args: -machine q35 -nodefaults -device virtio-mouse
> +      objects: virtio*
> +
> +    - name: e1000
> +      args: >
> +        -M q35 -nodefaults
> +        -device e1000,netdev=net0 -netdev user,id=net0
> +      objects: e1000
> +
> +    - name: e1000e
> +      args: >
> +        -M q35 -nodefaults
> +        -device e1000e,netdev=net0 -netdev user,id=net0
> +      objects: e1000e
> +
> +    - name: cirrus-vga
> +      args: -machine q35 -nodefaults -device cirrus-vga
> +      objects: cirrus*
> +
> +    - name: bochs-display
> +      args: -machine q35 -nodefaults -device bochs-display
> +      objects: bochs*
> +
> +    - name: intel-hda
> +      args: >
> +        -machine q35 -nodefaults -device intel-hda,id=hda0
> +        -device hda-output,bus=hda0.0 -device hda-micro,bus=hda0.0
> +        -device hda-duplex,bus=hda0.0
> +      objects: intel-hda
> +
> +    - name: ide-hd
> +      args: >
> +        -machine q35 -nodefaults
> +        -drive file=null-co://,if=none,format=raw,id=disk0
> +        -device ide-hd,drive=disk0
> +      objects: ahci*
> +
> +    - name: floppy
> +      args: >
> +        -machine pc -nodefaults -device floppy,id=floppy0
> +        -drive id=disk0,file=null-co://,file.read-zeroes=on,if=none
> +        -device floppy,drive=disk0,drive-type=288
> +      objects: fd* floppy*
> +
> +    - name: xhci
> +      args: >
> +        -machine q35 -nodefaults
> +        -drive file=null-co://,if=none,format=raw,id=disk0
> +        -device qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0 -device usb-bot
> +        -device usb-storage,drive=disk0 -chardev null,id=cd0 -chardev null,id=cd1
> +        -device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid
> +        -device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1
> +        -device usb-tablet -device usb-wacom-tablet -device usb-audio
> +      objects: "*usb* *uhci* *xhci*"
> +
> +    - name: pc-i440fx
> +      args: -machine pc
> +      objects: "*"
> +
> +    - name: pc-q35
> +      args: -machine q35
> +      objects: "*"
> -- 
> 2.27.0

Re: [PATCH v2 13/15] scripts/oss-fuzz: build the general-fuzzer configs

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:08 -04, Alexander Bulekov wrote:
> Build general-fuzzer wrappers for each configuration defined in
> general_fuzzer_configs.yml and move the actual general-fuzzer to a
> subdirectory, so oss-fuzz doesn't treat it as a standalone fuzzer.

You didn't mention the removeal of *uhci* from the config below, should
probably be at least referenced.

>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

With that,

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

> ---
>  scripts/oss-fuzz/build.sh                   | 8 +++++++-
>  scripts/oss-fuzz/general_fuzzer_configs.yml | 2 +-
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh
> index a07b3022e8..2071e77ac2 100755
> --- a/scripts/oss-fuzz/build.sh
> +++ b/scripts/oss-fuzz/build.sh
> @@ -38,7 +38,7 @@ OSS_FUZZ_BUILD_DIR="./build-oss-fuzz/"
>  # remove it, resulting in an unresolved reference to qemu_build_not_reached
>  # Undefine the __OPTIMIZE__ macro which compiler.h relies on to choose whether
>  # to " #define qemu_build_not_reached()  g_assert_not_reached() "
> -EXTRA_CFLAGS="$CFLAGS -U __OPTIMIZE__"
> +EXTRA_CFLAGS="$CFLAGS -U __OPTIMIZE__ -DCONFIG_FUZZ=y"
>  
>  if ! { [ -e "./COPYING" ] &&
>     [ -e "./MAINTAINERS" ] &&
> @@ -101,5 +101,11 @@ do
>      cp ./i386-softmmu/qemu-fuzz-i386 "$DEST_DIR/qemu-fuzz-i386-target-$target"
>  done
>  
> +mkdir -p "$DEST_DIR/deps"
> +mv "$DEST_DIR/qemu-fuzz-i386-target-general-fuzz" "$DEST_DIR/deps/"
> +
> +./scripts/oss-fuzz/build_general_fuzzers.py \
> +    "./scripts/oss-fuzz/general_fuzzer_configs.yml" "$DEST_DIR/general-fuzz-"
> +
>  echo "Done. The fuzzers are located in $DEST_DIR"
>  exit 0
> diff --git a/scripts/oss-fuzz/general_fuzzer_configs.yml b/scripts/oss-fuzz/general_fuzzer_configs.yml
> index 010e92a2a5..f70bacb243 100644
> --- a/scripts/oss-fuzz/general_fuzzer_configs.yml
> +++ b/scripts/oss-fuzz/general_fuzzer_configs.yml
> @@ -92,7 +92,7 @@ configs:
>          -device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid
>          -device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1
>          -device usb-tablet -device usb-wacom-tablet -device usb-audio
> -      objects: "*usb* *uhci* *xhci*"
> +      objects: "*usb* *xhci*"
>  
>      - name: pc-i440fx
>        args: -machine pc
> -- 
> 2.27.0

Re: [PATCH v2 14/15] scripts/oss-fuzz: Add script to reorder a general-fuzzer trace

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:09 -04, Alexander Bulekov wrote:
> The general-fuzzer uses hooks to fulfill DMA requests just-in-time.
> This means that if we try to use QTEST_LOG=1 to build a reproducer, the
> DMA writes will be logged _after_ the in/out/read/write that triggered
> the DMA read. To work work around this, the general-fuzzer annotates
> these just-in time DMA fulfilments with a tag that we can use to
> discern them. This script simply iterates over a raw qtest
> trace (including log messages, errors, timestamps etc), filters it and
> re-orders it so that DMA fulfillments are placed directly _before_ the
> qtest command that will cause the DMA access.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
>  .../oss-fuzz/reorder_fuzzer_qtest_trace.py    | 94 +++++++++++++++++++
>  1 file changed, 94 insertions(+)
>  create mode 100755 scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
>
> diff --git a/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py b/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
> new file mode 100755
> index 0000000000..9fb7edb6ee
> --- /dev/null
> +++ b/scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py
> @@ -0,0 +1,94 @@
> +#!/usr/bin/env python3
> +# -*- coding: utf-8 -*-
> +
> +"""
> +Use this to convert qtest log info from a generic fuzzer input into a qtest
> +trace that you can feed into a standard qemu-system process. Example usage:
> +
> +QEMU_FUZZ_ARGS="-machine q35,accel=qtest" QEMU_FUZZ_OBJECTS="*" \
> +        ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=general-pci-fuzz
> +# .. Finds some crash
> +QTEST_LOG=1 FUZZ_SERIALIZE_QTEST=1 \
> +QEMU_FUZZ_ARGS="-machine q35,accel=qtest" QEMU_FUZZ_OBJECTS="*" \
> +        ./i386-softmmu/qemu-fuzz-i386 --fuzz-target=general-pci-fuzz
> +        /path/to/crash 2> qtest_log_output
> +scripts/oss-fuzz/reorder_fuzzer_qtest_trace.py qtest_log_output > qtest_trace
> +./i386-softmmu/qemu-fuzz-i386 -machine q35,accel=qtest \
> +        -qtest stdin < qtest_trace
> +
> +### Details ###
> +
> +Some fuzzer make use of hooks that allow us to populate some memory range, just
> +before a DMA read from that range. This means that the fuzzer can produce
> +activity that looks like:
> +    [start] read from mmio addr
> +    [end]   read from mmio addr
> +    [start] write to pio addr
> +        [start] fill a DMA buffer just in time
> +        [end]   fill a DMA buffer just in time
> +        [start] fill a DMA buffer just in time
> +        [end]   fill a DMA buffer just in time
> +    [end]   write to pio addr
> +    [start] read from mmio addr
> +    [end]   read from mmio addr
> +
> +We annotate these "nested" DMA writes, so with QTEST_LOG=1 the QTest trace
> +might look something like:
> +[R +0.028431] readw 0x10000
> +[R +0.028434] outl 0xc000 0xbeef  # Triggers a DMA read from 0xbeef and 0xbf00
> +[DMA][R +0.034639] write 0xbeef 0x2 0xAAAA
> +[DMA][R +0.034639] write 0xbf00 0x2 0xBBBB
> +[R +0.028431] readw 0xfc000
> +
> +This script would reorder the above trace so it becomes:
> +readw 0x10000
> +write 0xbeef 0x2 0xAAAA
> +write 0xbf00 0x2 0xBBBB
> +outl 0xc000 0xbeef
> +readw 0xfc000
> +
> +I.e. by the time, 0xc000 tries to read from DMA, those DMA buffers have already
> +been set up, removing the need for the DMA hooks. We can simply provide this
> +reordered trace via -qtest stdio to reproduce the input
> +
> +Note: this won't work for traces where the device tries to read from the same
> +DMA region twice in between MMIO/PIO commands. E.g:
> +    [R +0.028434] outl 0xc000 0xbeef
> +    [DMA][R +0.034639] write 0xbeef 0x2 0xAAAA
> +    [DMA][R +0.034639] write 0xbeef 0x2 0xBBBB
> +"""
> +
> +import sys
> +
> +__author__     = "Alexander Bulekov <alxndr@bu.edu>"
> +__copyright__  = "Copyright (C) 2020, Red Hat, Inc."
> +__license__    = "GPL version 2 or (at your option) any later version"
> +
> +__maintainer__ = "Alexander Bulekov"
> +__email__      = "alxndr@bu.edu"
> +
> +
> +def usage():
> +    sys.exit("Usage: {} /path/to/qtest_log_output".format((sys.argv[0])))
> +
> +
> +def main(filename):
> +    with open(filename, "r") as f:
> +        trace = f.readlines()
> +
> +    # Leave only lines that look like logged qtest commands
> +    trace[:] = [x.strip() for x in trace if "[R +" in x
> +                or "[S +" in x and "CLOSED" not in x]
> +
> +    for i in range(len(trace)):
> +        if i+1 < len(trace):
> +            if "[DMA]" in trace[i+1]:
> +                trace[i], trace[i+1] = trace[i+1], trace[i]
> +    for line in trace:
> +        print(line.split("]")[-1].strip())
> +
> +
> +if __name__ == '__main__':
> +    if len(sys.argv) == 1:
> +        usage()
> +    main(sys.argv[1])
> -- 
> 2.27.0

Re: [PATCH v2 15/15] scripts/oss-fuzz: Add crash trace minimization script

[Darren Kenny]

On Wednesday, 2020-08-19 at 02:11:10 -04, Alexander Bulekov wrote:
> Once we find a crash, we can convert it into a QTest trace. Usually this
> trace will contain many operations that are unneeded to reproduce the
> crash. This script tries to minimize the crashing trace, by removing
> operations and trimming QTest bufwrite(write addr len data...) commands.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  scripts/oss-fuzz/minimize_qtest_trace.py | 118 +++++++++++++++++++++++
>  1 file changed, 118 insertions(+)
>  create mode 100755 scripts/oss-fuzz/minimize_qtest_trace.py
>
> diff --git a/scripts/oss-fuzz/minimize_qtest_trace.py b/scripts/oss-fuzz/minimize_qtest_trace.py
> new file mode 100755
> index 0000000000..2f1f4f368e
> --- /dev/null
> +++ b/scripts/oss-fuzz/minimize_qtest_trace.py
> @@ -0,0 +1,118 @@
> +#!/usr/bin/env python3
> +# -*- coding: utf-8 -*-
> +
> +"""
> +This takes a crashing qtest trace and tries to remove superflous operations
> +"""
> +
> +import sys
> +import os
> +import subprocess
> +import time
> +
> +QEMU_ARGS = None
> +QEMU_PATH = None
> +TIMEOUT = 5
> +CRASH_TOKEN = None
> +
> +
> +def usage():
> +    sys.exit("""\
> +Usage: QEMU_PATH="/path/to/qemu" QEMU_ARGS="args" {} input_trace output_trace
> +By default, will try to use the second-to-last line in the output to identify
> +whether the crash occred. Optionally, manually set a string that idenitifes the
> +crash by setting CRASH_TOKEN=
> +""".format((sys.argv[0])))
> +
> +
> +def check_if_trace_crashes(trace, path):
> +    global CRASH_TOKEN
> +    with open(path, "w") as tracefile:
> +        tracefile.write("".join(trace))
> +    rc = subprocess.Popen("timeout -s 9 {}s {} {} 2>&1 < {}".format(TIMEOUT,
> +                          QEMU_PATH, QEMU_ARGS, path),
> +                          shell=True, stdin=subprocess.PIPE,
> +                          stdout=subprocess.PIPE)

NIT: Similar comment to before, it is nicer to name the placeholders if
     there are more than 1 and you can.

> +    stdo = rc.communicate()[0]
> +    output = stdo.decode('unicode_escape')
> +    if rc.returncode == 137:    # Timed Out
> +        return False
> +    if len(output.splitlines()) < 2:
> +        return False
> +
> +    if CRASH_TOKEN is None:
> +        CRASH_TOKEN = output.splitlines()[-2]
> +
> +    return CRASH_TOKEN in output
> +
> +
> +def minimize_trace(inpath, outpath):
> +    global TIMEOUT
> +    with open(inpath) as f:
> +        trace = f.readlines()
> +    start = time.time()
> +    if not check_if_trace_crashes(trace, outpath):
> +        sys.exit("The input qtest trace didn't cause a crash...")
> +    end = time.time()
> +    print("Crashed in {} seconds".format(end-start))
> +    TIMEOUT = (end-start)*5
> +    print("Setting the timeout for {} seconds".format(TIMEOUT))
> +    print("Identifying Crashes by this string: {}".format(CRASH_TOKEN))
> +
> +    i = 0
> +    newtrace = trace[:]
> +    while i < len(newtrace):
> +        prior = newtrace[i]
> +        print("Trying to remove {}".format(newtrace[i]))
> +        # Try to remove the line completely
> +        newtrace[i] = ""
> +        if check_if_trace_crashes(newtrace, outpath):
> +            i += 1
> +            continue
> +        newtrace[i] = prior
> +        # Try to split up writes into multiple commands, each of which can be
> +        # removed.
> +        if newtrace[i].startswith("write "):

NIT: Would be good to document the assumptions here, just in case things
     change in future.

> +            addr = int(newtrace[i].split()[1], 16)
> +            length = int(newtrace[i].split()[2], 16)
> +            data = newtrace[i].split()[3][2:]
> +            if length > 1:
> +                leftlength = int(length/2)
> +                rightlength = length - leftlength
> +                newtrace.insert(i+1, "")
> +                while leftlength > 0:
> +                    newtrace[i] = "write {} {} 0x{}\n".format(
> +                            hex(addr),
> +                            hex(leftlength),
> +                            data[:leftlength*2])
> +                    newtrace[i+1] = "write {} {} 0x{}\n".format(
> +                            hex(addr+leftlength),
> +                            hex(rightlength),
> +                            data[leftlength*2:])

NIT: Similar comment w.r.t. naming the placeholders.

> +                    if check_if_trace_crashes(newtrace, outpath):
> +                        break
> +                    else:
> +                        leftlength -= 1
> +                        rightlength += 1
> +                if check_if_trace_crashes(newtrace, outpath):
> +                    i -= 1
> +                else:
> +                    newtrace[i] = prior
> +                    del newtrace[i+1]
> +        i += 1
> +    check_if_trace_crashes(newtrace, outpath)
> +
> +
> +if __name__ == '__main__':
> +    if len(sys.argv) < 3:
> +        usage()
> +
> +    QEMU_PATH = os.getenv("QEMU_PATH")
> +    QEMU_ARGS = os.getenv("QEMU_ARGS")
> +    if QEMU_PATH is None or QEMU_ARGS is None:
> +        usage()
> +    if "accel" not in QEMU_ARGS:
> +        QEMU_ARGS += " -accel qtest"
> +    CRASH_TOKEN = os.getenv("CRASH_TOKEN")
> +    QEMU_ARGS += " -qtest stdio -monitor none -serial none "
> +    minimize_trace(sys.argv[1], sys.argv[2])

Since all only nits:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

Re: [PATCH v2 02/15] fuzz: Add general virtual-device fuzzer

[Alexander Bulekov]

On 200902 1103, Darren Kenny wrote:
> 
> Hi Alex,
> 
> Apologies for not taking so long to get to this.
> 
> On Wednesday, 2020-08-19 at 02:10:57 -04, Alexander Bulekov wrote:
> > This is a generic fuzzer designed to fuzz a virtual device's
> > MemoryRegions, as long as they exist within the Memory or Port IO (if it
> > exists) AddressSpaces. The fuzzer's input is interpreted into a sequence
> > of qtest commands (outb, readw, etc). The interpreted commands are
> > separated by a magic seaparator, which should be easy for the fuzzer to
> > guess. Without ASan, the separator can be specified as a "dictionary
> > value" using the -dict argument (see libFuzzer documentation).
> >
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> >  tests/qtest/fuzz/Makefile.include |   1 +
> >  tests/qtest/fuzz/general_fuzz.c   | 494 ++++++++++++++++++++++++++++++
> >  2 files changed, 495 insertions(+)
> >  create mode 100644 tests/qtest/fuzz/general_fuzz.c
> >
> > diff --git a/tests/qtest/fuzz/Makefile.include b/tests/qtest/fuzz/Makefile.include
> > index 5bde793bf2..854322efb6 100644
> > --- a/tests/qtest/fuzz/Makefile.include
> > +++ b/tests/qtest/fuzz/Makefile.include
> > @@ -11,6 +11,7 @@ fuzz-obj-y += tests/qtest/fuzz/qtest_wrappers.o
> >  fuzz-obj-$(CONFIG_PCI_I440FX) += tests/qtest/fuzz/i440fx_fuzz.o
> >  fuzz-obj-$(CONFIG_VIRTIO_NET) += tests/qtest/fuzz/virtio_net_fuzz.o
> >  fuzz-obj-$(CONFIG_SCSI) += tests/qtest/fuzz/virtio_scsi_fuzz.o
> > +fuzz-obj-y += tests/qtest/fuzz/general_fuzz.o
> >  
> >  FUZZ_CFLAGS += -I$(SRC_PATH)/tests -I$(SRC_PATH)/tests/qtest
> >  
> > diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
> > new file mode 100644
> > index 0000000000..01bcb029b1
> > --- /dev/null
> > +++ b/tests/qtest/fuzz/general_fuzz.c
> > @@ -0,0 +1,494 @@
> > +/*
> > + * General Virtual-Device Fuzzing Target
> > + *
> > + * Copyright Red Hat Inc., 2020
> > + *
> > + * Authors:
> > + *  Alexander Bulekov   <alxndr@bu.edu>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include <wordexp.h>
> > +
> > +#include "cpu.h"
> > +#include "tests/qtest/libqtest.h"
> > +#include "fuzz.h"
> > +#include "fork_fuzz.h"
> > +#include "exec/address-spaces.h"
> > +#include "string.h"
> > +#include "exec/memory.h"
> > +#include "exec/ramblock.h"
> > +#include "exec/address-spaces.h"
> > +#include "hw/qdev-core.h"
> > +
> > +/*
> > + * SEPARATOR is used to separate "operations" in the fuzz input
> > + */
> > +#define SEPARATOR "FUZZ"
> > +
> > +enum cmds{
> > +    OP_IN,
> > +    OP_OUT,
> > +    OP_READ,
> > +    OP_WRITE,
> > +    OP_CLOCK_STEP,
> > +};
> > +
> > +#define DEFAULT_TIMEOUT_US 100000
> > +#define USEC_IN_SEC 100000000
> > +
> > +typedef struct {
> > +    size_t addr;
> > +    size_t len; /* The number of bytes until the end of the I/O region */
> > +} address_range;
> > +
> > +static useconds_t timeout = 100000;
> > +/*
> > + * List of memory regions that are children of QOM objects specified by the
> > + * user for fuzzing.
> > + */
> > +static GPtrArray *fuzzable_memoryregions;
> > +/*
> > + * Here we want to convert a fuzzer-provided [io-region-index, offset] to
> > + * a physical address. To do this, we iterate over all of the matched
> > + * MemoryRegions. Check whether each region exists within the particular io
> > + * space. Return the absolute address of the offset within the index'th region
> > + * that is a subregion of the io_space and the distance until the end of the
> > + * memory region.
> > + */
> > +static bool get_io_address(address_range *result,
> > +                                    MemoryRegion *io_space,
> > +                                    uint8_t index,
> > +                                    uint32_t offset) {
> > +    MemoryRegion *mr, *root;
> > +    index = index % fuzzable_memoryregions->len;
> > +    int candidate_regions = 0;
> > +    int i = 0;
> > +    int ind = index;
> > +    size_t abs_addr;
> > +    AddressSpace *as = (io_space == get_system_memory()) ? &address_space_memory : &address_space_io;
> > +
> > +    while (ind >= 0 && fuzzable_memoryregions->len) {
> > +        *result = (address_range){0, 0};
> > +        mr = g_ptr_array_index(fuzzable_memoryregions, i);
> > +        if (mr->enabled) {
> > +            abs_addr = mr->addr;
> > +            for (root = mr; root->container; ) {
> > +                root = root->container;
> > +                abs_addr += root->addr;
> > +            }
> > +            /*
> > +             * Only consider the region if it is rooted at the io_space we want
> > +             */
> > +            if (root == io_space) {
> > +                hwaddr xlat, len;
> > +                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){
> > +                    ind--;
> 
> I'm wondering what is the purpose of ind, we never really do anything
> with it except possibly decrement it here and test in the while
> condition.
> 
> With candidate_regions also only being incremented here, we could just
> as easily compare that against index.
> 

Yes it is not clear. The overall idea here is:
* fuzzable_memoryregions contains regions that belong both to the
  Memory/MMIO AddressSpace and the PIO AddressSpace. 
* Thus fuzzable_mr can look like [PIO_1, MMIO_1, MMIO_2, PIO_2, PIO_3]
* If index == 4 and we want an MMIO region, we need to use that as an
  index into the sub-array of only MMIO-Type regions

I think instead, I should either
1. Have separate arrays for PIO/MMIO MRs. This will simplify this
   function, but I'm also not sure whether it is always possible to
   identify whether the mr is pio/mmio (e.g. when a PCI BAR has not yet
   been mapped)
2. Have a single read/write operation instead of in/out and read/write.
   Then, instead of differentiating between MMIO and PIO here, we could
   do that in the OP.
3. Instead of keeping track of MemoryRegions here, try instead to walk
   the corresponding "flatview" and match the memory-region pointers.

I'll try out (3) first. hopefully that will clear this up and make
everything more legible. 

> > +                    candidate_regions++;
> > +                    result->addr = abs_addr;
> > +                    if(mr->size){
> > +                        result->addr += (offset % mr->size);
> > +                    }
> > +                    result->len = mr->size-(result->addr-abs_addr);
> 
> Should we have a break here? If we don't break the look now, *result
> will be cleared again at the start of this while block, making this work
> here pointless other than counting candidate_regions and decrementing
> ind.
> 
> > +                }
> > +            }
> > +        }
> > +        ++i;
> > +        /* Loop around */
> > +        if (i == fuzzable_memoryregions->len) {
> > +            /* No enabled regions in our io_space? */
> > +            if (candidate_regions == 0) {
> > +                break;
> > +            }
> > +            i = 0;
> 
> I'm not really clear on what this block does - it resets i, only breaks
> if we've not found any candidates after reaching the end of the
> fuzzable_memoryregions array.
> 
> Overall, this while loop seems to lack a clear exit strategy, it looks
> like where it is doing today is to lopp through  all the
> fuzzable_memoryregions multiple times until ind < 0 or we simply don't
> find any candidate_regions on our first pass.
> 
> Maybe I'm missing something?
> 
> > +        }
> > +    }
> > +    return candidate_regions != 0;
> > +}
> > +static bool get_pio_address(address_range *result,
> > +                                     uint8_t index, uint16_t offset)
> > +{
> > +    /*
> > +     * PIO BARs can be set past the maximum port address (0xFFFF). Thus, result
> > +     * can contain an addr that extends past the PIO space. When we pass this
> > +     * address to qtest_in/qtest_out, it is cast to a uint16_t, so we might end
> > +     * up fuzzing a completely different MemoryRegion/Device. Therefore, check
> > +     * that the address here is within the PIO space limits.
> > +     */
> > +
> > +    bool success = get_io_address(result, get_system_io(), index, offset);
> > +    return success && result->addr <= 0xFFFF;
> > +}
> > +static bool get_mmio_address(address_range *result,
> > +                                      uint8_t index, uint32_t offset)
> > +{
> > +    return get_io_address(result, get_system_memory(), index, offset);
> > +}
> > +
> > +static void op_in(QTestState *s, const unsigned char * data, size_t len)
> > +{
> > +    enum Sizes {Byte, Word, Long, end_sizes};
> > +    struct {
> > +        uint8_t size;
> > +        uint8_t base;
> > +        uint16_t offset;
> > +    } a;
> > +    address_range abs;
> > +
> > +    if (len < sizeof(a)) {
> > +        return;
> > +    }
> > +    memcpy(&a, data, sizeof(a));
> > +    if (get_pio_address(&abs, a.base, a.offset) == 0) {
> > +        return;
> > +    }
> > +
> > +    switch (a.size %= end_sizes) {
> > +    case Byte:
> > +        qtest_inb(s, abs.addr);
> > +        break;
> > +    case Word:
> > +        if (abs.len >= 2) {
> > +            qtest_inw(s, abs.addr);
> > +        }
> > +        break;
> > +    case Long:
> > +        if (abs.len >= 4) {
> > +            qtest_inl(s, abs.addr);
> > +        }
> > +        break;
> > +    }
> > +}
> > +
> > +static void op_out(QTestState *s, const unsigned char * data, size_t len)
> > +{
> > +    enum Sizes {Byte, Word, Long, end_sizes};
> > +    struct {
> > +        uint8_t size;
> > +        uint8_t base;
> > +        uint16_t offset;
> > +        uint32_t value;
> > +    } a;
> > +    address_range abs;
> > +
> > +    if (len < sizeof(a)) {
> > +        return;
> > +    }
> > +    memcpy(&a, data, sizeof(a));
> > +
> > +    if (get_pio_address(&abs, a.base, a.offset) == 0) {
> > +        return;
> > +    }
> > +
> > +    switch (a.size %= end_sizes) {
> > +    case Byte:
> > +        qtest_outb(s, abs.addr, a.value & 0xFF);
> > +        break;
> > +    case Word:
> > +        if (abs.len >= 2) {
> > +            qtest_outw(s, abs.addr, a.value & 0xFFFF);
> > +        }
> > +        break;
> > +    case Long:
> > +        if (abs.len >= 4) {
> > +            qtest_outl(s, abs.addr, a.value);
> > +        }
> > +        break;
> > +    }
> > +}
> > +
> > +static void op_read(QTestState *s, const unsigned char * data, size_t len)
> > +{
> > +    enum Sizes {Byte, Word, Long, Quad, end_sizes};
> > +    struct {
> > +        uint8_t size;
> > +        uint8_t base;
> > +        uint32_t offset;
> > +    } a;
> > +    address_range abs;
> > +
> > +    if (len < sizeof(a)) {
> > +        return;
> > +    }
> > +    memcpy(&a, data, sizeof(a));
> > +
> > +    if (get_mmio_address(&abs, a.base, a.offset) == 0) {
> > +        return;
> > +    }
> > +
> > +    switch (a.size %= end_sizes) {
> > +    case Byte:
> > +        qtest_readb(s, abs.addr);
> > +        break;
> > +    case Word:
> > +        if (abs.len >= 2) {
> > +            qtest_readw(s, abs.addr);
> > +        }
> > +        break;
> > +    case Long:
> > +        if (abs.len >= 4) {
> > +            qtest_readl(s, abs.addr);
> > +        }
> > +        break;
> > +    case Quad:
> > +        if (abs.len >= 8) {
> > +            qtest_readq(s, abs.addr);
> > +        }
> > +        break;
> > +    }
> > +}
> > +
> > +static void op_write(QTestState *s, const unsigned char * data, size_t len)
> > +{
> > +    enum Sizes {Byte, Word, Long, Quad, end_sizes};
> > +    struct {
> > +        uint8_t size;
> > +        uint8_t base;
> > +        uint32_t offset;
> > +        uint64_t value;
> > +    } a;
> > +    address_range abs;
> > +
> > +    if (len < sizeof(a)) {
> > +        return;
> > +    }
> > +    memcpy(&a, data, sizeof(a));
> > +
> > +    if (get_mmio_address(&abs, a.base, a.offset) == 0) {
> > +        return;
> > +    }
> > +
> > +    switch (a.size %= end_sizes) {
> > +    case Byte:
> > +            qtest_writeb(s, abs.addr, a.value & 0xFF);
> > +        break;
> > +    case Word:
> > +        if (abs.len >= 2) {
> > +            qtest_writew(s, abs.addr, a.value & 0xFFFF);
> > +        }
> > +        break;
> > +    case Long:
> > +        if (abs.len >= 4) {
> > +            qtest_writel(s, abs.addr, a.value & 0xFFFFFFFF);
> > +        }
> > +        break;
> > +    case Quad:
> > +        if (abs.len >= 8) {
> > +            qtest_writeq(s, abs.addr, a.value);
> > +        }
> > +        break;
> > +    }
> > +}
> > +static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
> > +{
> > +    qtest_clock_step_next(s);
> > +}
> > +
> > +static void handle_timeout(int sig)
> > +{
> > +    if (getenv("QTEST_LOG")) {
> > +        fprintf(stderr, "[Timeout]\n");
> > +        fflush(stderr);
> > +    }
> > +    _Exit(0);
> > +}
> > +
> > +/*
> > + * Here, we interpret random bytes from the fuzzer, as a sequence of commands.
> > + * Our commands are variable-width, so we use a separator, SEPARATOR, to specify
> > + * the boundaries between commands. This is just a random 32-bit value, which
> > + * is easily identified by libfuzzer+AddressSanitizer, as long as we use
> > + * memmem. It can also be included in the fuzzer's dictionary. More details
> > + * here:
> > + * https://github.com/google/fuzzing/blob/master/docs/split-inputs.md
> > + *
> > + * As a result, the stream of bytes is converted into a sequence of commands.
> > + * In a simplified example where SEPARATOR is 0xFF:
> > + * 00 01 02 FF 03 04 05 06 FF 01 FF ...
> > + * becomes this sequence of commands:
> > + * 00 01 02    -> op00 (0102)   -> in (0102, 2)
> > + * 03 04 05 06 -> op03 (040506) -> write (040506, 3)
> > + * 01          -> op01 (-,0)    -> out (-,0)
> > + * ...
> > + *
> > + * Note here that it is the job of the individual opcode functions to check
> > + * that enough data was provided. I.e. in the last command out (,0), out needs
> > + * to check that there is not enough data provided to select an address/value
> > + * for the operation.
> > + */
> 
> Out if curiosity, do any of our corpus actually make use of the FUZZ string, or are we
> just falling back to always using the full size of the provided input?
> 

Do you mean if there is some case where "FUZZ" needs to be used as a
real value, rather than a magical separator?

Or are asking whether the fuzzer actually generates inputs with the
"FUZZ" separator?
With ASan enabled, libfuzzer immediately figures out that "FUZZ" is an
interesting string (because it instruments memmem) and starts inserting
it all over the place. Without --enable-sanitizers, I add it to a fuzzer
dictionary for the same effect (see bullet-point 1 in PATCH v2 00/15).

> > +static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
> > +{
> > +    void (*ops[]) (QTestState *s, const unsigned char* , size_t) = {
> > +        [OP_IN]                 = op_in,
> > +        [OP_OUT]                = op_out,
> > +        [OP_READ]               = op_read,
> > +        [OP_WRITE]              = op_write,
> > +        [OP_CLOCK_STEP]         = op_clock_step,
> > +    };
> > +    const unsigned char *cmd = Data;
> > +    const unsigned char *nextcmd;
> > +    size_t cmd_len;
> > +    uint8_t op;
> > +
> > +    if (fork() == 0) {
> > +        /*
> > +         * Sometimes the fuzzer will find inputs that take quite a long time to
> > +         * process. Often times, these inputs do not result in new coverage.
> > +         * Even if these inputs might be interesting, they can slow down the
> > +         * fuzzer, overall. Set a timeout to avoid hurting performance, too much
> > +         */
> > +        if (timeout) {
> > +            struct sigaction sact;
> > +            struct itimerval timer;
> > +
> > +            sigemptyset(&sact.sa_mask);
> > +            sact.sa_flags   = SA_NODEFER;
> > +            sact.sa_handler = handle_timeout;
> > +            sigaction(SIGALRM, &sact, NULL);
> > +
> > +            memset(&timer, 0, sizeof(timer));
> > +            timer.it_value.tv_sec = timeout / USEC_IN_SEC;
> > +            timer.it_value.tv_usec = timeout % USEC_IN_SEC;
> > +            setitimer(ITIMER_VIRTUAL, &timer, NULL);
> > +        }
> > +
> > +        while (cmd && Size) {
> > +            /* Get the length until the next command or end of input */
> > +            nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
> > +            cmd_len = nextcmd ? nextcmd - cmd : Size;
> > +
> > +            if (cmd_len > 0) {
> > +                /* Interpret the first byte of the command as an opcode */
> > +                op = *cmd % (sizeof(ops) / sizeof((ops)[0]));
> > +                ops[op](s, cmd + 1, cmd_len - 1);
> > +
> > +                /* Run the main loop */
> > +                flush_events(s);
> > +            }
> > +            /* Advance to the next command */
> > +            cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
> > +            Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
> > +        }
> > +        _Exit(0);
> > +    } else {
> > +        flush_events(s);
> > +        wait(0);
> > +    }
> > +}
> > +
> > +static void usage(void)
> > +{
> > +    printf("Please specify the following environment variables:\n");
> > +    printf("QEMU_FUZZ_ARGS= the command line arguments passed to qemu\n");
> > +    printf("QEMU_FUZZ_OBJECTS= "
> > +            "a space separated list of QOM type names for objects to fuzz\n");
> > +    printf("Optionally: QEMU_FUZZ_TIMEOUT= Specify a custom timeout (us). "
> > +            "0 to disable. %d by default\n", timeout);
> > +    exit(0);
> > +}
> > +
> > +static int locate_fuzz_memory_regions(Object *child, void *opaque)
> > +{
> > +    const char *name;
> > +    MemoryRegion *mr;
> > +    if (object_dynamic_cast(child, TYPE_MEMORY_REGION)) {
> > +        mr = MEMORY_REGION(child);
> > +        if ((memory_region_is_ram(mr) ||
> > +            memory_region_is_ram_device(mr) ||
> > +            memory_region_is_rom(mr) ||
> > +            memory_region_is_romd(mr)) == false) {
> > +            name = object_get_canonical_path_component(child);
> > +            /*
> > +             * We don't want duplicate pointers to the same MemoryRegion, so
> > +             * try to remove copies of the pointer, before adding it.
> > +             */
> > +            g_ptr_array_remove_fast(fuzzable_memoryregions, mr);
> > +            g_ptr_array_add(fuzzable_memoryregions, mr);
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +static int locate_fuzz_objects(Object *child, void *opaque)
> > +{
> > +    char *pattern = opaque;
> > +    if (g_pattern_match_simple(pattern, object_get_typename(child))) {
> > +        /* Find and save ptrs to any child MemoryRegions */
> > +        object_child_foreach_recursive(child, locate_fuzz_memory_regions, NULL);
> > +    } else if (object_dynamic_cast(OBJECT(child), TYPE_MEMORY_REGION)) {
> > +        if (g_pattern_match_simple(pattern,
> > +            object_get_canonical_path_component(child))) {
> > +            MemoryRegion *mr;
> > +            mr = MEMORY_REGION(child);
> > +            if ((memory_region_is_ram(mr) ||
> > +                 memory_region_is_ram_device(mr) ||
> > +                 memory_region_is_rom(mr) ||
> > +                 memory_region_is_romd(mr)) == false) {
> > +                g_ptr_array_remove_fast(fuzzable_memoryregions, mr);
> > +                g_ptr_array_add(fuzzable_memoryregions, mr);
> > +            }
> > +        }
> > +    }
> > +    return 0;
> > +}
> > +
> > +static void general_pre_fuzz(QTestState *s)
> > +{
> > +    if (!getenv("QEMU_FUZZ_OBJECTS")) {
> > +        usage();
> > +    }
> > +    if (getenv("QEMU_FUZZ_TIMEOUT")) {
> > +        timeout = g_ascii_strtoll(getenv("QEMU_FUZZ_TIMEOUT"), NULL, 0);
> > +    }
> > +
> > +    fuzzable_memoryregions = g_ptr_array_new();
> > +    char **result = g_strsplit (getenv("QEMU_FUZZ_OBJECTS"), " ", -1);
> > +    for (int i = 0; result[i] != NULL; i++) {
> > +        printf("Matching objects by name %s\n", result[i]);
> > +        object_child_foreach_recursive(qdev_get_machine(),
> > +                                    locate_fuzz_objects,
> > +                                    result[i]);
> > +    }
> > +    g_strfreev(result);
> > +    printf("This process will try to fuzz the following MemoryRegions:\n");
> > +    for (int i = 0; i < fuzzable_memoryregions->len; i++) {
> > +        MemoryRegion *mr;
> > +        mr = g_ptr_array_index(fuzzable_memoryregions, i);
> > +        printf("  * %s (size %lx)\n",
> > +               object_get_canonical_path_component(&(mr->parent_obj)),
> > +               mr->addr);
> > +    }
> > +
> > +    if(!fuzzable_memoryregions->len){
> > +        printf("No fuzzable memory regions found...\n");
> > +        exit(0);
> 
> Possibly this should exit with a non-zero value since it probably is an
> error case?

Yes - that would certainly be useful for CI.

Thank you for your reviews, Darren!
-Alex

> 
> Thanks,
> 
> Darren.

Re: [PATCH v2 04/15] fuzz: Add DMA support to the generic-fuzzer

[Alexander Bulekov]

On 200903 0943, Darren Kenny wrote:
> On Wednesday, 2020-08-19 at 02:10:59 -04, Alexander Bulekov wrote:
> > When a virtual-device tries to access some buffer in memory over DMA, we
> > add call-backs into the fuzzer(next commit). The fuzzer checks verifies
> > that the DMA request maps to a physical RAM address and fills the memory
> > with fuzzer-provided data. The patterns that we use to fill this memory
> > are specified using add_dma_pattern and clear_dma_patterns operations.
> >
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> > ---
> >  tests/qtest/fuzz/general_fuzz.c | 178 ++++++++++++++++++++++++++++++++
> >  1 file changed, 178 insertions(+)
> >
> > diff --git a/tests/qtest/fuzz/general_fuzz.c b/tests/qtest/fuzz/general_fuzz.c
> > index 17b572a439..36d41acea0 100644
> > --- a/tests/qtest/fuzz/general_fuzz.c
> > +++ b/tests/qtest/fuzz/general_fuzz.c
> > @@ -25,6 +25,8 @@
> >  #include "exec/address-spaces.h"
> >  #include "hw/qdev-core.h"
> >  #include "hw/pci/pci.h"
> > +#include "hw/boards.h"
> > +#include "exec/memory-internal.h"
> >  
> >  /*
> >   * SEPARATOR is used to separate "operations" in the fuzz input
> > @@ -38,12 +40,16 @@ enum cmds{
> >      OP_WRITE,
> >      OP_PCI_READ,
> >      OP_PCI_WRITE,
> > +    OP_ADD_DMA_PATTERN,
> > +    OP_CLEAR_DMA_PATTERNS,
> >      OP_CLOCK_STEP,
> >  };
> >  
> >  #define DEFAULT_TIMEOUT_US 100000
> >  #define USEC_IN_SEC 100000000
> >  
> > +#define MAX_DMA_FILL_SIZE 0x10000
> > +
> >  #define PCI_HOST_BRIDGE_CFG 0xcf8
> >  #define PCI_HOST_BRIDGE_DATA 0xcfc
> >  
> > @@ -53,6 +59,24 @@ typedef struct {
> >  } address_range;
> >  
> >  static useconds_t timeout = 100000;
> > +/*
> > + * A pattern used to populate a DMA region or perform a memwrite. This is
> > + * useful for e.g. populating tables of unique addresses.
> > + * Example {.index = 1; .stride = 2; .len = 3; .data = "\x00\x01\x02"}
> > + * Renders as: 00 01 02   00 03 03   00 05 03   00 07 03 ...
> 
> TYPO: I think this wrong, and that the last digit should be 02 not 03 in
>       each group.
>

Ah thanks for catching that.

> > + */
> > +typedef struct {
> > +    uint8_t index;      /* Index of a byte to increment by stride */
> > +    uint8_t stride;     /* Increment each index'th byte by this amount */
> > +    size_t len;
> > +    const uint8_t *data;
> > +} pattern;
> > +
> > +/* Avoid filling the same DMA region between MMIO/PIO commands ? */
> > +static bool avoid_double_fetches;
> > +
> > +static QTestState *qts_global; /* Need a global for the DMA callback */
> > +
> >  /*
> >   * List of memory regions that are children of QOM objects specified by the
> >   * user for fuzzing.
> > @@ -60,6 +84,116 @@ static useconds_t timeout = 100000;
> >  static GPtrArray *fuzzable_memoryregions;
> >  static GPtrArray *fuzzable_pci_devices;
> >  
> > +/*
> > + * List of dma regions populated since the last fuzzing command. Used to ensure
> > + * that we only write to each DMA address once, to avoid race conditions when
> > + * building reproducers.
> > + */
> > +static GArray *dma_regions;
> > +
> > +static GArray *dma_patterns;
> > +static int dma_pattern_index;
> > +
> > +void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write);
> > +
> > +/*
> > + * Allocate a block of memory and populate it with a pattern.
> > + */
> > +static void *pattern_alloc(pattern p, size_t len)
> > +{
> > +    int i;
> > +    uint8_t *buf = g_malloc(len);
> > +    uint8_t sum = 0;
> > +
> > +    for (i = 0; i < len; ++i) {
> > +        buf[i] = p.data[i % p.len];
> > +        if ((i % p.len) == p.index) {
> > +            buf[i] += sum;
> > +            sum += p.stride;
> > +        }
> > +    }
> > +    return buf;
> > +}
> > +
> > +/*
> > + * Call-back for functions that perform DMA reads from guest memory. Confirm
> > + * that the region has not already been populated since the last loop in
> > + * general_fuzz(), avoiding potential race-conditions, which we don't have
> > + * a good way for reproducing right now.
> > + */
> > +void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write)
> > +{
> > +    /* Are we in the general-fuzzer or are we using another fuzz-target? */
> > +    if (!qts_global) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * Return immediately if:
> > +     * - We have no DMA patterns defined
> > +     * - The length of the DMA read request is zero
> > +     * - The DMA read is hitting an MR other than the machine's main RAM
> > +     * - The DMA request is not a read (what happens for a address_space_map
> > +     *   with is_write=True? Can the device use the same pointer to do reads?)
> > +     * - The DMA request hits past the bounds of our RAM
> > +     */
> > +    if (dma_patterns->len == 0
> > +        || len == 0
> > +        || (mr != MACHINE(qdev_get_machine())->ram && !(mr->ops == &unassigned_mem_ops))
> > +        || is_write
> > +        || addr > current_machine->ram_size) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * If we overlap with any existing dma_regions, split the range and only
> > +     * populate the non-overlapping parts.
> > +     */
> > +    for (int i = 0; i < dma_regions->len && avoid_double_fetches; ++i) {
> > +        address_range region = g_array_index(dma_regions, address_range, i);
> 
> NIT: Can be slightly more expensive to declare a variable on each
> iteration, but also tends to be cleaner not to do this.

Ok, I'll fix that.

> > +        if (addr < region.addr + region.len && addr + len > region.addr) {
> > +            if (addr < region.addr) {
> > +                fuzz_dma_read_cb(addr, region.addr - addr, mr, is_write);
> > +            }
> > +            if (addr + len > region.addr + region.len) {
> > +                fuzz_dma_read_cb(region.addr + region.len,
> > +                        addr + len - (region.addr + region.len), mr, is_write);
> > +            }
> > +            return;
> > +        }
> > +    }
> > +
> > +    /* Cap the length of the DMA access to something reasonable */
> > +    len = MIN(len, MAX_DMA_FILL_SIZE);
> > +
> > +    address_range ar = {addr, len};
> > +    g_array_append_val(dma_regions, ar);
> > +    pattern p = g_array_index(dma_patterns, pattern, dma_pattern_index);
> > +    void *buf = pattern_alloc(p, ar.len);
> > +    if (getenv("QTEST_LOG")) {
> 
> NIT: It might be cleaner to put any testing of env vars in to
> the code in general_fuzz() where most others are being tested, and
> instead set a static global boolean which can be used here instead.
> Depending on how many times this is called, it may also be slightly
> faster since getenv() has to search an array of strings, etc. to get the
> value.

True. I think the env-variables, in-general, need to be
handled/documented in some consistent location.

> > +        /*
> > +         * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log
> > +         * that will be written by qtest.c with a DMA tag, so we can reorder
> > +         * the resulting QTest trace so the DMA fills precede the last PIO/MMIO
> > +         * command.
> > +         */
> > +        fprintf(stderr, "[DMA] ");
> > +        fflush(stderr);
> > +        qtest_memwrite(qts_global, ar.addr, buf, ar.len);
> > +    } else {
> > +       /*
> > +        * Populate the region using address_space_write_rom to avoid writing to
> > +        * any IO MemoryRegions
> > +        */
> > +        address_space_write_rom(first_cpu->as, ar.addr, MEMTXATTRS_UNSPECIFIED,
> > +                buf, ar.len);
> > +    }
> > +    free(buf);
> 
> NIT: For consistency, this probably should be a g_free(), since the memory
> was allocated using g_malloc().
> 

Oops - I'll fix that.

> > +
> > +    /* Increment the index of the pattern for the next DMA access */
> > +    dma_pattern_index = (dma_pattern_index + 1) % dma_patterns->len;
> > +}
> > +
> >  /*
> >   * Here we want to convert a fuzzer-provided [io-region-index, offset] to
> >   * a physical address. To do this, we iterate over all of the matched
> > @@ -350,6 +484,35 @@ static void op_pci_write(QTestState *s, const unsigned char * data, size_t len)
> >      }
> >  }
> >  
> > +static void op_add_dma_pattern(QTestState *s,
> > +                               const unsigned char *data, size_t len)
> > +{
> > +    struct {
> > +        /*
> > +         * index and stride can be used to increment the index-th byte of the
> > +         * pattern by the value stride, for each loop of the pattern.
> > +         */
> > +        uint8_t index;
> > +        uint8_t stride;
> > +    } a;
> > +
> > +    if (len < sizeof(a) + 1) {
> > +        return;
> > +    }
> > +    memcpy(&a, data, sizeof(a));
> > +    pattern p = {a.index, a.stride, len - sizeof(a), data + sizeof(a)};
> > +    p.index = a.index % p.len;
> > +    g_array_append_val(dma_patterns, p);
> > +    return;
> > +}
> > +
> > +static void op_clear_dma_patterns(QTestState *s,
> > +                                  const unsigned char *data, size_t len)
> > +{
> > +    g_array_set_size(dma_patterns, 0);
> > +    dma_pattern_index = 0;
> > +}
> > +
> >  static void op_clock_step(QTestState *s, const unsigned char *data, size_t len)
> >  {
> >      qtest_clock_step_next(s);
> > @@ -396,6 +559,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
> >          [OP_WRITE]              = op_write,
> >          [OP_PCI_READ]           = op_pci_read,
> >          [OP_PCI_WRITE]          = op_pci_write,
> > +        [OP_ADD_DMA_PATTERN]    = op_add_dma_pattern,
> > +        [OP_CLEAR_DMA_PATTERNS] = op_clear_dma_patterns,
> >          [OP_CLOCK_STEP]         = op_clock_step,
> >      };
> >      const unsigned char *cmd = Data;
> > @@ -425,6 +590,8 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
> >              setitimer(ITIMER_VIRTUAL, &timer, NULL);
> >          }
> >  
> > +        op_clear_dma_patterns(s, NULL, 0);
> > +
> >          while (cmd && Size) {
> >              /* Get the length until the next command or end of input */
> >              nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
> > @@ -441,6 +608,7 @@ static void general_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
> >              /* Advance to the next command */
> >              cmd = nextcmd ? nextcmd + sizeof(SEPARATOR) - 1 : nextcmd;
> >              Size = Size - (cmd_len + sizeof(SEPARATOR) - 1);
> > +            g_array_set_size(dma_regions, 0);
> >          }
> >          _Exit(0);
> >      } else {
> > @@ -455,6 +623,9 @@ static void usage(void)
> >      printf("QEMU_FUZZ_ARGS= the command line arguments passed to qemu\n");
> >      printf("QEMU_FUZZ_OBJECTS= "
> >              "a space separated list of QOM type names for objects to fuzz\n");
> > +    printf("Optionally: QEMU_AVOID_DOUBLE_FETCH= "
> > +            "Try to avoid racy DMA double fetch bugs? %d by default\n",
> > +            avoid_double_fetches);
> >      printf("Optionally: QEMU_FUZZ_TIMEOUT= Specify a custom timeout (us). "
> >              "0 to disable. %d by default\n", timeout);
> >      exit(0);
> > @@ -522,9 +693,16 @@ static void general_pre_fuzz(QTestState *s)
> >      if (!getenv("QEMU_FUZZ_OBJECTS")) {
> >          usage();
> >      }
> > +    if (getenv("QEMU_AVOID_DOUBLE_FETCH")) {
> > +        avoid_double_fetches = 1;
> > +    }
> >      if (getenv("QEMU_FUZZ_TIMEOUT")) {
> >          timeout = g_ascii_strtoll(getenv("QEMU_FUZZ_TIMEOUT"), NULL, 0);
> >      }
> > +    qts_global = s;
> > +
> > +    dma_regions = g_array_new(false, false, sizeof(address_range));
> > +    dma_patterns = g_array_new(false, false, sizeof(pattern));
> >  
> >      fuzzable_memoryregions = g_ptr_array_new();
> >      fuzzable_pci_devices   = g_ptr_array_new();
> 
> Since mostly nits and typos:
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> 
> Thanks,
> 
> Darren.

Re: [PATCH v2 13/15] scripts/oss-fuzz: build the general-fuzzer configs

[Alexander Bulekov]

On 200903 1017, Darren Kenny wrote:
> On Wednesday, 2020-08-19 at 02:11:08 -04, Alexander Bulekov wrote:
> > Build general-fuzzer wrappers for each configuration defined in
> > general_fuzzer_configs.yml and move the actual general-fuzzer to a
> > subdirectory, so oss-fuzz doesn't treat it as a standalone fuzzer.
> 
> You didn't mention the removeal of *uhci* from the config below, should
> probably be at least referenced.

Must have made a mistake when I was fixup/rebasing. Shouldn't be there,
next time around.

Thanks
-Alex

> >
> > Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> 
> With that,
> 
> Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
> 
> Thanks,
> 
> Darren.
> 
> > ---
> >  scripts/oss-fuzz/build.sh                   | 8 +++++++-
> >  scripts/oss-fuzz/general_fuzzer_configs.yml | 2 +-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh
> > index a07b3022e8..2071e77ac2 100755
> > --- a/scripts/oss-fuzz/build.sh
> > +++ b/scripts/oss-fuzz/build.sh
> > @@ -38,7 +38,7 @@ OSS_FUZZ_BUILD_DIR="./build-oss-fuzz/"
> >  # remove it, resulting in an unresolved reference to qemu_build_not_reached
> >  # Undefine the __OPTIMIZE__ macro which compiler.h relies on to choose whether
> >  # to " #define qemu_build_not_reached()  g_assert_not_reached() "
> > -EXTRA_CFLAGS="$CFLAGS -U __OPTIMIZE__"
> > +EXTRA_CFLAGS="$CFLAGS -U __OPTIMIZE__ -DCONFIG_FUZZ=y"
> >  
> >  if ! { [ -e "./COPYING" ] &&
> >     [ -e "./MAINTAINERS" ] &&
> > @@ -101,5 +101,11 @@ do
> >      cp ./i386-softmmu/qemu-fuzz-i386 "$DEST_DIR/qemu-fuzz-i386-target-$target"
> >  done
> >  
> > +mkdir -p "$DEST_DIR/deps"
> > +mv "$DEST_DIR/qemu-fuzz-i386-target-general-fuzz" "$DEST_DIR/deps/"
> > +
> > +./scripts/oss-fuzz/build_general_fuzzers.py \
> > +    "./scripts/oss-fuzz/general_fuzzer_configs.yml" "$DEST_DIR/general-fuzz-"
> > +
> >  echo "Done. The fuzzers are located in $DEST_DIR"
> >  exit 0
> > diff --git a/scripts/oss-fuzz/general_fuzzer_configs.yml b/scripts/oss-fuzz/general_fuzzer_configs.yml
> > index 010e92a2a5..f70bacb243 100644
> > --- a/scripts/oss-fuzz/general_fuzzer_configs.yml
> > +++ b/scripts/oss-fuzz/general_fuzzer_configs.yml
> > @@ -92,7 +92,7 @@ configs:
> >          -device usb-braille,chardev=cd0 -device usb-ccid -device usb-ccid
> >          -device usb-kbd -device usb-mouse -device usb-serial,chardev=cd1
> >          -device usb-tablet -device usb-wacom-tablet -device usb-audio
> > -      objects: "*usb* *uhci* *xhci*"
> > +      objects: "*usb* *xhci*"
> >  
> >      - name: pc-i440fx
> >        args: -machine pc
> > -- 
> > 2.27.0

Re: [PATCH v2 02/15] fuzz: Add general virtual-device fuzzer

[Darren Kenny]

On Monday, 2020-09-07 at 11:39:32 -04, Alexander Bulekov wrote:
> On 200902 1103, Darren Kenny wrote:

...

>> > +
>> > +    while (ind >= 0 && fuzzable_memoryregions->len) {
>> > +        *result = (address_range){0, 0};
>> > +        mr = g_ptr_array_index(fuzzable_memoryregions, i);
>> > +        if (mr->enabled) {
>> > +            abs_addr = mr->addr;
>> > +            for (root = mr; root->container; ) {
>> > +                root = root->container;
>> > +                abs_addr += root->addr;
>> > +            }
>> > +            /*
>> > +             * Only consider the region if it is rooted at the io_space we want
>> > +             */
>> > +            if (root == io_space) {
>> > +                hwaddr xlat, len;
>> > +                if(address_space_translate(as, abs_addr, &xlat, &len, true, MEMTXATTRS_UNSPECIFIED) == mr){
>> > +                    ind--;
>> 
>> I'm wondering what is the purpose of ind, we never really do anything
>> with it except possibly decrement it here and test in the while
>> condition.
>> 
>> With candidate_regions also only being incremented here, we could just
>> as easily compare that against index.
>> 
>
> Yes it is not clear. The overall idea here is:
> * fuzzable_memoryregions contains regions that belong both to the
>   Memory/MMIO AddressSpace and the PIO AddressSpace. 
> * Thus fuzzable_mr can look like [PIO_1, MMIO_1, MMIO_2, PIO_2, PIO_3]
> * If index == 4 and we want an MMIO region, we need to use that as an
>   index into the sub-array of only MMIO-Type regions
>
> I think instead, I should either
> 1. Have separate arrays for PIO/MMIO MRs. This will simplify this
>    function, but I'm also not sure whether it is always possible to
>    identify whether the mr is pio/mmio (e.g. when a PCI BAR has not yet
>    been mapped)
> 2. Have a single read/write operation instead of in/out and read/write.
>    Then, instead of differentiating between MMIO and PIO here, we could
>    do that in the OP.
> 3. Instead of keeping track of MemoryRegions here, try instead to walk
>    the corresponding "flatview" and match the memory-region pointers.
>
> I'll try out (3) first. hopefully that will clear this up and make
> everything more legible. 

OK, thanks.

...

>> > +/*
>> > + * Here, we interpret random bytes from the fuzzer, as a sequence of commands.
>> > + * Our commands are variable-width, so we use a separator, SEPARATOR, to specify
>> > + * the boundaries between commands. This is just a random 32-bit value, which
>> > + * is easily identified by libfuzzer+AddressSanitizer, as long as we use
>> > + * memmem. It can also be included in the fuzzer's dictionary. More details
>> > + * here:
>> > + * https://github.com/google/fuzzing/blob/master/docs/split-inputs.md
>> > + *
>> > + * As a result, the stream of bytes is converted into a sequence of commands.
>> > + * In a simplified example where SEPARATOR is 0xFF:
>> > + * 00 01 02 FF 03 04 05 06 FF 01 FF ...
>> > + * becomes this sequence of commands:
>> > + * 00 01 02    -> op00 (0102)   -> in (0102, 2)
>> > + * 03 04 05 06 -> op03 (040506) -> write (040506, 3)
>> > + * 01          -> op01 (-,0)    -> out (-,0)
>> > + * ...
>> > + *
>> > + * Note here that it is the job of the individual opcode functions to check
>> > + * that enough data was provided. I.e. in the last command out (,0), out needs
>> > + * to check that there is not enough data provided to select an address/value
>> > + * for the operation.
>> > + */
>> 
>> Out if curiosity, do any of our corpus actually make use of the FUZZ string, or are we
>> just falling back to always using the full size of the provided input?
>> 
>
> Do you mean if there is some case where "FUZZ" needs to be used as a
> real value, rather than a magical separator?
>
> Or are asking whether the fuzzer actually generates inputs with the
> "FUZZ" separator?
> With ASan enabled, libfuzzer immediately figures out that "FUZZ" is an
> interesting string (because it instruments memmem) and starts inserting
> it all over the place. Without --enable-sanitizers, I add it to a fuzzer
> dictionary for the same effect (see bullet-point 1 in PATCH v2 00/15).

Should have responded to this, saw that you also used FUZZ later in the
patchset when I finally got there :)

Thanks,

Darren.