Re: [PATCH v3 02/16] fuzz: Add general virtual-device fuzzer

[Darren Kenny]

Hi Alex,

On Monday, 2020-09-21 at 10:34:05 -04, Alexander Bulekov wrote:
> On 200921 0743, Philippe Mathieu-Daudé wrote:
>> Hi Alexander,
>> 
>> On 9/21/20 4:24 AM, 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/general_fuzz.c | 498 ++++++++++++++++++++++++++++++++
>> >  tests/qtest/fuzz/meson.build    |   1 +
>> >  2 files changed, 499 insertions(+)
>> >  create mode 100644 tests/qtest/fuzz/general_fuzz.c
>> > 
>> > diff --git a/tests/qtest/fuzz/general_fuzz.c 
>> > b/tests/qtest/fuzz/general_fuzz.c
>> > new file mode 100644
>> > index 0000000000..bf75b215ca
>> > --- /dev/null
>> > +++ b/tests/qtest/fuzz/general_fuzz.c
>> > @@ -0,0 +1,498 @@
>> > +/*
>> > + * 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 "hw/core/cpu.h"
>> > +#include "tests/qtest/libqos/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"
>> 
>> Why use a separator when all pkt sizes are known?
> Good point. 
> 1. When we add the DMA Pattern OP in patch 04/16, we now have
> variable-width OPs.
> 2. Even when everything has a known size, take for example the input:
> Acb Bd Caaaa Effff
> Where Operation A has size 3, B: size 2, C size 5 ...:
> Simply by removing the first byte, we now have a completely different
> sequence of operations:
> Cbbdc Aaa Aef Ff...
> Thus the separators "add some stability" to random mutations:
> Cb FUZZ Bd FUZZ Caaaa FUZZ Effff ...
> (Cb is now invalid/ignored, but the rest of the commands are still
> intact)
> There is some libfuzzer documentation about this technique:
> https://github.com/google/fuzzing/blob/master/docs/split-inputs.md#magic-separator
>
> There is also a promising "FuzzDataProvider" header library that lets
> you directly call functions, such as ConsumeBytes, or
> ConsumeIntegralInRange, but unfortunately it is a C++ header.

It might make sense to put the definition of SEPARATOR and some variant
of the above the comments in patch 9 where you're adding this related
functionality?

It seems a little out of place here.

Thanks,

Darren.

>> 
>> Can you fuzz writing "FUZZ" in memory? Like:
>> OP_WRITE(0x100000, "UsingLibFUZZerString")?
>
> No.. Hopefully that's not a huge problem.
>
>> > +
>> > +enum cmds {
>> > +    OP_IN,
>> > +    OP_OUT,
>> > +    OP_READ,
>> > +    OP_WRITE,
>> > +    OP_CLOCK_STEP,
>> > +};
>> > +
>> > +#define DEFAULT_TIMEOUT_US 100000
>> > +#define USEC_IN_SEC 100000000
>> 
>> Are you sure this definition is correct?
>> 
> Thanks for the catch...
>
>> > +
>> > +typedef struct {
>> > +    ram_addr_t addr;
>> > +    ram_addr_t size; /* The number of bytes until the end of the I/O 
>> > region */
>> > +} address_range;
>> > +
>> > +static useconds_t timeout = 100000;
>> [...]
>>