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.