* [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer @ 2021-03-17 23:02 Mark Cave-Ayland 2021-03-17 23:02 ` [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present Mark Cave-Ayland ` (6 more replies) 0 siblings, 7 replies; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-17 23:02 UTC (permalink / raw) To: qemu-devel, alxndr, laurent, pbonzini Recently there have been a number of issues raised on Launchpad as a result of fuzzing the am53c974 (ESP) device. I spent some time over the past couple of days checking to see if anything had improved since my last patchset: from what I can tell the issues are still present, but the cmdfifo related failures now assert rather than corrupting memory. This patchset applied to master passes my local tests using the qtest fuzz test cases added by Alexander for the following Launchpad bugs: https://bugs.launchpad.net/qemu/+bug/1919035 https://bugs.launchpad.net/qemu/+bug/1919036 https://bugs.launchpad.net/qemu/+bug/1910723 https://bugs.launchpad.net/qemu/+bug/1909247 I'm posting this now just before soft freeze since I see that some of the issues have recently been allocated CVEs and so it could be argued that even though they have existed for some time, it is worth fixing them for 6.0. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> v2: - Add Alexander's R-B tag for patch 2 and Phil's R-B for patch 3 - Add patch 4 for additional testcase provided in Alexander's patch 1 comment - Move current_req NULL checks forward in DMA functions (fixes ASAN bug reported at https://bugs.launchpad.net/qemu/+bug/1909247/comments/6) in patch 3 - Add qtest for am53c974 containing a basic set of regression tests using the automatic test cases generated by the fuzzer as requested by Paolo Mark Cave-Ayland (6): esp: don't underflow cmdfifo if no message out/command data is present esp: don't overflow cmdfifo if TC is larger than the cmdfifo size esp: ensure cmdfifo is not empty and current_dev is non-NULL esp: don't underflow fifo when writing to the device esp: always check current_req is not NULL before use in DMA callbacks tests/qtest: add tests for am53c974 device hw/scsi/esp.c | 73 +++++++++++++-------- tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ tests/qtest/meson.build | 1 + 3 files changed, 171 insertions(+), 25 deletions(-) create mode 100644 tests/qtest/am53c974-test.c -- 2.20.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland @ 2021-03-17 23:02 ` Mark Cave-Ayland 2021-03-17 23:58 ` Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 2/6] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size Mark Cave-Ayland ` (5 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-17 23:02 UTC (permalink / raw) To: qemu-devel, alxndr, laurent, pbonzini If a guest sends a TI (Transfer Information) command without previously sending any message out/command phase data then cmdfifo will underflow triggering an assert reading the IDENTIFY byte. Buglink: https://bugs.launchpad.net/qemu/+bug/1919035 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 507ab363bc..5d3f1ccbc8 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -318,18 +318,24 @@ static void do_busid_cmd(ESPState *s, uint8_t busid) static void do_cmd(ESPState *s) { - uint8_t busid = fifo8_pop(&s->cmdfifo); + uint8_t busid; uint32_t n; - s->cmdfifo_cdb_offset--; + if (fifo8_num_used(&s->cmdfifo)) { + busid = fifo8_pop(&s->cmdfifo); - /* Ignore extended messages for now */ - if (s->cmdfifo_cdb_offset) { - fifo8_pop_buf(&s->cmdfifo, s->cmdfifo_cdb_offset, &n); - s->cmdfifo_cdb_offset = 0; - } + if (s->cmdfifo_cdb_offset) { + s->cmdfifo_cdb_offset--; + + /* Ignore extended messages for now */ + if (s->cmdfifo_cdb_offset) { + fifo8_pop_buf(&s->cmdfifo, s->cmdfifo_cdb_offset, &n); + s->cmdfifo_cdb_offset = 0; + } + } - do_busid_cmd(s, busid); + do_busid_cmd(s, busid); + } } static void satn_pdma_cb(ESPState *s) -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present 2021-03-17 23:02 ` [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present Mark Cave-Ayland @ 2021-03-17 23:58 ` Alexander Bulekov 0 siblings, 0 replies; 17+ messages in thread From: Alexander Bulekov @ 2021-03-17 23:58 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: pbonzini, qemu-devel, laurent On 210317 2302, Mark Cave-Ayland wrote: > If a guest sends a TI (Transfer Information) command without previously sending > any message out/command phase data then cmdfifo will underflow triggering an > assert reading the IDENTIFY byte. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1919035 > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) Tested-by: Alexander Bulekov <alxndr@bu.edu> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 2/6] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland 2021-03-17 23:02 ` [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present Mark Cave-Ayland @ 2021-03-17 23:02 ` Mark Cave-Ayland 2021-03-17 23:02 ` [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL Mark Cave-Ayland ` (4 subsequent siblings) 6 siblings, 0 replies; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-17 23:02 UTC (permalink / raw) To: qemu-devel, alxndr, laurent, pbonzini If a guest transfers the message out/command phase data using DMA with a TC that is larger than the cmdfifo size then the cmdfifo overflows triggering an assert. Limit the size of the transfer to the free space available in cmdfifo. Buglink: https://bugs.launchpad.net/qemu/+bug/1919036 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Tested-by: Alexander Bulekov <alxndr@bu.edu> --- hw/scsi/esp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 5d3f1ccbc8..bbcbfa4a91 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -579,6 +579,7 @@ static void esp_do_dma(ESPState *s) cmdlen = fifo8_num_used(&s->cmdfifo); trace_esp_do_dma(cmdlen, len); if (s->dma_memory_read) { + len = MIN(len, fifo8_num_free(&s->cmdfifo)); s->dma_memory_read(s->dma_opaque, buf, len); fifo8_push_all(&s->cmdfifo, buf, len); } else { -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland 2021-03-17 23:02 ` [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present Mark Cave-Ayland 2021-03-17 23:02 ` [PATCH v2 2/6] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size Mark Cave-Ayland @ 2021-03-17 23:02 ` Mark Cave-Ayland 2021-03-18 0:10 ` [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL\ Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 4/6] esp: don't underflow fifo when writing to the device Mark Cave-Ayland ` (3 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-17 23:02 UTC (permalink / raw) To: qemu-devel, alxndr, laurent, pbonzini When about to execute a SCSI command, ensure that cmdfifo is not empty and current_dev is non-NULL. This can happen if the guest tries to execute a TI (Transfer Information) command without issuing one of the select commands first. Buglink: https://bugs.launchpad.net/qemu/+bug/1910723 Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index bbcbfa4a91..ae362c9dfb 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -286,6 +286,9 @@ static void do_busid_cmd(ESPState *s, uint8_t busid) trace_esp_do_busid_cmd(busid); lun = busid & 7; cmdlen = fifo8_num_used(&s->cmdfifo); + if (!cmdlen || !s->current_dev) { + return; + } buf = (uint8_t *)fifo8_pop_buf(&s->cmdfifo, cmdlen, &n); current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun); -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL\ 2021-03-17 23:02 ` [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL Mark Cave-Ayland @ 2021-03-18 0:10 ` Alexander Bulekov 0 siblings, 0 replies; 17+ messages in thread From: Alexander Bulekov @ 2021-03-18 0:10 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: pbonzini, qemu-devel, laurent On 210317 2302, Mark Cave-Ayland wrote: > When about to execute a SCSI command, ensure that cmdfifo is not empty and > current_dev is non-NULL. This can happen if the guest tries to execute a TI > (Transfer Information) command without issuing one of the select commands > first. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1910723 > Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 3 +++ > 1 file changed, 3 insertions(+) > Tested-by: Alexander Bulekov <alxndr@bu.edu> > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index bbcbfa4a91..ae362c9dfb 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -286,6 +286,9 @@ static void do_busid_cmd(ESPState *s, uint8_t busid) > trace_esp_do_busid_cmd(busid); > lun = busid & 7; > cmdlen = fifo8_num_used(&s->cmdfifo); > + if (!cmdlen || !s->current_dev) { > + return; > + } > buf = (uint8_t *)fifo8_pop_buf(&s->cmdfifo, cmdlen, &n); > > current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun); > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 4/6] esp: don't underflow fifo when writing to the device 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland ` (2 preceding siblings ...) 2021-03-17 23:02 ` [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL Mark Cave-Ayland @ 2021-03-17 23:02 ` Mark Cave-Ayland 2021-03-18 0:12 ` Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks Mark Cave-Ayland ` (2 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-17 23:02 UTC (permalink / raw) To: qemu-devel, alxndr, laurent, pbonzini When writing to the device make sure that the fifo is not empty, otherwise the fifo will underflow triggering an assert. Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index ae362c9dfb..bb57125035 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -509,18 +509,20 @@ static void do_dma_pdma_cb(ESPState *s) /* Copy FIFO data to device */ len = MIN(s->async_len, ESP_FIFO_SZ); len = MIN(len, fifo8_num_used(&s->fifo)); - memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); - s->async_buf += n; - s->async_len -= n; - s->ti_size += n; - - if (n < len) { - /* Unaligned accesses can cause FIFO wraparound */ - len = len - n; + if (len) { memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); s->async_buf += n; s->async_len -= n; s->ti_size += n; + + if (n < len) { + /* Unaligned accesses can cause FIFO wraparound */ + len = len - n; + memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); + s->async_buf += n; + s->async_len -= n; + s->ti_size += n; + } } if (s->async_len == 0) { @@ -730,10 +732,12 @@ static void esp_do_nodma(ESPState *s) if (to_device) { len = MIN(fifo8_num_used(&s->fifo), ESP_FIFO_SZ); - memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); - s->async_buf += len; - s->async_len -= len; - s->ti_size += len; + if (len) { + memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); + s->async_buf += len; + s->async_len -= len; + s->ti_size += len; + } } else { len = MIN(s->ti_size, s->async_len); len = MIN(len, fifo8_num_free(&s->fifo)); -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/6] esp: don't underflow fifo when writing to the device 2021-03-17 23:02 ` [PATCH v2 4/6] esp: don't underflow fifo when writing to the device Mark Cave-Ayland @ 2021-03-18 0:12 ` Alexander Bulekov 0 siblings, 0 replies; 17+ messages in thread From: Alexander Bulekov @ 2021-03-18 0:12 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: pbonzini, qemu-devel, laurent On 210317 2302, Mark Cave-Ayland wrote: > When writing to the device make sure that the fifo is not empty, otherwise the > fifo will underflow triggering an assert. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > Tested-by: Alexander Bulekov <alxndr@bu.edu> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland ` (3 preceding siblings ...) 2021-03-17 23:02 ` [PATCH v2 4/6] esp: don't underflow fifo when writing to the device Mark Cave-Ayland @ 2021-03-17 23:02 ` Mark Cave-Ayland 2021-03-18 0:12 ` Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 6/6] tests/qtest: add tests for am53c974 device Mark Cave-Ayland 2021-03-18 18:13 ` [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Paolo Bonzini 6 siblings, 1 reply; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-17 23:02 UTC (permalink / raw) To: qemu-devel, alxndr, laurent, pbonzini After issuing a SCSI command the SCSI layer can call the SCSIBusInfo .cancel callback which resets both current_req and current_dev to NULL. If any data is left in the transfer buffer (async_len != 0) then the next TI (Transfer Information) command will attempt to reference the NULL pointer causing a segfault. Buglink: https://bugs.launchpad.net/qemu/+bug/1910723 Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index bb57125035..d7b81b7608 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -505,6 +505,10 @@ static void do_dma_pdma_cb(ESPState *s) return; } + if (!s->current_req) { + return; + } + if (to_device) { /* Copy FIFO data to device */ len = MIN(s->async_len, ESP_FIFO_SZ); @@ -538,11 +542,9 @@ static void do_dma_pdma_cb(ESPState *s) return; } else { if (s->async_len == 0) { - if (s->current_req) { - /* Defer until the scsi layer has completed */ - scsi_req_continue(s->current_req); - s->data_in_ready = false; - } + /* Defer until the scsi layer has completed */ + scsi_req_continue(s->current_req); + s->data_in_ready = false; return; } @@ -616,6 +618,9 @@ static void esp_do_dma(ESPState *s) } return; } + if (!s->current_req) { + return; + } if (s->async_len == 0) { /* Defer until data is available. */ return; @@ -725,6 +730,10 @@ static void esp_do_nodma(ESPState *s) return; } + if (!s->current_req) { + return; + } + if (s->async_len == 0) { /* Defer until data is available. */ return; -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks 2021-03-17 23:02 ` [PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks Mark Cave-Ayland @ 2021-03-18 0:12 ` Alexander Bulekov 0 siblings, 0 replies; 17+ messages in thread From: Alexander Bulekov @ 2021-03-18 0:12 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: pbonzini, qemu-devel, laurent On 210317 2302, Mark Cave-Ayland wrote: > After issuing a SCSI command the SCSI layer can call the SCSIBusInfo .cancel > callback which resets both current_req and current_dev to NULL. If any data > is left in the transfer buffer (async_len != 0) then the next TI (Transfer > Information) command will attempt to reference the NULL pointer causing a > segfault. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1910723 > Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- Tested-by: Alexander Bulekov <alxndr@bu.edu> ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 6/6] tests/qtest: add tests for am53c974 device 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland ` (4 preceding siblings ...) 2021-03-17 23:02 ` [PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks Mark Cave-Ayland @ 2021-03-17 23:02 ` Mark Cave-Ayland 2021-03-18 0:14 ` Alexander Bulekov 2021-03-18 12:10 ` Paolo Bonzini 2021-03-18 18:13 ` [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Paolo Bonzini 6 siblings, 2 replies; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-17 23:02 UTC (permalink / raw) To: qemu-devel, alxndr, laurent, pbonzini Use the autogenerated fuzzer test cases as the basis for a set of am53c974 regression tests. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ tests/qtest/meson.build | 1 + 2 files changed, 123 insertions(+) create mode 100644 tests/qtest/am53c974-test.c diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c new file mode 100644 index 0000000000..c90bd4c187 --- /dev/null +++ b/tests/qtest/am53c974-test.c @@ -0,0 +1,122 @@ +/* + * QTest testcase for am53c974 + * + * This work is licensed under the terms of the GNU GPL, version 2 or + * later. See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" + +#include "libqos/libqtest.h" + + +static void test_cmdfifo_underflow_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outl(s, 0xcf8, 0x8000100e); + qtest_outl(s, 0xcfc, 0x8a000000); + qtest_outl(s, 0x8a09, 0x42000000); + qtest_outl(s, 0x8a0d, 0x00); + qtest_outl(s, 0x8a0b, 0x1000); + qtest_quit(s); +} + +static void test_cmdfifo_overflow_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outl(s, 0xcf8, 0x8000100e); + qtest_outl(s, 0xcfc, 0x0e000000); + qtest_outl(s, 0xe40, 0x03); + qtest_outl(s, 0xe0b, 0x4100); + qtest_outl(s, 0xe0b, 0x9000); + qtest_quit(s); +} + +static void test_target_selected_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001001); + qtest_outl(s, 0xcfc, 0x01000000); + qtest_outl(s, 0xcf8, 0x8000100e); + qtest_outl(s, 0xcfc, 0xef800000); + qtest_outl(s, 0xef8b, 0x4100); + qtest_outw(s, 0xef80, 0x01); + qtest_outl(s, 0xefc0, 0x03); + qtest_outl(s, 0xef8b, 0xc100); + qtest_outl(s, 0xef8b, 0x9000); + qtest_quit(s); +} + +static void test_fifo_underflow_on_write_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001010); + qtest_outl(s, 0xcfc, 0xc000); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outl(s, 0xc008, 0x0a); + qtest_outl(s, 0xc009, 0x41000000); + qtest_outl(s, 0xc009, 0x41000000); + qtest_outl(s, 0xc00b, 0x1000); + qtest_quit(s); +} + +static void test_cancelled_request_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001010); + qtest_outl(s, 0xcfc, 0xc000); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x05); + qtest_outb(s, 0xc046, 0x02); + qtest_outl(s, 0xc00b, 0xc100); + qtest_outl(s, 0xc040, 0x03); + qtest_outl(s, 0xc040, 0x03); + qtest_bufwrite(s, 0x0, "\x41", 0x1); + qtest_outl(s, 0xc00b, 0xc100); + qtest_outw(s, 0xc040, 0x02); + qtest_outw(s, 0xc040, 0x81); + qtest_outl(s, 0xc00b, 0x9000); + qtest_quit(s); +} + +int main(int argc, char **argv) +{ + const char *arch = qtest_get_arch(); + + g_test_init(&argc, &argv, NULL); + + if (strcmp(arch, "i386") == 0) { + qtest_add_func("am53c974/test_cmdfifo_underflow_ok", + test_cmdfifo_underflow_ok); + qtest_add_func("am53c974/test_cmdfifo_overflow_ok", + test_cmdfifo_overflow_ok); + qtest_add_func("am53c974/test_target_selected_ok", + test_target_selected_ok); + qtest_add_func("am53c974/test_fifo_underflow_on_write_ok", + test_fifo_underflow_on_write_ok); + qtest_add_func("am53c974/test_cancelled_request_ok", + test_cancelled_request_ok); + } + + return g_test_run(); +} diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 66ee9fbf45..a44f612a34 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -63,6 +63,7 @@ qtests_i386 = \ (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) + \ (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) + \ (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + \ + (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) + \ qtests_pci + \ ['fdc-test', 'ide-test', -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/6] tests/qtest: add tests for am53c974 device 2021-03-17 23:02 ` [PATCH v2 6/6] tests/qtest: add tests for am53c974 device Mark Cave-Ayland @ 2021-03-18 0:14 ` Alexander Bulekov 2021-03-18 12:10 ` Paolo Bonzini 1 sibling, 0 replies; 17+ messages in thread From: Alexander Bulekov @ 2021-03-18 0:14 UTC (permalink / raw) To: Mark Cave-Ayland; +Cc: pbonzini, qemu-devel, laurent On 210317 2302, Mark Cave-Ayland wrote: > Use the autogenerated fuzzer test cases as the basis for a set of am53c974 > regression tests. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- Reviewed-by: Alexander Bulekov <alxndr@bu.edu> Thanks ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/6] tests/qtest: add tests for am53c974 device 2021-03-17 23:02 ` [PATCH v2 6/6] tests/qtest: add tests for am53c974 device Mark Cave-Ayland 2021-03-18 0:14 ` Alexander Bulekov @ 2021-03-18 12:10 ` Paolo Bonzini 1 sibling, 0 replies; 17+ messages in thread From: Paolo Bonzini @ 2021-03-18 12:10 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel, alxndr, laurent On 18/03/21 00:02, Mark Cave-Ayland wrote: > Use the autogenerated fuzzer test cases as the basis for a set of am53c974 > regression tests. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Most of these should be easy to change to libqos, but it's good enough to have them like this so that there'll be an example in the future of the conversion (I'll take a look myself). Paolo > --- > tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ > tests/qtest/meson.build | 1 + > 2 files changed, 123 insertions(+) > create mode 100644 tests/qtest/am53c974-test.c > > diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c > new file mode 100644 > index 0000000000..c90bd4c187 > --- /dev/null > +++ b/tests/qtest/am53c974-test.c > @@ -0,0 +1,122 @@ > +/* > + * QTest testcase for am53c974 > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or > + * later. See the COPYING file in the top-level directory. > + */ > + > +#include "qemu/osdep.h" > + > +#include "libqos/libqtest.h" > + > + > +static void test_cmdfifo_underflow_ok(void) > +{ > + QTestState *s = qtest_init( > + "-device am53c974,id=scsi " > + "-device scsi-hd,drive=disk0 -drive " > + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); > + qtest_outl(s, 0xcf8, 0x80001004); > + qtest_outw(s, 0xcfc, 0x01); > + qtest_outl(s, 0xcf8, 0x8000100e); > + qtest_outl(s, 0xcfc, 0x8a000000); > + qtest_outl(s, 0x8a09, 0x42000000); > + qtest_outl(s, 0x8a0d, 0x00); > + qtest_outl(s, 0x8a0b, 0x1000); > + qtest_quit(s); > +} > + > +static void test_cmdfifo_overflow_ok(void) > +{ > + QTestState *s = qtest_init( > + "-device am53c974,id=scsi " > + "-device scsi-hd,drive=disk0 -drive " > + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); > + qtest_outl(s, 0xcf8, 0x80001004); > + qtest_outw(s, 0xcfc, 0x01); > + qtest_outl(s, 0xcf8, 0x8000100e); > + qtest_outl(s, 0xcfc, 0x0e000000); > + qtest_outl(s, 0xe40, 0x03); > + qtest_outl(s, 0xe0b, 0x4100); > + qtest_outl(s, 0xe0b, 0x9000); > + qtest_quit(s); > +} > + > +static void test_target_selected_ok(void) > +{ > + QTestState *s = qtest_init( > + "-device am53c974,id=scsi " > + "-device scsi-hd,drive=disk0 -drive " > + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); > + qtest_outl(s, 0xcf8, 0x80001001); > + qtest_outl(s, 0xcfc, 0x01000000); > + qtest_outl(s, 0xcf8, 0x8000100e); > + qtest_outl(s, 0xcfc, 0xef800000); > + qtest_outl(s, 0xef8b, 0x4100); > + qtest_outw(s, 0xef80, 0x01); > + qtest_outl(s, 0xefc0, 0x03); > + qtest_outl(s, 0xef8b, 0xc100); > + qtest_outl(s, 0xef8b, 0x9000); > + qtest_quit(s); > +} > + > +static void test_fifo_underflow_on_write_ok(void) > +{ > + QTestState *s = qtest_init( > + "-device am53c974,id=scsi " > + "-device scsi-hd,drive=disk0 -drive " > + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); > + qtest_outl(s, 0xcf8, 0x80001010); > + qtest_outl(s, 0xcfc, 0xc000); > + qtest_outl(s, 0xcf8, 0x80001004); > + qtest_outw(s, 0xcfc, 0x01); > + qtest_outl(s, 0xc008, 0x0a); > + qtest_outl(s, 0xc009, 0x41000000); > + qtest_outl(s, 0xc009, 0x41000000); > + qtest_outl(s, 0xc00b, 0x1000); > + qtest_quit(s); > +} > + > +static void test_cancelled_request_ok(void) > +{ > + QTestState *s = qtest_init( > + "-device am53c974,id=scsi " > + "-device scsi-hd,drive=disk0 -drive " > + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); > + qtest_outl(s, 0xcf8, 0x80001010); > + qtest_outl(s, 0xcfc, 0xc000); > + qtest_outl(s, 0xcf8, 0x80001004); > + qtest_outw(s, 0xcfc, 0x05); > + qtest_outb(s, 0xc046, 0x02); > + qtest_outl(s, 0xc00b, 0xc100); > + qtest_outl(s, 0xc040, 0x03); > + qtest_outl(s, 0xc040, 0x03); > + qtest_bufwrite(s, 0x0, "\x41", 0x1); > + qtest_outl(s, 0xc00b, 0xc100); > + qtest_outw(s, 0xc040, 0x02); > + qtest_outw(s, 0xc040, 0x81); > + qtest_outl(s, 0xc00b, 0x9000); > + qtest_quit(s); > +} > + > +int main(int argc, char **argv) > +{ > + const char *arch = qtest_get_arch(); > + > + g_test_init(&argc, &argv, NULL); > + > + if (strcmp(arch, "i386") == 0) { > + qtest_add_func("am53c974/test_cmdfifo_underflow_ok", > + test_cmdfifo_underflow_ok); > + qtest_add_func("am53c974/test_cmdfifo_overflow_ok", > + test_cmdfifo_overflow_ok); > + qtest_add_func("am53c974/test_target_selected_ok", > + test_target_selected_ok); > + qtest_add_func("am53c974/test_fifo_underflow_on_write_ok", > + test_fifo_underflow_on_write_ok); > + qtest_add_func("am53c974/test_cancelled_request_ok", > + test_cancelled_request_ok); > + } > + > + return g_test_run(); > +} > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build > index 66ee9fbf45..a44f612a34 100644 > --- a/tests/qtest/meson.build > +++ b/tests/qtest/meson.build > @@ -63,6 +63,7 @@ qtests_i386 = \ > (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-test'] : []) + \ > (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) + \ > (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + \ > + (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) + \ > qtests_pci + \ > ['fdc-test', > 'ide-test', > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland ` (5 preceding siblings ...) 2021-03-17 23:02 ` [PATCH v2 6/6] tests/qtest: add tests for am53c974 device Mark Cave-Ayland @ 2021-03-18 18:13 ` Paolo Bonzini 2021-03-30 7:34 ` Mark Cave-Ayland 6 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2021-03-18 18:13 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel, alxndr, laurent On 18/03/21 00:02, Mark Cave-Ayland wrote: > Recently there have been a number of issues raised on Launchpad as a result of > fuzzing the am53c974 (ESP) device. I spent some time over the past couple of > days checking to see if anything had improved since my last patchset: from > what I can tell the issues are still present, but the cmdfifo related failures > now assert rather than corrupting memory. > > This patchset applied to master passes my local tests using the qtest fuzz test > cases added by Alexander for the following Launchpad bugs: > > https://bugs.launchpad.net/qemu/+bug/1919035 > https://bugs.launchpad.net/qemu/+bug/1919036 > https://bugs.launchpad.net/qemu/+bug/1910723 > https://bugs.launchpad.net/qemu/+bug/1909247 > > I'm posting this now just before soft freeze since I see that some of the issues > have recently been allocated CVEs and so it could be argued that even though > they have existed for some time, it is worth fixing them for 6.0. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > v2: > - Add Alexander's R-B tag for patch 2 and Phil's R-B for patch 3 > - Add patch 4 for additional testcase provided in Alexander's patch 1 comment > - Move current_req NULL checks forward in DMA functions (fixes ASAN bug reported > at https://bugs.launchpad.net/qemu/+bug/1909247/comments/6) in patch 3 > - Add qtest for am53c974 containing a basic set of regression tests using the > automatic test cases generated by the fuzzer as requested by Paolo > > > Mark Cave-Ayland (6): > esp: don't underflow cmdfifo if no message out/command data is present > esp: don't overflow cmdfifo if TC is larger than the cmdfifo size > esp: ensure cmdfifo is not empty and current_dev is non-NULL > esp: don't underflow fifo when writing to the device > esp: always check current_req is not NULL before use in DMA callbacks > tests/qtest: add tests for am53c974 device > > hw/scsi/esp.c | 73 +++++++++++++-------- > tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ > tests/qtest/meson.build | 1 + > 3 files changed, 171 insertions(+), 25 deletions(-) > create mode 100644 tests/qtest/am53c974-test.c > Queued, thanks. Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer 2021-03-18 18:13 ` [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Paolo Bonzini @ 2021-03-30 7:34 ` Mark Cave-Ayland 2021-03-30 9:59 ` Paolo Bonzini 0 siblings, 1 reply; 17+ messages in thread From: Mark Cave-Ayland @ 2021-03-30 7:34 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, alxndr, laurent On 18/03/2021 18:13, Paolo Bonzini wrote: > On 18/03/21 00:02, Mark Cave-Ayland wrote: >> Recently there have been a number of issues raised on Launchpad as a result of >> fuzzing the am53c974 (ESP) device. I spent some time over the past couple of >> days checking to see if anything had improved since my last patchset: from >> what I can tell the issues are still present, but the cmdfifo related failures >> now assert rather than corrupting memory. >> >> This patchset applied to master passes my local tests using the qtest fuzz test >> cases added by Alexander for the following Launchpad bugs: >> >> https://bugs.launchpad.net/qemu/+bug/1919035 >> https://bugs.launchpad.net/qemu/+bug/1919036 >> https://bugs.launchpad.net/qemu/+bug/1910723 >> https://bugs.launchpad.net/qemu/+bug/1909247 >> I'm posting this now just before soft freeze since I see that some of the issues >> have recently been allocated CVEs and so it could be argued that even though >> they have existed for some time, it is worth fixing them for 6.0. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> v2: >> - Add Alexander's R-B tag for patch 2 and Phil's R-B for patch 3 >> - Add patch 4 for additional testcase provided in Alexander's patch 1 comment >> - Move current_req NULL checks forward in DMA functions (fixes ASAN bug reported >> at https://bugs.launchpad.net/qemu/+bug/1909247/comments/6) in patch 3 >> - Add qtest for am53c974 containing a basic set of regression tests using the >> automatic test cases generated by the fuzzer as requested by Paolo >> >> >> Mark Cave-Ayland (6): >> esp: don't underflow cmdfifo if no message out/command data is present >> esp: don't overflow cmdfifo if TC is larger than the cmdfifo size >> esp: ensure cmdfifo is not empty and current_dev is non-NULL >> esp: don't underflow fifo when writing to the device >> esp: always check current_req is not NULL before use in DMA callbacks >> tests/qtest: add tests for am53c974 device >> >> hw/scsi/esp.c | 73 +++++++++++++-------- >> tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ >> tests/qtest/meson.build | 1 + >> 3 files changed, 171 insertions(+), 25 deletions(-) >> create mode 100644 tests/qtest/am53c974-test.c >> > > Queued, thanks. > > Paolo Hi Paolo, I had a quick look at Alex's updated test cases and most of them are based on an incorrect assumption I made around the behaviour of fifo8_pop_buf(). Can you drop these for now, and I will submit a v3 shortly once I've given it a full run through my test images? ATB, Mark. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer 2021-03-30 7:34 ` Mark Cave-Ayland @ 2021-03-30 9:59 ` Paolo Bonzini 2021-04-01 7:56 ` Mark Cave-Ayland 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2021-03-30 9:59 UTC (permalink / raw) To: Mark Cave-Ayland, qemu-devel, alxndr, laurent On 30/03/21 09:34, Mark Cave-Ayland wrote: > Hi Paolo, > > I had a quick look at Alex's updated test cases and most of them are > based on an incorrect assumption I made around the behaviour of > fifo8_pop_buf(). Can you drop these for now, and I will submit a v3 > shortly once I've given it a full run through my test images? Hi, I also had some failures of the tests on CI, which is why I hadn't incorporated these changes yet. Thanks for the advance warning, I'll wait for your v3. Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer 2021-03-30 9:59 ` Paolo Bonzini @ 2021-04-01 7:56 ` Mark Cave-Ayland 0 siblings, 0 replies; 17+ messages in thread From: Mark Cave-Ayland @ 2021-04-01 7:56 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, alxndr, laurent On 30/03/2021 10:59, Paolo Bonzini wrote: > Hi, > > I also had some failures of the tests on CI, which is why I hadn't incorporated these > changes yet. Thanks for the advance warning, I'll wait for your v3. > > Paolo Hi Paolo, I've just posted the latest v3 which passes all my local boot tests and the extra test cases posted to LP. There is one failure on Gitlab CI but that is for the clang-user build for tcg-tests-s390x-linux-user which is an existing issue. Apologies it took a bit longer than expected: my laptop isn't the fastest in the world and booting everything will full debug and ASAN across several targets is tremendously slow :/ Also it seems there is something wrong with the qtest dependencies: for my current build of ~2900 files, the final commit to add the qtest for am53c974 which adds the test to test/qtest/meson.build causes ~2100 of those files to be rebuilt :( ATB, Mark. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-04-01 8:04 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-17 23:02 [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Mark Cave-Ayland 2021-03-17 23:02 ` [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present Mark Cave-Ayland 2021-03-17 23:58 ` Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 2/6] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size Mark Cave-Ayland 2021-03-17 23:02 ` [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL Mark Cave-Ayland 2021-03-18 0:10 ` [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL\ Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 4/6] esp: don't underflow fifo when writing to the device Mark Cave-Ayland 2021-03-18 0:12 ` Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks Mark Cave-Ayland 2021-03-18 0:12 ` Alexander Bulekov 2021-03-17 23:02 ` [PATCH v2 6/6] tests/qtest: add tests for am53c974 device Mark Cave-Ayland 2021-03-18 0:14 ` Alexander Bulekov 2021-03-18 12:10 ` Paolo Bonzini 2021-03-18 18:13 ` [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer Paolo Bonzini 2021-03-30 7:34 ` Mark Cave-Ayland 2021-03-30 9:59 ` Paolo Bonzini 2021-04-01 7:56 ` Mark Cave-Ayland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).