[PATCH v3 0/4] Miscellaneous fuzzer changes

[PATCH v3 0/4] Miscellaneous fuzzer changes

[Alexander Bulekov]

v3:
    - Check in ./configure whether clang supports -fsanitize-coverage-allowlist
v2:
    - Add the instrumentation filter to the instrumentation filter patch

These patches
1.) Change generic-fuzzer timeouts so they are reconfigured prior to
each individual IO command, to allow for longer-running inputs
2.) Add an instrumentation filter to prevent libfuzzer from tracking
noisy/irrelevant parts of the code.
3.) Fix the AC97 and ES1370 fuzzer configs

Alexander Bulekov (4):
  fuzz: adjust timeout to allow for longer inputs
  fuzz: add an instrumentation filter
  fuzz: fix the AC97 generic-fuzzer config.
  fuzz: fix the ES1370 generic-fuzzer config.

 configure                               | 11 +++++++++++
 scripts/oss-fuzz/instrumentation-filter | 14 ++++++++++++++
 tests/qtest/fuzz/generic_fuzz.c         | 13 +++++++++----
 tests/qtest/fuzz/generic_fuzz_configs.h |  4 ++--
 4 files changed, 36 insertions(+), 6 deletions(-)
 create mode 100644 scripts/oss-fuzz/instrumentation-filter

-- 
2.28.0

[PATCH v3 1/4] fuzz: adjust timeout to allow for longer inputs

[Alexander Bulekov]

Using a custom timeout is useful to continue fuzzing complex devices,
even after we run into some slow code-path. However, simply adding a
fixed timeout to each input effectively caps the maximum input
length/number of operations at some artificial value. There are two
major problems with this:
1. Some code might only be reachable through long IO sequences.
2. Longer inputs can actually be _better_ for performance. While the
   raw number of fuzzer executions decreases with larger inputs, the
   number of MMIO/PIO/DMA operation/second actually increases, since
   were are speding proportionately less time fork()ing.

