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.