From af16990a1b3aac7a32a58cd4e3509e9e4d44fe69 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 13 Jul 2021 11:00:34 -0400 Subject: 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(-) (limited to 'tests') 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) { /* -- cgit 1.4.1 From 993f52f4d43ddcddcb6f68b79a528599f4f099f9 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 13 Jul 2021 11:00:35 -0400 Subject: 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(-) (limited to 'tests') 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; -- cgit 1.4.1 From f2e8b87a1afeec13157094909bf129c4b64192ba Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Tue, 13 Jul 2021 11:00:36 -0400 Subject: 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(-) (limited to 'tests') 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"); -- cgit 1.4.1 From 40c0d963db2a9d4a49c15554817bbaa11e0bed47 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Wed, 4 Aug 2021 09:56:20 -0400 Subject: 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(-) (limited to 'tests') 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 */ -- cgit 1.4.1 From aaa94a1b3c7bc834c183ddcc8c4199cccebe58ac Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Wed, 4 Aug 2021 09:56:21 -0400 Subject: 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(+) (limited to 'tests') 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; -- cgit 1.4.1