Re: [Qemu-devel] [PATCH v3 20/22] fuzz: add i440fx fuzz targets

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

On Wed, Sep 18, 2019 at 11:19:47PM +0000, Oleinik, Alexander wrote:
> +static void i440fx_fuzz_qtest(QTestState *s,
> +        const unsigned char *Data, size_t Size) {
> +
> +    typedef struct QTestFuzzAction {
> +        uint8_t id;
> +        uint8_t addr;
> +        uint32_t value;
> +    } QTestFuzzAction;

I'm concerned about padding within the struct and struct alignment
causing us to skip some bytes in Data[].  Also, on some architectures
unaligned memory accesses are unsupported so memcpy() is safer than
casting a pointer directly into Data[].

The question is whether skipping bytes in Data[] matters.
Feedback-directed fuzzing should realize that certain offsets in Data[]
are unused and do not affect the input space.  Still, I think we should
arrange things so that every bit in Data[] matters as much as possible.

The struct can be arranged to avoid struct field padding:

  uint32_t value;
  uint8_t id;
  uint8_t addr;

> +    QTestFuzzAction *a = (QTestFuzzAction *)Data;
> +    while (Size >= sizeof(QTestFuzzAction)) {

To avoid struct alignment issues:

  QTestFuzzAction a;
  while (Size >= sizeof(a)) {
      memcpy(&a, Data, sizeof(a));
      ...
      Data += sizeof(a);
  }

> +        uint16_t addr = a->addr % 2 ? 0xcf8 : 0xcfc;

Please define constants for these magic numbers (e.g.
I440FX_PCI_HOST_BRIDGE_CFG, I440FX_PCI_HOST_BRIDGE_DATA).

> +        switch (a->id) {

How about:

  switch (a->id % ACTION_MAX) {

This way we always exercise a useful code path instead of just skipping
the input.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]