[PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer

[PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer

[Mark Cave-Ayland]

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

[PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present

[Mark Cave-Ayland]

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

Re: [PATCH v2 1/6] esp: don't underflow cmdfifo if no message out/command data is present

[Alexander Bulekov]

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>

[PATCH v2 2/6] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size

[Mark Cave-Ayland]

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

[PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL

[Mark Cave-Ayland]

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

Re: [PATCH v2 3/6] esp: ensure cmdfifo is not empty and current_dev is non-NULL\

[Alexander Bulekov]

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
> 

[PATCH v2 4/6] esp: don't underflow fifo when writing to the device

[Mark Cave-Ayland]

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

Re: [PATCH v2 4/6] esp: don't underflow fifo when writing to the device

[Alexander Bulekov]

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>

[PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks

[Mark Cave-Ayland]

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

Re: [PATCH v2 5/6] esp: always check current_req is not NULL before use in DMA callbacks

[Alexander Bulekov]

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>

[PATCH v2 6/6] tests/qtest: add tests for am53c974 device

[Mark Cave-Ayland]

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

Re: [PATCH v2 6/6] tests/qtest: add tests for am53c974 device

[Alexander Bulekov]

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

Re: [PATCH v2 6/6] tests/qtest: add tests for am53c974 device

[Paolo Bonzini]

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',
> 

Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer

[Paolo Bonzini]

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

Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer

[Mark Cave-Ayland]

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.

Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer

[Paolo Bonzini]

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

Re: [PATCH v2 0/6] esp: fix asserts/segfaults discovered by fuzzer

[Mark Cave-Ayland]

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.