Hello Peter, The following changes since commit ec397e90d21269037280633b6058d1f280e27667: Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20210901-2' into staging (2021-09-01 08:33:02 +0100) are available in the Git repository at: https://gitlab.com/a1xndr/qemu/ tags/fuzz-pull-2021-09-01 for you to fetch changes up to 5d32fc3b60ffad175cab27dfaf07e0929a4f5755: MAINTAINERS: add fuzzing reviewer (2021-09-01 07:33:13 -0400) Thank you ---------------------------------------------------------------- Fuzzing Patches for 2021-09-01 * Add Darren and Qiuhao as reviewers * Make coverage-instrumentation and timeouts more granular * Fix a problem with impossible writes in the DMA callback, leading to false-positives * Attempt to fix oss-fuzz coverage builds, by dealing with a signal-related problem ---------------------------------------------------------------- Alexander Bulekov (6): fuzz: fix sparse memory access in the DMA callback fuzz: adjust timeout to allow for longer inputs fuzz: make object-name matching case-insensitive fuzz: add an instrumentation filter fuzz: use ITIMER_REAL for timeouts fuzz: unblock SIGALRM so the timeout works Darren Kenny (1): MAINTAINERS: Add myself as a reviewer for Device Fuzzing Qiuhao Li (1): MAINTAINERS: add fuzzing reviewer MAINTAINERS | 2 ++ configure | 28 ++++++++++++++++++++++------ scripts/oss-fuzz/instrumentation-filter-template | 15 +++++++++++++++ tests/qtest/fuzz/generic_fuzz.c | 55 ++++++++++++++++++++++++++++++++++++++++++++----------- 4 files changed, 83 insertions(+), 17 deletions(-) create mode 100644 scripts/oss-fuzz/instrumentation-filter-template -- 2.30.2
The code mistakenly relied on address_space_translate to store the length remaining until the next memory-region. We care about this because when there is RAM or sparse-memory neighboring on an MMIO region, we should only write up to the border, to prevent inadvertently invoking MMIO handlers within the DMA callback. However address_space_translate_internal only stores the length until the end of the MemoryRegion if memory_region_is_ram(mr). Otherwise the *len is left unmodified. This caused some false-positive issues, where the fuzzer found a way to perform a nested MMIO write through a DMA callback on an [address, length] that started within sparse memory and spanned some device MMIO regions. To fix this, write to sparse memory in small chunks of memory_access_size (similar to the underlying address_space_write code), which will prevent accidentally hitting MMIO handlers through large writes. Signed-off-by: Alexander Bulekov <alxndr@bu.edu> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- tests/qtest/fuzz/generic_fuzz.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index 6c67522717..0ea47298b7 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -240,10 +240,17 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr) addr, &addr1, &l, true, MEMTXATTRS_UNSPECIFIED); - if (!(memory_region_is_ram(mr1) || - memory_region_is_romd(mr1)) && mr1 != sparse_mem_mr) { + /* + * If mr1 isn't RAM, address_space_translate doesn't update l. Use + * memory_access_size to identify the number of bytes that it is safe + * to write without accidentally writing to another MemoryRegion. + */ + if (!memory_region_is_ram(mr1)) { l = memory_access_size(mr1, l, addr1); - } else { + } + if (memory_region_is_ram(mr1) || + memory_region_is_romd(mr1) || + mr1 == sparse_mem_mr) { /* ROM/RAM case */ if (qtest_log_enabled) { /* -- 2.30.2
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 specific operation takes a long time. Reviewed-by: Darren Kenny <darren.kenny@oracle.com> 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 0ea47298b7..80eb29bd2d 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -668,15 +668,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; @@ -686,13 +687,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.30.2
We have some configs for devices such as the AC97 and ES1370 that were not matching memory-regions correctly, because the configs provided lowercase names. To resolve these problems and prevent them from occurring again in the future, convert both the pattern and names to lower-case, prior to checking for a match. Suggested-by: Darren Kenny <darren.kenny@oracle.com> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> --- tests/qtest/fuzz/generic_fuzz.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index 80eb29bd2d..3e8ce29227 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -758,8 +758,13 @@ static int locate_fuzz_memory_regions(Object *child, void *opaque) static int locate_fuzz_objects(Object *child, void *opaque) { + GString *type_name; + GString *path_name; char *pattern = opaque; - if (g_pattern_match_simple(pattern, object_get_typename(child))) { + + type_name = g_string_new(object_get_typename(child)); + g_string_ascii_down(type_name); + if (g_pattern_match_simple(pattern, type_name->str)) { /* Find and save ptrs to any child MemoryRegions */ object_child_foreach_recursive(child, locate_fuzz_memory_regions, NULL); @@ -776,8 +781,9 @@ static int locate_fuzz_objects(Object *child, void *opaque) g_ptr_array_add(fuzzable_pci_devices, PCI_DEVICE(child)); } } else if (object_dynamic_cast(OBJECT(child), TYPE_MEMORY_REGION)) { - if (g_pattern_match_simple(pattern, - object_get_canonical_path_component(child))) { + path_name = g_string_new(object_get_canonical_path_component(child)); + g_string_ascii_down(path_name); + if (g_pattern_match_simple(pattern, path_name->str)) { MemoryRegion *mr; mr = MEMORY_REGION(child); if ((memory_region_is_ram(mr) || @@ -786,7 +792,9 @@ static int locate_fuzz_objects(Object *child, void *opaque) g_hash_table_insert(fuzzable_memoryregions, mr, (gpointer)true); } } + g_string_free(path_name, true); } + g_string_free(type_name, true); return 0; } @@ -814,6 +822,7 @@ static void generic_pre_fuzz(QTestState *s) MemoryRegion *mr; QPCIBus *pcibus; char **result; + GString *name_pattern; if (!getenv("QEMU_FUZZ_OBJECTS")) { usage(); @@ -843,10 +852,17 @@ static void generic_pre_fuzz(QTestState *s) result = g_strsplit(getenv("QEMU_FUZZ_OBJECTS"), " ", -1); for (int i = 0; result[i] != NULL; i++) { + name_pattern = g_string_new(result[i]); + /* + * Make the pattern lowercase. We do the same for all the MemoryRegion + * and Type names so the configs are case-insensitive. + */ + g_string_ascii_down(name_pattern); printf("Matching objects by name %s\n", result[i]); object_child_foreach_recursive(qdev_get_machine(), locate_fuzz_objects, - result[i]); + name_pattern->str); + g_string_free(name_pattern, true); } g_strfreev(result); printf("This process will try to fuzz the following MemoryRegions:\n"); -- 2.30.2
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> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> --- configure | 28 +++++++++++++++---- .../oss-fuzz/instrumentation-filter-template | 15 ++++++++++ 2 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 scripts/oss-fuzz/instrumentation-filter-template diff --git a/configure b/configure index 9a79a004d7..dcdbe3f068 100755 --- a/configure +++ b/configure @@ -4198,13 +4198,21 @@ fi ########################################## # checks for fuzzer -if test "$fuzzing" = "yes" && test -z "${LIB_FUZZING_ENGINE+xxx}"; then +if test "$fuzzing" = "yes" ; then write_c_fuzzer_skeleton - if compile_prog "$CPU_CFLAGS -Werror -fsanitize=fuzzer" ""; then - have_fuzzer=yes - else - error_exit "Your compiler doesn't support -fsanitize=fuzzer" - exit 1 + if test -z "${LIB_FUZZING_ENGINE+xxx}"; then + if compile_prog "$CPU_CFLAGS -Werror -fsanitize=fuzzer" ""; then + have_fuzzer=yes + else + error_exit "Your compiler doesn't support -fsanitize=fuzzer" + exit 1 + fi + 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 @@ -4884,6 +4892,14 @@ if test "$fuzzing" = "yes" ; then else FUZZ_EXE_LDFLAGS="$LIB_FUZZING_ENGINE" fi + + # Specify a filter to only instrument code that is directly related to + # virtual-devices. + if test "$have_clang_coverage_filter" = "yes" ; then + cp "$source_path/scripts/oss-fuzz/instrumentation-filter-template" \ + instrumentation-filter + QEMU_CFLAGS="$QEMU_CFLAGS -fsanitize-coverage-allowlist=instrumentation-filter" + fi fi if test "$plugins" = "yes" ; then diff --git a/scripts/oss-fuzz/instrumentation-filter-template b/scripts/oss-fuzz/instrumentation-filter-template new file mode 100644 index 0000000000..76d2b6139a --- /dev/null +++ b/scripts/oss-fuzz/instrumentation-filter-template @@ -0,0 +1,15 @@ +# 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/* +src:*/net/* + +# 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.30.2
Using ITIMER_VIRTUAL is a bad idea, if the fuzzer hits a blocking syscall - e.g. ppoll with a NULL timespec. This causes timeout issues while fuzzing some block-device code. Fix that by using wall-clock time. This might cause inputs to timeout sometimes due to scheduling effects/ambient load, but it is better than bringing the entire fuzzing process to a halt. Based-on: <20210713150037.9297-1-alxndr@bu.edu> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> --- tests/qtest/fuzz/generic_fuzz.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index 3e8ce29227..de427a3727 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -695,7 +695,7 @@ static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size) while (cmd && Size) { /* Reset the timeout, each time we run a new command */ if (timeout) { - setitimer(ITIMER_VIRTUAL, &timer, NULL); + setitimer(ITIMER_REAL, &timer, NULL); } /* Get the length until the next command or end of input */ -- 2.30.2
The timeout mechanism won't work if SIGALRM is blocked. This changes unmasks SIGALRM when the timer is installed. This doesn't completely solve the problem, as the fuzzer could trigger some device activity that re-masks SIGALRM. However, there are currently no inputs on OSS-Fuzz that re-mask SIGALRM and timeout. If that turns out to be a real issue, we could try to hook sigmask-type calls, or use a separate timer thread. Based-on: <20210713150037.9297-1-alxndr@bu.edu> Signed-off-by: Alexander Bulekov <alxndr@bu.edu> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> --- tests/qtest/fuzz/generic_fuzz.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index de427a3727..dd7e25851c 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -670,6 +670,7 @@ static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size) if (fork() == 0) { struct sigaction sact; struct itimerval timer; + sigset_t set; /* * Sometimes the fuzzer will find inputs that take quite a long time to * process. Often times, these inputs do not result in new coverage. @@ -684,6 +685,10 @@ static void generic_fuzz(QTestState *s, const unsigned char *Data, size_t Size) sact.sa_handler = handle_timeout; sigaction(SIGALRM, &sact, NULL); + sigemptyset(&set); + sigaddset(&set, SIGALRM); + pthread_sigmask(SIG_UNBLOCK, &set, NULL); + memset(&timer, 0, sizeof(timer)); timer.it_value.tv_sec = timeout / USEC_IN_SEC; timer.it_value.tv_usec = timeout % USEC_IN_SEC; -- 2.30.2
From: Darren Kenny <darren.kenny@oracle.com> Should have done this much sooner given the amount of reviewing I'm already doing in this area. Signed-off-by: Darren Kenny <darren.kenny@oracle.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alexander Bulekov <alxndr@bu.edu> --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index dffcb651f4..8b725091d4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2711,6 +2711,7 @@ R: Paolo Bonzini <pbonzini@redhat.com> R: Bandan Das <bsd@redhat.com> R: Stefan Hajnoczi <stefanha@redhat.com> R: Thomas Huth <thuth@redhat.com> +R: Darren Kenny <darren.kenny@oracle.com> S: Maintained F: tests/qtest/fuzz/ F: tests/qtest/fuzz-*test.c -- 2.30.2
From: Qiuhao Li <Qiuhao.Li@outlook.com> To keep me cc-ed when something changes. Suggested by Alexander. https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg03631.html Signed-off-by: Qiuhao Li <Qiuhao.Li@outlook.com> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alexander Bulekov <alxndr@bu.edu> Reviewed-by: Darren Kenny <darren.kenny@oracle.com> --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8b725091d4..590e1bc492 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2712,6 +2712,7 @@ R: Bandan Das <bsd@redhat.com> R: Stefan Hajnoczi <stefanha@redhat.com> R: Thomas Huth <thuth@redhat.com> R: Darren Kenny <darren.kenny@oracle.com> +R: Qiuhao Li <Qiuhao.Li@outlook.com> S: Maintained F: tests/qtest/fuzz/ F: tests/qtest/fuzz-*test.c -- 2.30.2
On Wed, 1 Sept 2021 at 12:50, Alexander Bulekov <alxndr@bu.edu> wrote: > > Hello Peter, > > The following changes since commit ec397e90d21269037280633b6058d1f280e27667: > > Merge remote-tracking branch 'remotes/alistair/tags/pull-riscv-to-apply-20210901-2' into staging (2021-09-01 08:33:02 +0100) > > are available in the Git repository at: > > https://gitlab.com/a1xndr/qemu/ tags/fuzz-pull-2021-09-01 > > for you to fetch changes up to 5d32fc3b60ffad175cab27dfaf07e0929a4f5755: > > MAINTAINERS: add fuzzing reviewer (2021-09-01 07:33:13 -0400) > > Thank you > > ---------------------------------------------------------------- > Fuzzing Patches for 2021-09-01 > * Add Darren and Qiuhao as reviewers > * Make coverage-instrumentation and timeouts more granular > * Fix a problem with impossible writes in the DMA callback, leading to > false-positives > * Attempt to fix oss-fuzz coverage builds, by dealing with a > signal-related problem > Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/6.2 for any user-visible changes. -- PMM