From fd250172842b3bbd4213242eb83bd5fa989f7381 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Mon, 2 Nov 2020 11:33:36 -0500 Subject: qtest: add a reproducer for LP#1878642 https://bugs.launchpad.net/qemu/+bug/1878642 Suggested-by: Paolo Bonzini Signed-off-by: Alexander Bulekov Message-Id: <20201102163336.115444-1-alxndr@bu.edu> Signed-off-by: Paolo Bonzini --- tests/qtest/fuzz-test.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'tests') diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c index 2f38bb1..9cb4c42 100644 --- a/tests/qtest/fuzz-test.c +++ b/tests/qtest/fuzz-test.c @@ -34,6 +34,19 @@ static void test_lp1878263_megasas_zero_iov_cnt(void) qtest_quit(s); } +static void test_lp1878642_pci_bus_get_irq_level_assert(void) +{ + QTestState *s; + + s = qtest_init("-M pc-q35-5.0 " + "-nographic -monitor none -serial none " + "-d guest_errors -trace pci*"); + + qtest_outl(s, 0xcf8, 0x8400f841); + qtest_outl(s, 0xcfc, 0xebed205d); + qtest_outl(s, 0x5d02, 0xebed205d); +} + int main(int argc, char **argv) { const char *arch = qtest_get_arch(); @@ -43,6 +56,8 @@ int main(int argc, char **argv) if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { qtest_add_func("fuzz/test_lp1878263_megasas_zero_iov_cnt", test_lp1878263_megasas_zero_iov_cnt); + qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert", + test_lp1878642_pci_bus_get_irq_level_assert); } return g_test_run(); -- cgit v1.1 From 1d72d9c4874f61c38df9a473e2fd4de869ba0b11 Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 3 Nov 2020 11:51:12 +0000 Subject: tests/qtest/libqtest.c: Check for setsockopt() failure In socket_accept() we use setsockopt() to set SO_RCVTIMEO, but we don't check the return value for failure. Do so. Fixes: Coverity CID 1432321 Signed-off-by: Peter Maydell Message-Id: <20201103115112.19211-1-peter.maydell@linaro.org> Signed-off-by: Paolo Bonzini --- tests/qtest/libqtest.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 99deff4..be0fb43 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -110,8 +110,13 @@ static int socket_accept(int sock) struct timeval timeout = { .tv_sec = SOCKET_TIMEOUT, .tv_usec = 0 }; - setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, (void *)&timeout, - sizeof(timeout)); + if (qemu_setsockopt(sock, SOL_SOCKET, SO_RCVTIMEO, + (void *)&timeout, sizeof(timeout))) { + fprintf(stderr, "%s failed to set SO_RCVTIMEO: %s\n", + __func__, strerror(errno)); + close(sock); + return -1; + } do { addrlen = sizeof(addr); -- cgit v1.1 From 0250edf1eb4ff0b164c0cdabcbf4313507f3082e Mon Sep 17 00:00:00 2001 From: Peter Maydell Date: Tue, 3 Nov 2020 11:52:57 +0000 Subject: tests/qtest/libqos/ahci.c: Avoid NULL dereference in ahci_exec() In ahci_exec() we attempt to permit the caller to pass a NULL pointer for opts_in (in which case we use a default set of options). However although we check for NULL when setting up the opts variable at the top of the function, we unconditionally dereference opts_in at the end of the function as part of freeing the opts->buffer. Switch to checking whether the final buffer is the same as the buffer we started with, instead of assuming the value we started with is always opts_in->buffer. At the moment all the callers pass a non-NULL opts argument, so we never saw any crashes in practice. Fixes: Coverity CID 1432302 Signed-off-by: Peter Maydell Message-Id: <20201103115257.23623-1-peter.maydell@linaro.org> Signed-off-by: Paolo Bonzini --- tests/qtest/libqos/ahci.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/qtest/libqos/ahci.c b/tests/qtest/libqos/ahci.c index 2946abc..fba3e7a 100644 --- a/tests/qtest/libqos/ahci.c +++ b/tests/qtest/libqos/ahci.c @@ -637,10 +637,13 @@ void ahci_exec(AHCIQState *ahci, uint8_t port, AHCICommand *cmd; int rc; AHCIOpts *opts; + uint64_t buffer_in; opts = g_memdup((opts_in == NULL ? &default_opts : opts_in), sizeof(AHCIOpts)); + buffer_in = opts->buffer; + /* No guest buffer provided, create one. */ if (opts->size && !opts->buffer) { opts->buffer = ahci_alloc(ahci, opts->size); @@ -686,7 +689,7 @@ void ahci_exec(AHCIQState *ahci, uint8_t port, g_assert_cmpint(rc, ==, 0); } ahci_command_free(cmd); - if (opts->buffer != opts_in->buffer) { + if (opts->buffer != buffer_in) { ahci_free(ahci, opts->buffer); } g_free(opts); -- cgit v1.1 From c59c582d56ee3bbde15e6788c0d28329792b2573 Mon Sep 17 00:00:00 2001 From: AlexChen Date: Tue, 3 Nov 2020 22:53:09 +0800 Subject: tests/qtest: Fix potential NULL pointer dereference in qos_build_main_args() In qos_build_main_args(), the pointer 'path' is dereferenced before checking it is valid, which may lead to NULL pointer dereference. So move the assignment to 'cmd_line' after checking 'path' is valid. Reported-by: Euler Robot Signed-off-by: Alex Chen Message-Id: <5FA16ED5.4000203@huawei.com> Signed-off-by: Paolo Bonzini --- tests/qtest/fuzz/qos_fuzz.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/qtest/fuzz/qos_fuzz.c b/tests/qtest/fuzz/qos_fuzz.c index b943577..cee1a2a 100644 --- a/tests/qtest/fuzz/qos_fuzz.c +++ b/tests/qtest/fuzz/qos_fuzz.c @@ -70,7 +70,7 @@ static GString *qos_build_main_args(void) { char **path = fuzz_path_vec; QOSGraphNode *test_node; - GString *cmd_line = g_string_new(path[0]); + GString *cmd_line; void *test_arg; if (!path) { @@ -79,6 +79,7 @@ static GString *qos_build_main_args(void) } /* Before test */ + cmd_line = g_string_new(path[0]); current_path = path; test_node = qos_graph_get_node(path[(g_strv_length(path) - 1)]); test_arg = test_node->u.test.arg; -- cgit v1.1 From a9f67c1d51dda405bc6a406d13c8802b98df904e Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Thu, 29 Oct 2020 13:28:58 -0400 Subject: fuzz: fix writing DMA patterns This code had all sorts of issues. We used a loop similar to address_space_write_rom, but I did not remove a "break" that only made sense in the context of the switch statement in the original code. Then, after the loop, we did a separate qtest_memwrite over the entire DMA access range, defeating the purpose of the loop. Additionally, we increment the buf pointer, and then try to g_free() it. Fix these problems. Reported-by: OSS-Fuzz (Issue 26725) Signed-off-by: Alexander Bulekov Reported-by: OSS-Fuzz (Issue 26691) Reviewed-by: Darren Kenny Message-Id: <20201029172901.534442-2-alxndr@bu.edu> Signed-off-by: Paolo Bonzini --- tests/qtest/fuzz/generic_fuzz.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) (limited to 'tests') diff --git a/tests/qtest/fuzz/generic_fuzz.c b/tests/qtest/fuzz/generic_fuzz.c index a8f5864..3e2d50f 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -229,10 +229,10 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write) address_range ar = {addr, len}; g_array_append_val(dma_regions, ar); pattern p = g_array_index(dma_patterns, pattern, dma_pattern_index); - void *buf = pattern_alloc(p, ar.size); + void *buf_base = pattern_alloc(p, ar.size); + void *buf = buf_base; hwaddr l, addr1; MemoryRegion *mr1; - uint8_t *ram_ptr; while (len > 0) { l = len; mr1 = address_space_translate(first_cpu->as, @@ -244,30 +244,27 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write) l = memory_access_size(mr1, l, addr1); } else { /* ROM/RAM case */ - ram_ptr = qemu_map_ram_ptr(mr1->ram_block, addr1); - memcpy(ram_ptr, buf, l); - break; + if (qtest_log_enabled) { + /* + * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log + * that will be written by qtest.c with a DMA tag, so we can reorder + * the resulting QTest trace so the DMA fills precede the last PIO/MMIO + * command. + */ + fprintf(stderr, "[DMA] "); + if (double_fetch) { + fprintf(stderr, "[DOUBLE-FETCH] "); + } + fflush(stderr); + } + qtest_memwrite(qts_global, addr, buf, l); } len -= l; buf += l; addr += l; } - if (qtest_log_enabled) { - /* - * With QTEST_LOG, use a normal, slow QTest memwrite. Prefix the log - * that will be written by qtest.c with a DMA tag, so we can reorder - * the resulting QTest trace so the DMA fills precede the last PIO/MMIO - * command. - */ - fprintf(stderr, "[DMA] "); - if (double_fetch) { - fprintf(stderr, "[DOUBLE-FETCH] "); - } - fflush(stderr); - } - qtest_memwrite(qts_global, ar.addr, buf, ar.size); - g_free(buf); + g_free(buf_base); /* Increment the index of the pattern for the next DMA access */ dma_pattern_index = (dma_pattern_index + 1) % dma_patterns->len; -- cgit v1.1 From cc3d99c7418925b9f252482d67055e7c3f2c4814 Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Thu, 29 Oct 2020 13:28:59 -0400 Subject: fuzz: check the MR in the DMA callback We should be checking that the device is trying to read from RAM, before filling the region with data. Otherwise, we will try to populate nonsensical addresses in RAM for callbacks on PIO/MMIO reads. We did this originally, however the final version I sent had the line commented out.. Signed-off-by: Alexander Bulekov Reviewed-by: Darren Kenny Message-Id: <20201029172901.534442-3-alxndr@bu.edu> Signed-off-by: Paolo Bonzini --- 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 3e2d50f..3a5dbc3 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -192,7 +192,7 @@ void fuzz_dma_read_cb(size_t addr, size_t len, MemoryRegion *mr, bool is_write) */ if (dma_patterns->len == 0 || len == 0 - /* || mr != MACHINE(qdev_get_machine())->ram */ + || mr != current_machine->ram || is_write || addr > current_machine->ram_size) { return; -- cgit v1.1 From 953e6d7c0e94126dbfdb63ba1546e6b74ed9ccee Mon Sep 17 00:00:00 2001 From: Alexander Bulekov Date: Thu, 29 Oct 2020 13:29:00 -0400 Subject: fuzz: fuzz offsets within pio/mmio regions The code did not add offsets to FlatRange bases, so we did not fuzz offsets within device MemoryRegions. Signed-off-by: Alexander Bulekov Reviewed-by: Darren Kenny Message-Id: <20201029172901.534442-4-alxndr@bu.edu> Signed-off-by: Paolo Bonzini --- 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 3a5dbc3..262a963 100644 --- a/tests/qtest/fuzz/generic_fuzz.c +++ b/tests/qtest/fuzz/generic_fuzz.c @@ -298,6 +298,11 @@ static bool get_io_address(address_range *result, AddressSpace *as, } while (cb_info.index != index && !cb_info.found); *result = cb_info.result; + if (result->size) { + offset = offset % result->size; + result->addr += offset; + result->size -= offset; + } return cb_info.found; } -- cgit v1.1 From 794b95608f8d92189baba697a7d6072e94ed2e0e Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 3 Nov 2020 07:37:46 -0500 Subject: ivshmem-test: do not use short-form boolean option This QemuOpts idiom will be deprecated, so get rid of it in the tests. Signed-off-by: Paolo Bonzini --- tests/qtest/ivshmem-test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/qtest/ivshmem-test.c b/tests/qtest/ivshmem-test.c index d5c8b9f..dfa6942 100644 --- a/tests/qtest/ivshmem-test.c +++ b/tests/qtest/ivshmem-test.c @@ -135,7 +135,7 @@ static void setup_vm_cmd(IVState *s, const char *cmd, bool msix) static void setup_vm(IVState *s) { char *cmd = g_strdup_printf("-object memory-backend-file" - ",id=mb1,size=1M,share,mem-path=/dev/shm%s" + ",id=mb1,size=1M,share=on,mem-path=/dev/shm%s" " -device ivshmem-plain,memdev=mb1", tmpshm); setup_vm_cmd(s, cmd, false); -- cgit v1.1 From e27bd4987699df5f49a03e93cf57941abeb82938 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Tue, 3 Nov 2020 06:57:46 -0500 Subject: qtest: escape device name in device-introspect-test device-introspect-test uses HMP, so it should escape the device name properly. Because of this, a few devices that had commas in their names were escaping testing. Signed-off-by: Paolo Bonzini --- tests/qtest/device-introspect-test.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/qtest/device-introspect-test.c b/tests/qtest/device-introspect-test.c index 9f22340..bbec166 100644 --- a/tests/qtest/device-introspect-test.c +++ b/tests/qtest/device-introspect-test.c @@ -104,7 +104,8 @@ static QList *device_type_list(QTestState *qts, bool abstract) static void test_one_device(QTestState *qts, const char *type) { QDict *resp; - char *help; + char *help, *escaped; + GRegex *comma; g_test_message("Testing device '%s'", type); @@ -113,8 +114,13 @@ static void test_one_device(QTestState *qts, const char *type) type); qobject_unref(resp); - help = qtest_hmp(qts, "device_add \"%s,help\"", type); + comma = g_regex_new(",", 0, 0, NULL); + escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL); + g_regex_unref(comma); + + help = qtest_hmp(qts, "device_add \"%s,help\"", escaped); g_free(help); + g_free(escaped); } static void test_device_intro_list(void) -- cgit v1.1