* [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
* [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
* [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
* [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
* [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 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
* 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
* 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
* 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
* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.