From af16990a1b3aac7a32a58cd4e3509e9e4d44fe69 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 13 Jul 2021 11:00:34 -0400 Subject: [PATCH 1/8] fuzz: fix sparse memory access in the DMA callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Darren Kenny Reviewed-by: Philippe Mathieu-Daudé --- 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) { /* From 993f52f4d43ddcddcb6f68b79a528599f4f099f9 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 13 Jul 2021 11:00:35 -0400 Subject: [PATCH 2/8] fuzz: adjust timeout to allow for longer inputs 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 Signed-off-by: Alexander Bulekov --- 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; From f2e8b87a1afeec13157094909bf129c4b64192ba Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 13 Jul 2021 11:00:36 -0400 Subject: [PATCH 3/8] fuzz: make object-name matching case-insensitive 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 Reviewed-by: Darren Kenny Signed-off-by: Alexander Bulekov --- 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"); From dfc86c0f25126ce3242b317087234c7228418eb2 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 13 Jul 2021 11:00:37 -0400 Subject: [PATCH 4/8] fuzz: add an instrumentation filter 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 Reviewed-by: Darren Kenny --- 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:* From 40c0d963db2a9d4a49c15554817bbaa11e0bed47 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Wed, 4 Aug 2021 09:56:20 -0400 Subject: [PATCH 5/8] fuzz: use ITIMER_REAL for timeouts 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 Reviewed-by: Darren Kenny --- 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 */ From aaa94a1b3c7bc834c183ddcc8c4199cccebe58ac Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Wed, 4 Aug 2021 09:56:21 -0400 Subject: [PATCH 6/8] fuzz: unblock SIGALRM so the timeout works 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 Reviewed-by: Darren Kenny --- 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; From 85221b05f8831d816dd5f8945b4733b092519db1 Mon Sep 17 00:00:00 2001 From: Darren Kenny Date: Tue, 24 Aug 2021 14:04:01 +0000 Subject: [PATCH 7/8] MAINTAINERS: Add myself as a reviewer for Device Fuzzing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should have done this much sooner given the amount of reviewing I'm already doing in this area. Signed-off-by: Darren Kenny Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alexander Bulekov --- 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 R: Bandan Das R: Stefan Hajnoczi R: Thomas Huth +R: Darren Kenny S: Maintained F: tests/qtest/fuzz/ F: tests/qtest/fuzz-*test.c From 5d32fc3b60ffad175cab27dfaf07e0929a4f5755 Mon Sep 17 00:00:00 2001 From: Qiuhao Li Date: Tue, 24 Aug 2021 14:26:39 +0800 Subject: [PATCH 8/8] MAINTAINERS: add fuzzing reviewer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Alexander Bulekov Reviewed-by: Darren Kenny --- 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 R: Stefan Hajnoczi R: Thomas Huth R: Darren Kenny +R: Qiuhao Li S: Maintained F: tests/qtest/fuzz/ F: tests/qtest/fuzz-*test.c