With this change, we keep the custom-timeout, but we renew it, prior to
each MMIO/PIO/DMA operation. Thus, we time-out only when a particaly
operation takes a long time.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/generic_fuzz.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
index cea7d4058e..71d36e8f6f 100644
--- a/tests/qtest/fuzz/generic_fuzz.c
+++ b/tests/qtest/fuzz/generic_fuzz.c
@@ -661,15 +661,16 @@ static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
     uint8_t op;
 
     if (fork() == 0) {
+        struct sigaction sact;
+        struct itimerval timer;
         /*
          * Sometimes the fuzzer will find inputs that take quite a long time to
          * process. Often times, these inputs do not result in new coverage.
          * Even if these inputs might be interesting, they can slow down the
-         * fuzzer, overall. Set a timeout to avoid hurting performance, too much
+         * fuzzer, overall. Set a timeout for each command to avoid hurting
+         * performance, too much
          */
         if (timeout) {
-            struct sigaction sact;
-            struct itimerval timer;
 
             sigemptyset(&sact.sa_mask);
             sact.sa_flags   = SA_NODEFER;
@@ -679,13 +680,17 @@ static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
             memset(&timer, 0, sizeof(timer));
             timer.it_value.tv_sec = timeout / USEC_IN_SEC;
             timer.it_value.tv_usec = timeout % USEC_IN_SEC;
-            setitimer(ITIMER_VIRTUAL, &timer, NULL);
         }
 
         op_clear_dma_patterns(s, NULL, 0);
         pci_disabled = false;
 
         while (cmd && Size) {
+            /* Reset the timeout, each time we run a new command */
+            if (timeout) {
+                setitimer(ITIMER_VIRTUAL, &timer, NULL);
+            }
+
             /* Get the length until the next command or end of input */
             nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
             cmd_len = nextcmd ? nextcmd - cmd : Size;
-- 
2.28.0

Re: [PATCH v3 1/4] fuzz: adjust timeout to allow for longer inputs

[Darren Kenny]

On Wednesday, 2021-06-23 at 23:45:00 -04, Alexander Bulekov wrote:
> Using a custom timeout is useful to continue fuzzing complex devices,
> even after we run into some slow code-path. However, simply adding a
> fixed timeout to each input effectively caps the maximum input
> length/number of operations at some artificial value. There are two
> major problems with this:
> 1. Some code might only be reachable through long IO sequences.
> 2. Longer inputs can actually be _better_ for performance. While the
>    raw number of fuzzer executions decreases with larger inputs, the
>    number of MMIO/PIO/DMA operation/second actually increases, since
>    were are speding proportionately less time fork()ing.
>
> With this change, we keep the custom-timeout, but we renew it, prior to
> each MMIO/PIO/DMA operation. Thus, we time-out only when a particaly

TYPO: s/particaly/specific/ or s/particaly/particular/ ?

> operation takes a long time.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Otherwise,

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

> ---
>  tests/qtest/fuzz/generic_fuzz.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c
> index cea7d4058e..71d36e8f6f 100644
> --- a/tests/qtest/fuzz/generic_fuzz.c
> +++ b/tests/qtest/fuzz/generic_fuzz.c
> @@ -661,15 +661,16 @@ static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>      uint8_t op;
>  
>      if (fork() == 0) {
> +        struct sigaction sact;
> +        struct itimerval timer;
>          /*
>           * Sometimes the fuzzer will find inputs that take quite a long time to
>           * process. Often times, these inputs do not result in new coverage.
>           * Even if these inputs might be interesting, they can slow down the
> -         * fuzzer, overall. Set a timeout to avoid hurting performance, too much
> +         * fuzzer, overall. Set a timeout for each command to avoid hurting
> +         * performance, too much
>           */
>          if (timeout) {
> -            struct sigaction sact;
> -            struct itimerval timer;
>  
>              sigemptyset(&sact.sa_mask);
>              sact.sa_flags   = SA_NODEFER;
> @@ -679,13 +680,17 @@ static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size)
>              memset(&timer, 0, sizeof(timer));
>              timer.it_value.tv_sec = timeout / USEC_IN_SEC;
>              timer.it_value.tv_usec = timeout % USEC_IN_SEC;
> -            setitimer(ITIMER_VIRTUAL, &timer, NULL);
>          }
>  
>          op_clear_dma_patterns(s, NULL, 0);
>          pci_disabled = false;
>  
>          while (cmd && Size) {
> +            /* Reset the timeout, each time we run a new command */
> +            if (timeout) {
> +                setitimer(ITIMER_VIRTUAL, &timer, NULL);
> +            }
> +
>              /* Get the length until the next command or end of input */
>              nextcmd = memmem(cmd, Size, SEPARATOR, strlen(SEPARATOR));
>              cmd_len = nextcmd ? nextcmd - cmd : Size;
> -- 
> 2.28.0

[PATCH v3 2/4] fuzz: add an instrumentation filter

[Alexander Bulekov]

By default, -fsanitize=fuzzer instruments all code with coverage
information. However, this means that libfuzzer will track coverage over
hundreds of source files that are unrelated to virtual-devices. This
means that libfuzzer will optimize inputs for coverage observed in timer
code, memory APIs etc. This slows down the fuzzer and stores many inputs
that are not relevant to the actual virtual-devices.

With this change, clang versions that support the
"-fsanitize-coverage-allowlist" will only instrument a subset of the
compiled code, that is directly related to virtual-devices.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 configure                               | 11 +++++++++++
 scripts/oss-fuzz/instrumentation-filter | 14 ++++++++++++++
 2 files changed, 25 insertions(+)
 create mode 100644 scripts/oss-fuzz/instrumentation-filter

diff --git a/configure b/configure
index debd50c085..db8bee939a 100755
--- a/configure
+++ b/configure
@@ -5176,6 +5176,11 @@ if test "$fuzzing" = "yes" && test -z "${LIB_FUZZING_ENGINE+xxx}"; then
     error_exit "Your compiler doesn't support -fsanitize=fuzzer"
     exit 1
   fi
+  have_clang_coverage_filter=no
+  echo > $TMPTXT
+  if compile_prog "$CPU_CFLAGS -Werror -fsanitize=fuzzer -fsanitize-coverage-allowlist=$TMPTXT" ""; then
+      have_clang_coverage_filter=yes
+  fi
 fi
 
 # Thread sanitizer is, for now, much noisier than the other sanitizers;
@@ -6101,6 +6106,12 @@ if test "$fuzzing" = "yes" ; then
     # rule for the fuzzer adds these to the link_args. They need to be
     # configurable, to support OSS-Fuzz
     FUZZ_EXE_LDFLAGS="-fsanitize=fuzzer"
+
+    # Specify a filter to only instrument code that is directly related to
+    # virtual-devices.
+    if test "$have_clang_coverage_filter" = "yes" ; then
+        QEMU_CFLAGS="$QEMU_CFLAGS -fsanitize-coverage-allowlist=$source_path/scripts/oss-fuzz/instrumentation-filter"
+    fi
   else
     FUZZ_EXE_LDFLAGS="$LIB_FUZZING_ENGINE"
   fi
diff --git a/scripts/oss-fuzz/instrumentation-filter b/scripts/oss-fuzz/instrumentation-filter
new file mode 100644
index 0000000000..44e853159c
--- /dev/null
+++ b/scripts/oss-fuzz/instrumentation-filter
@@ -0,0 +1,14 @@
+# Code that we actually want the fuzzer to target
+# See: https://clang.llvm.org/docs/SanitizerCoverage.html#disabling-instrumentation-without-source-modification
+#
+src:*/hw/*
+src:*/include/hw/*
+src:*/slirp/*
+
+# We don't care about coverage over fuzzer-specific code, however we should
+# instrument the fuzzer entry-point so libFuzzer always sees at least some
+# coverage - otherwise it will exit after the first input
+src:*/tests/qtest/fuzz/fuzz.c
+
+# Enable instrumentation for all functions in those files
+fun:*
-- 
2.28.0

Re: [PATCH v3 2/4] fuzz: add an instrumentation filter

[Philippe Mathieu-Daudé]

On 6/24/21 5:45 AM, Alexander Bulekov wrote:
> By default, -fsanitize=fuzzer instruments all code with coverage
> information. However, this means that libfuzzer will track coverage over
> hundreds of source files that are unrelated to virtual-devices. This
> means that libfuzzer will optimize inputs for coverage observed in timer
> code, memory APIs etc. This slows down the fuzzer and stores many inputs
> that are not relevant to the actual virtual-devices.
> 
> With this change, clang versions that support the
> "-fsanitize-coverage-allowlist" will only instrument a subset of the
> compiled code, that is directly related to virtual-devices.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  configure                               | 11 +++++++++++
>  scripts/oss-fuzz/instrumentation-filter | 14 ++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100644 scripts/oss-fuzz/instrumentation-filter

>  # Thread sanitizer is, for now, much noisier than the other sanitizers;
> @@ -6101,6 +6106,12 @@ if test "$fuzzing" = "yes" ; then
>      # rule for the fuzzer adds these to the link_args. They need to be
>      # configurable, to support OSS-Fuzz
>      FUZZ_EXE_LDFLAGS="-fsanitize=fuzzer"
> +
> +    # Specify a filter to only instrument code that is directly related to
> +    # virtual-devices.
> +    if test "$have_clang_coverage_filter" = "yes" ; then
> +        QEMU_CFLAGS="$QEMU_CFLAGS -fsanitize-coverage-allowlist=$source_path/scripts/oss-fuzz/instrumentation-filter"

Wouldn't it be more useful if we copy the instrumentation-filter
template to the build directory and use it from there? So we could
easily adapt individual fuzzing sessions while using the same
branch (not modifying the source). At least that would be my use
case :)

If so, then better rename as instrumentation-filter-template.txt, and
copy it as [fuzzer-]instrumentation-filter.txt.

Regards,

Phil.

[PATCH v3 3/4] fuzz: fix the AC97 generic-fuzzer config.

[Alexander Bulekov]

TYPE_AC97 is "AC97", capitalized. Fix the config to account for that.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/generic_fuzz_configs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
index 004c701915..049697b974 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -218,7 +218,7 @@ const generic_fuzz_config predefined_configs[] = {
         .name = "ac97",
         .args = "-machine q35 -nodefaults "
         "-device ac97,audiodev=snd0 -audiodev none,id=snd0 -nodefaults",
-        .objects = "ac97*",
+        .objects = "ac97* AC97",
     },{
         .name = "cs4231a",
         .args = "-machine q35 -nodefaults "
-- 
2.28.0

Re: [PATCH v3 3/4] fuzz: fix the AC97 generic-fuzzer config.

[Darren Kenny]

Hi Alex,

Given that 2 of the patches here are simply adding a capitalized version
of the string, I wonder if this is something that should be case
insensitive, and thus the code should change instead?

Hypothetically, how likely is it that there are unrelated objects with
the same name but different case in the name?

Isn't it more likely to be that any object with the same name, despite
the case, is the same object?

Thanks,

Darren.

On Wednesday, 2021-06-23 at 23:45:02 -04, Alexander Bulekov wrote:
> TYPE_AC97 is "AC97", capitalized. Fix the config to account for that.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  tests/qtest/fuzz/generic_fuzz_configs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
> index 004c701915..049697b974 100644
> --- a/tests/qtest/fuzz/generic_fuzz_configs.h
> +++ b/tests/qtest/fuzz/generic_fuzz_configs.h
> @@ -218,7 +218,7 @@ const generic_fuzz_config predefined_configs[] = {
>          .name = "ac97",
>          .args = "-machine q35 -nodefaults "
>          "-device ac97,audiodev=snd0 -audiodev none,id=snd0 -nodefaults",
> -        .objects = "ac97*",
> +        .objects = "ac97* AC97",
>      },{
>          .name = "cs4231a",
>          .args = "-machine q35 -nodefaults "
> -- 
> 2.28.0

[PATCH v3 4/4] fuzz: fix the ES1370 generic-fuzzer config.

[Alexander Bulekov]

TYPE_ES1370 is "ES1370", capitalized. Fix the config to account for
that.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/fuzz/generic_fuzz_configs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/fuzz/generic_fuzz_configs.h b/tests/qtest/fuzz/generic_fuzz_configs.h
index 049697b974..5070bc175a 100644
--- a/tests/qtest/fuzz/generic_fuzz_configs.h
+++ b/tests/qtest/fuzz/generic_fuzz_configs.h
@@ -228,7 +228,7 @@ const generic_fuzz_config predefined_configs[] = {
         .name = "es1370",
         .args = "-machine q35 -nodefaults "
         "-device es1370,audiodev=snd0 -audiodev none,id=snd0 -nodefaults",
-        .objects = "es1370*",
+        .objects = "es1370* ES1370",
     },{
         .name = "sb16",
         .args = "-machine q35 -nodefaults "
-- 
2.28.0