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

[Darren Kenny <darren.kenny@oracle.com>]

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.