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

[Alexander Bulekov]

On 201001 1629, Darren Kenny wrote:
> 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.
> 

Hi Darren,
If I move the definition of SEPARATOR to Patch 9, I would need some
different way to parse commands here, to keep everything bisectable. I
don't think the separator is only important in the context of the
Crossover functionality (Patch 9) - it is useful in general as a
"stable" way to parse an input into multiple commands.
Is it OK if I keep SEPARATOR in this patch and add the comments you
mention to both this patch and patch 9?
Thanks
-Alex

> >> 
> >> 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;
> >> [...]
> >>