[PATCH v4 for-6.0 00/12] esp: fix asserts/segfaults discovered by fuzzer

[PATCH v4 for-6.0 00/12] 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>

v4:
- Rebase onto master
- Add R-B tags from Phil
- Fix accidental line space removal in patch 3 discovered by Phil
- Change spelling of sanitiser -> sanitizer in patch 5 as suggested by Phil
- Fix up cmdfifo length checks in patch 8
- Add T-B tags from Alex
- Add patch 11 to handle additional assert discovered by Alex during fuzzing

v3:
- Rebase onto master
- Rearrange patch ordering (move patch 5 to the front) to help reduce cross-talk
  between the regression tests
- Introduce patch 2 to remove unnecessary FIFO usage
- Introduce patches 3-4 to consolidate esp_fifo_pop()/esp_fifo_push() wrapper
  functions to avoid having to introduce 2 variants of esp_fifo_pop_buf()
- Introduce esp_fifo_pop_buf() in patch 5 to prevent callers from overflowing
  the array used to model the FIFO
- Introduce patch 10 to clarify cancellation logic should all occur in the .cancel
  SCSI callback rather than at the site of the caller
- Add extra qtests in patch 11 to cover addition test cases provided on LP

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 (12):
  esp: always check current_req is not NULL before use in DMA callbacks
  esp: rework write_response() to avoid using the FIFO for DMA
    transactions
  esp: consolidate esp_cmdfifo_push() into esp_fifo_push()
  esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop()
  esp: introduce esp_fifo_pop_buf() and use it instead of
    fifo8_pop_buf()
  esp: ensure cmdfifo is not empty and current_dev is non-NULL
  esp: don't underflow cmdfifo in do_cmd()
  esp: don't overflow cmdfifo in get_cmd()
  esp: don't overflow cmdfifo if TC is larger than the cmdfifo size
  esp: don't reset async_len directly in esp_select() if cancelling
    request
  esp: ensure that do_cmd is set to zero before submitting an ESP select
    command
  tests/qtest: add tests for am53c974 device

 MAINTAINERS                 |   1 +
 hw/scsi/esp.c               | 119 +++++++++++---------
 tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
 tests/qtest/meson.build     |   1 +
 4 files changed, 285 insertions(+), 52 deletions(-)
 create mode 100644 tests/qtest/am53c974-test.c

-- 
2.20.1

[PATCH v4 for-6.0 01/12] 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>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
---
 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 507ab363bc..bafea0d4e6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -496,6 +496,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);
@@ -527,11 +531,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;
         }
 
@@ -604,6 +606,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;
@@ -713,6 +718,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

[PATCH v4 for-6.0 02/12] esp: rework write_response() to avoid using the FIFO for DMA transactions

[Mark Cave-Ayland]

The code for write_response() has always used the FIFO to store the data for
the status/message in phases, even for DMA transactions. Switch to using a
separate buffer that can be used directly for DMA transactions and restrict
the FIFO use to the non-DMA case.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
---
 hw/scsi/esp.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index bafea0d4e6..26fe1dcb9d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -445,18 +445,16 @@ static void write_response_pdma_cb(ESPState *s)
 
 static void write_response(ESPState *s)
 {
-    uint32_t n;
+    uint8_t buf[2];
 
     trace_esp_write_response(s->status);
 
-    fifo8_reset(&s->fifo);
-    esp_fifo_push(s, s->status);
-    esp_fifo_push(s, 0);
+    buf[0] = s->status;
+    buf[1] = 0;
 
     if (s->dma) {
         if (s->dma_memory_write) {
-            s->dma_memory_write(s->dma_opaque,
-                                (uint8_t *)fifo8_pop_buf(&s->fifo, 2, &n), 2);
+            s->dma_memory_write(s->dma_opaque, buf, 2);
             s->rregs[ESP_RSTAT] = STAT_TC | STAT_ST;
             s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC;
             s->rregs[ESP_RSEQ] = SEQ_CD;
@@ -466,7 +464,8 @@ static void write_response(ESPState *s)
             return;
         }
     } else {
-        s->ti_size = 2;
+        fifo8_reset(&s->fifo);
+        fifo8_push_all(&s->fifo, buf, 2);
         s->rregs[ESP_RFLAGS] = 2;
     }
     esp_raise_irq(s);
-- 
2.20.1

Re: [PATCH v4 for-6.0 02/12] esp: rework write_response() to avoid using the FIFO for DMA transactions

[Philippe Mathieu-Daudé]

On 4/7/21 9:57 PM, Mark Cave-Ayland wrote:
> The code for write_response() has always used the FIFO to store the data for
> the status/message in phases, even for DMA transactions. Switch to using a
> separate buffer that can be used directly for DMA transactions and restrict
> the FIFO use to the non-DMA case.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  hw/scsi/esp.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v4 for-6.0 03/12] esp: consolidate esp_cmdfifo_push() into esp_fifo_push()

[Mark Cave-Ayland]

Each FIFO currently has its own push functions with the only difference being
the capacity check. The original reason for this was that the fifo8
implementation doesn't have a formal API for retrieving the FIFO capacity,
however there are multiple examples within QEMU where the capacity field is
accessed directly.

Change esp_fifo_push() to access the FIFO capacity directly and then consolidate
esp_cmdfifo_push() into esp_fifo_push().

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 | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 26fe1dcb9d..16aaf8be93 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -98,16 +98,15 @@ void esp_request_cancelled(SCSIRequest *req)
     }
 }
 
-static void esp_fifo_push(ESPState *s, uint8_t val)
+static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
 {
-    if (fifo8_num_used(&s->fifo) == ESP_FIFO_SZ) {
+    if (fifo8_num_used(fifo) == fifo->capacity) {
         trace_esp_error_fifo_overrun();
         return;
     }
 
-    fifo8_push(&s->fifo, val);
+    fifo8_push(fifo, val);
 }
-
 static uint8_t esp_fifo_pop(ESPState *s)
 {
     if (fifo8_is_empty(&s->fifo)) {
@@ -117,16 +116,6 @@ static uint8_t esp_fifo_pop(ESPState *s)
     return fifo8_pop(&s->fifo);
 }
 
-static void esp_cmdfifo_push(ESPState *s, uint8_t val)
-{
-    if (fifo8_num_used(&s->cmdfifo) == ESP_CMDFIFO_SZ) {
-        trace_esp_error_fifo_overrun();
-        return;
-    }
-
-    fifo8_push(&s->cmdfifo, val);
-}
-
 static uint8_t esp_cmdfifo_pop(ESPState *s)
 {
     if (fifo8_is_empty(&s->cmdfifo)) {
@@ -187,9 +176,9 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
     }
 
     if (s->do_cmd) {
-        esp_cmdfifo_push(s, val);
+        esp_fifo_push(&s->cmdfifo, val);
     } else {
-        esp_fifo_push(s, val);
+        esp_fifo_push(&s->fifo, val);
     }
 
     dmalen--;
@@ -645,7 +634,7 @@ static void esp_do_dma(ESPState *s)
              */
             if (len < esp_get_tc(s) && esp_get_tc(s) <= ESP_FIFO_SZ) {
                 while (fifo8_num_used(&s->fifo) < ESP_FIFO_SZ) {
-                    esp_fifo_push(s, 0);
+                    esp_fifo_push(&s->fifo, 0);
                     len++;
                 }
             }
@@ -947,9 +936,9 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
         break;
     case ESP_FIFO:
         if (s->do_cmd) {
-            esp_cmdfifo_push(s, val);
+            esp_fifo_push(&s->cmdfifo, val);
         } else {
-            esp_fifo_push(s, val);
+            esp_fifo_push(&s->fifo, val);
         }
 
         /* Non-DMA transfers raise an interrupt after every byte */
-- 
2.20.1

[PATCH v4 for-6.0 04/12] esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop()

[Mark Cave-Ayland]

Each FIFO currently has its own pop functions with the only difference being
the capacity check. The original reason for this was that the fifo8
implementation doesn't have a formal API for retrieving the FIFO capacity,
however there are multiple examples within QEMU where the capacity field is
accessed directly.

Change esp_fifo_pop() to access the FIFO capacity directly and then consolidate
esp_cmdfifo_pop() into esp_fifo_pop().

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 | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 16aaf8be93..ff8fa73de9 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -107,22 +107,14 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
 
     fifo8_push(fifo, val);
 }
-static uint8_t esp_fifo_pop(ESPState *s)
-{
-    if (fifo8_is_empty(&s->fifo)) {
-        return 0;
-    }
-
-    return fifo8_pop(&s->fifo);
-}
 
-static uint8_t esp_cmdfifo_pop(ESPState *s)
+static uint8_t esp_fifo_pop(Fifo8 *fifo)
 {
-    if (fifo8_is_empty(&s->cmdfifo)) {
+    if (fifo8_is_empty(fifo)) {
         return 0;
     }
 
-    return fifo8_pop(&s->cmdfifo);
+    return fifo8_pop(fifo);
 }
 
 static uint32_t esp_get_tc(ESPState *s)
@@ -159,9 +151,9 @@ static uint8_t esp_pdma_read(ESPState *s)
     uint8_t val;
 
     if (s->do_cmd) {
-        val = esp_cmdfifo_pop(s);
+        val = esp_fifo_pop(&s->cmdfifo);
     } else {
-        val = esp_fifo_pop(s);
+        val = esp_fifo_pop(&s->fifo);
     }
 
     return val;
@@ -887,7 +879,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
             qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
             s->rregs[ESP_FIFO] = 0;
         } else {
-            s->rregs[ESP_FIFO] = esp_fifo_pop(s);
+            s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
         }
         val = s->rregs[ESP_FIFO];
         break;
-- 
2.20.1

[PATCH v4 for-6.0 05/12] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf()

[Mark Cave-Ayland]

The const pointer returned by fifo8_pop_buf() lies directly within the array used
to model the FIFO. Building with address sanitizers enabled shows that if the
caller expects a minimum number of bytes present then if the FIFO is nearly full,
the caller may unexpectedly access past the end of the array.

Introduce esp_fifo_pop_buf() which takes a destination buffer and performs a
memcpy() in it to guarantee that the caller cannot overwrite the FIFO array and
update all callers to use it. Similarly add underflow protection similar to
esp_fifo_push() and esp_fifo_pop() so that instead of triggering an assert()
the operation becomes a no-op.

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>
---
 hw/scsi/esp.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ff8fa73de9..1aa2caf57d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -117,6 +117,23 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo)
     return fifo8_pop(fifo);
 }
 
+static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+{
+    const uint8_t *buf;
+    uint32_t n;
+
+    if (maxlen == 0) {
+        return 0;
+    }
+
+    buf = fifo8_pop_buf(fifo, maxlen, &n);
+    if (dest) {
+        memcpy(dest, buf, n);
+    }
+
+    return n;
+}
+
 static uint32_t esp_get_tc(ESPState *s)
 {
     uint32_t dmalen;
@@ -241,11 +258,11 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
         if (dmalen == 0) {
             return 0;
         }
-        memcpy(buf, fifo8_pop_buf(&s->fifo, dmalen, &n), dmalen);
-        if (dmalen >= 3) {
+        n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
+        if (n >= 3) {
             buf[0] = buf[2] >> 5;
         }
-        fifo8_push_all(&s->cmdfifo, buf, dmalen);
+        fifo8_push_all(&s->cmdfifo, buf, n);
     }
     trace_esp_get_cmd(dmalen, target);
 
@@ -258,16 +275,16 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
 
 static void do_busid_cmd(ESPState *s, uint8_t busid)
 {
-    uint32_t n, cmdlen;
+    uint32_t cmdlen;
     int32_t datalen;
     int lun;
     SCSIDevice *current_lun;
-    uint8_t *buf;
+    uint8_t buf[ESP_CMDFIFO_SZ];
 
     trace_esp_do_busid_cmd(busid);
     lun = busid & 7;
     cmdlen = fifo8_num_used(&s->cmdfifo);
-    buf = (uint8_t *)fifo8_pop_buf(&s->cmdfifo, cmdlen, &n);
+    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
 
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
     s->current_req = scsi_req_new(current_lun, 0, lun, buf, s);
@@ -300,13 +317,12 @@ static void do_busid_cmd(ESPState *s, uint8_t busid)
 static void do_cmd(ESPState *s)
 {
     uint8_t busid = fifo8_pop(&s->cmdfifo);
-    uint32_t n;
 
     s->cmdfifo_cdb_offset--;
 
     /* Ignore extended messages for now */
     if (s->cmdfifo_cdb_offset) {
-        fifo8_pop_buf(&s->cmdfifo, s->cmdfifo_cdb_offset, &n);
+        esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset);
         s->cmdfifo_cdb_offset = 0;
     }
 
@@ -484,7 +500,7 @@ 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);
+        n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
         s->async_buf += n;
         s->async_len -= n;
         s->ti_size += n;
@@ -492,7 +508,7 @@ static void do_dma_pdma_cb(ESPState *s)
         if (n < len) {
             /* Unaligned accesses can cause FIFO wraparound */
             len = len - n;
-            memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len);
+            n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
             s->async_buf += n;
             s->async_len -= n;
             s->ti_size += n;
@@ -668,7 +684,7 @@ static void esp_do_dma(ESPState *s)
 static void esp_do_nodma(ESPState *s)
 {
     int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO);
-    uint32_t cmdlen, n;
+    uint32_t cmdlen;
     int len;
 
     if (s->do_cmd) {
@@ -709,7 +725,7 @@ 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);
+        esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
         s->async_buf += len;
         s->async_len -= len;
         s->ti_size += len;
-- 
2.20.1

Re: [PATCH v4 for-6.0 05/12] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf()

[Philippe Mathieu-Daudé]

On 4/7/21 9:57 PM, Mark Cave-Ayland wrote:
> The const pointer returned by fifo8_pop_buf() lies directly within the array used
> to model the FIFO. Building with address sanitizers enabled shows that if the
> caller expects a minimum number of bytes present then if the FIFO is nearly full,
> the caller may unexpectedly access past the end of the array.
> 
> Introduce esp_fifo_pop_buf() which takes a destination buffer and performs a
> memcpy() in it to guarantee that the caller cannot overwrite the FIFO array and
> update all callers to use it. Similarly add underflow protection similar to
> esp_fifo_push() and esp_fifo_pop() so that instead of triggering an assert()
> the operation becomes a no-op.
> 
> 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>
> ---
>  hw/scsi/esp.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)

Way cleaner/safer.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v4 for-6.0 05/12] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf()

[Peter Maydell]

On Wed, 7 Apr 2021 at 21:02, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> The const pointer returned by fifo8_pop_buf() lies directly within the array used
> to model the FIFO. Building with address sanitizers enabled shows that if the
> caller expects a minimum number of bytes present then if the FIFO is nearly full,
> the caller may unexpectedly access past the end of the array.
>
> Introduce esp_fifo_pop_buf() which takes a destination buffer and performs a
> memcpy() in it to guarantee that the caller cannot overwrite the FIFO array and
> update all callers to use it. Similarly add underflow protection similar to
> esp_fifo_push() and esp_fifo_pop() so that instead of triggering an assert()
> the operation becomes a no-op.
>
> 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>
> ---
>  hw/scsi/esp.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index ff8fa73de9..1aa2caf57d 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -117,6 +117,23 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo)
>      return fifo8_pop(fifo);
>  }
>
> +static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
> +{
> +    const uint8_t *buf;
> +    uint32_t n;
> +
> +    if (maxlen == 0) {
> +        return 0;
> +    }
> +
> +    buf = fifo8_pop_buf(fifo, maxlen, &n);
> +    if (dest) {
> +        memcpy(dest, buf, n);
> +    }
> +
> +    return n;
> +}
> +
>  static uint32_t esp_get_tc(ESPState *s)
>  {
>      uint32_t dmalen;
> @@ -241,11 +258,11 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
>          if (dmalen == 0) {
>              return 0;
>          }
> -        memcpy(buf, fifo8_pop_buf(&s->fifo, dmalen, &n), dmalen);
> -        if (dmalen >= 3) {
> +        n = esp_fifo_pop_buf(&s->fifo, buf, dmalen);
> +        if (n >= 3) {
>              buf[0] = buf[2] >> 5;
>          }
> -        fifo8_push_all(&s->cmdfifo, buf, dmalen);
> +        fifo8_push_all(&s->cmdfifo, buf, n);
>      }
>      trace_esp_get_cmd(dmalen, target);

This bit could be tidied up further -- it currently sets
   dmalen = MIN(fifo8_num_used(&s->fifo), maxlen);
in order not to pull more bytes out of the FIFO than it has in it;
but now we are making that check in esp_fifo_pop_buf() I think you
could just do
    dmalen = esp_fifo_pop_buf(&s->fifo, buf, maxlen);
and drop the
        dmalen = MIN(fifo8_num_used(&s->fifo), maxlen);
        if (dmalen == 0) {
            return 0;
        }
part and the use of 'n' entirely.

But this code isn't wrong, so we can do that later to avoid having
to respin here.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v4 for-6.0 06/12] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
---
 hw/scsi/esp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1aa2caf57d..4decbbfc29 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -284,6 +284,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;
+    }
     esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
 
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
-- 
2.20.1

[PATCH v4 for-6.0 07/12] esp: don't underflow cmdfifo in do_cmd()

[Mark Cave-Ayland]

If the guest tries to execute a CDB when cmdfifo is not empty before the start
of the message out phase then clearing the message out phase data will cause
cmdfifo to underflow due to cmdfifo_cdb_offset being larger than the amount of
data within.

Since this can only occur by issuing deliberately incorrect instruction
sequences, ensure that the maximum length of esp_fifo_pop_buf() is limited to
the size of the data within cmdfifo.

Buglink: https://bugs.launchpad.net/qemu/+bug/1909247
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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4decbbfc29..7f49522e1d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -319,13 +319,15 @@ 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 = esp_fifo_pop(&s->cmdfifo);
+    int len;
 
     s->cmdfifo_cdb_offset--;
 
     /* Ignore extended messages for now */
     if (s->cmdfifo_cdb_offset) {
-        esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset);
+        len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
+        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
         s->cmdfifo_cdb_offset = 0;
     }
 
-- 
2.20.1

[PATCH v4 for-6.0 08/12] esp: don't overflow cmdfifo in get_cmd()

[Mark Cave-Ayland]

If the guest tries to read a CDB using DMA and cmdfifo is not empty then it is
possible to overflow cmdfifo.

Since this can only occur by issuing deliberately incorrect instruction
sequences, ensure that the maximum length of the CDB transferred to cmdfifo is
limited to the available free space within cmdfifo.

Buglink: https://bugs.launchpad.net/qemu/+bug/1909247
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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 7f49522e1d..53cc569e8a 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -243,6 +243,7 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
         }
         if (s->dma_memory_read) {
             s->dma_memory_read(s->dma_opaque, buf, dmalen);
+            dmalen = MIN(fifo8_num_free(&s->cmdfifo), dmalen);
             fifo8_push_all(&s->cmdfifo, buf, dmalen);
         } else {
             if (esp_select(s) < 0) {
@@ -262,6 +263,7 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
         if (n >= 3) {
             buf[0] = buf[2] >> 5;
         }
+        n = MIN(fifo8_num_free(&s->cmdfifo), n);
         fifo8_push_all(&s->cmdfifo, buf, n);
     }
     trace_esp_get_cmd(dmalen, target);
-- 
2.20.1

[PATCH v4 for-6.0 09/12] 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 53cc569e8a..782c6ee357 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -578,6 +578,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 v4 for-6.0 10/12] esp: don't reset async_len directly in esp_select() if cancelling request

[Mark Cave-Ayland]

Instead let the SCSI layer invoke the .cancel callback itself to cancel and
reset the request state.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
---
 hw/scsi/esp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 782c6ee357..3b9037e4f4 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -95,6 +95,7 @@ void esp_request_cancelled(SCSIRequest *req)
         scsi_req_unref(s->current_req);
         s->current_req = NULL;
         s->current_dev = NULL;
+        s->async_len = 0;
     }
 }
 
@@ -206,7 +207,6 @@ static int esp_select(ESPState *s)
     if (s->current_req) {
         /* Started a new command before the old one finished.  Cancel it.  */
         scsi_req_cancel(s->current_req);
-        s->async_len = 0;
     }
 
     s->current_dev = scsi_device_find(&s->bus, 0, target, 0);
-- 
2.20.1

Re: [PATCH v4 for-6.0 10/12] esp: don't reset async_len directly in esp_select() if cancelling request

[Philippe Mathieu-Daudé]

On 4/7/21 9:57 PM, Mark Cave-Ayland wrote:
> Instead let the SCSI layer invoke the .cancel callback itself to cancel and
> reset the request state.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  hw/scsi/esp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v4 for-6.0 11/12] esp: ensure that do_cmd is set to zero before submitting an ESP select command

[Mark Cave-Ayland]

When a CDB has been received and is about to be submitted to the SCSI layer
via one of the ESP select commands, ensure that do_cmd is set to zero before
executing the command.

Otherwise a guest executing 2 valid CDBs in quick sequence can invoke the SCSI
.transfer_data callback again before do_cmd is set to zero by the callback
function triggering an assert at the start of esp_transfer_data().

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3b9037e4f4..326643aa39 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -357,6 +357,7 @@ static void handle_satn(ESPState *s)
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 1;
+        s->do_cmd = 0;
         do_cmd(s);
     } else if (cmdlen == 0) {
         s->do_cmd = 1;
@@ -390,6 +391,7 @@ static void handle_s_without_atn(ESPState *s)
     cmdlen = get_cmd(s, ESP_CMDFIFO_SZ);
     if (cmdlen > 0) {
         s->cmdfifo_cdb_offset = 0;
+        s->do_cmd = 0;
         do_busid_cmd(s, 0);
     } else if (cmdlen == 0) {
         s->do_cmd = 1;
-- 
2.20.1

Re: [PATCH v4 for-6.0 11/12] esp: ensure that do_cmd is set to zero before submitting an ESP select command

[Philippe Mathieu-Daudé]

On 4/7/21 9:58 PM, Mark Cave-Ayland wrote:
> When a CDB has been received and is about to be submitted to the SCSI layer
> via one of the ESP select commands, ensure that do_cmd is set to zero before
> executing the command.
> 
> Otherwise a guest executing 2 valid CDBs in quick sequence can invoke the SCSI
> .transfer_data callback again before do_cmd is set to zero by the callback
> function triggering an assert at the start of esp_transfer_data().
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v4 for-6.0 12/12] 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>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
---
 MAINTAINERS                 |   1 +
 tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
 tests/qtest/meson.build     |   1 +
 3 files changed, 218 insertions(+)
 create mode 100644 tests/qtest/am53c974-test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 58f342108e..fa258b7a92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1772,6 +1772,7 @@ F: include/hw/scsi/*
 F: hw/scsi/*
 F: tests/qtest/virtio-scsi-test.c
 F: tests/qtest/fuzz-virtio-scsi-test.c
+F: tests/qtest/am53c974-test.c
 T: git https://github.com/bonzini/qemu.git scsi-next
 
 SSI
diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
new file mode 100644
index 0000000000..9b06f2cf45
--- /dev/null
+++ b/tests/qtest/am53c974-test.c
@@ -0,0 +1,216 @@
+/*
+ * 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);
+}
+
+/* Reported as crash_1548bd10e7 */
+static void test_cmdfifo_underflow2_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_outw(s, 0xc00c, 0x41);
+    qtest_outw(s, 0xc00a, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00c, 0x43);
+    qtest_outw(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00c, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00a, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00c, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00a, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00c, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00a, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00c, 0x00);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outl(s, 0xc006, 0x00);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00b, 0x0800);
+    qtest_outw(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00b, 0x00);
+    qtest_outl(s, 0xc006, 0x00);
+    qtest_outl(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00b, 0x0800);
+    qtest_outw(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc00b, 0x4100);
+    qtest_outw(s, 0xc00a, 0x00);
+    qtest_outl(s, 0xc00a, 0x100000);
+    qtest_outl(s, 0xc00a, 0x00);
+    qtest_outw(s, 0xc00c, 0x43);
+    qtest_outl(s, 0xc00a, 0x100000);
+    qtest_outl(s, 0xc00a, 0x100000);
+    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);
+}
+
+/* Reported as crash_530ff2e211 */
+static void test_cmdfifo_overflow2_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, 0xc00b, 0x4100);
+    qtest_outw(s, 0xc00b, 0xc200);
+    qtest_outl(s, 0xc03f, 0x0300);
+    qtest_quit(s);
+}
+
+/* Reported as crash_0900379669 */
+static void test_fifo_pop_buf(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_outb(s, 0xc000, 0x4);
+    qtest_outb(s, 0xc008, 0xa0);
+    qtest_outl(s, 0xc03f, 0x0300);
+    qtest_outl(s, 0xc00b, 0xc300);
+    qtest_outw(s, 0xc00b, 0x9000);
+    qtest_outl(s, 0xc00b, 0xc300);
+    qtest_outl(s, 0xc00b, 0xc300);
+    qtest_outl(s, 0xc00b, 0xc300);
+    qtest_outw(s, 0xc00b, 0x9000);
+    qtest_outw(s, 0xc00b, 0x1000);
+    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_underflow2_ok",
+                       test_cmdfifo_underflow2_ok);
+        qtest_add_func("am53c974/test_cmdfifo_overflow_ok",
+                       test_cmdfifo_overflow_ok);
+        qtest_add_func("am53c974/test_cmdfifo_overflow2_ok",
+                       test_cmdfifo_overflow2_ok);
+        qtest_add_func("am53c974/test_fifo_pop_buf",
+                       test_fifo_pop_buf);
+        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 902cfef7cb..25f605cf1d 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -68,6 +68,7 @@ qtests_i386 = \
   (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_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) +   \
+  (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) +                 \
   qtests_pci +                                                                              \
   ['fdc-test',
    'ide-test',
-- 
2.20.1

Re: [PATCH v4 for-6.0 12/12] tests/qtest: add tests for am53c974 device

[Peter Maydell]

On Wed, 7 Apr 2021 at 21:16, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> 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>
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>  MAINTAINERS                 |   1 +
>  tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
>  tests/qtest/meson.build     |   1 +
>  3 files changed, 218 insertions(+)
>  create mode 100644 tests/qtest/am53c974-test.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 58f342108e..fa258b7a92 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1772,6 +1772,7 @@ F: include/hw/scsi/*
>  F: hw/scsi/*
>  F: tests/qtest/virtio-scsi-test.c
>  F: tests/qtest/fuzz-virtio-scsi-test.c
> +F: tests/qtest/am53c974-test.c
>  T: git https://github.com/bonzini/qemu.git scsi-next
>
>  SSI
> diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
> new file mode 100644
> index 0000000000..9b06f2cf45
> --- /dev/null
> +++ b/tests/qtest/am53c974-test.c
> @@ -0,0 +1,216 @@
> +/*
> + * 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.
> + */

No copyright line ?

thanks
-- PMM

Re: [PATCH v4 for-6.0 00/12] esp: fix asserts/segfaults discovered by fuzzer

[Mark Cave-Ayland]

On 07/04/2021 20:57, Mark Cave-Ayland wrote:

(added Peter to CC)

> 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>
> 
> v4:
> - Rebase onto master
> - Add R-B tags from Phil
> - Fix accidental line space removal in patch 3 discovered by Phil
> - Change spelling of sanitiser -> sanitizer in patch 5 as suggested by Phil
> - Fix up cmdfifo length checks in patch 8
> - Add T-B tags from Alex
> - Add patch 11 to handle additional assert discovered by Alex during fuzzing
> 
> v3:
> - Rebase onto master
> - Rearrange patch ordering (move patch 5 to the front) to help reduce cross-talk
>    between the regression tests
> - Introduce patch 2 to remove unnecessary FIFO usage
> - Introduce patches 3-4 to consolidate esp_fifo_pop()/esp_fifo_push() wrapper
>    functions to avoid having to introduce 2 variants of esp_fifo_pop_buf()
> - Introduce esp_fifo_pop_buf() in patch 5 to prevent callers from overflowing
>    the array used to model the FIFO
> - Introduce patch 10 to clarify cancellation logic should all occur in the .cancel
>    SCSI callback rather than at the site of the caller
> - Add extra qtests in patch 11 to cover addition test cases provided on LP
> 
> 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 (12):
>    esp: always check current_req is not NULL before use in DMA callbacks
>    esp: rework write_response() to avoid using the FIFO for DMA
>      transactions
>    esp: consolidate esp_cmdfifo_push() into esp_fifo_push()
>    esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop()
>    esp: introduce esp_fifo_pop_buf() and use it instead of
>      fifo8_pop_buf()
>    esp: ensure cmdfifo is not empty and current_dev is non-NULL
>    esp: don't underflow cmdfifo in do_cmd()
>    esp: don't overflow cmdfifo in get_cmd()
>    esp: don't overflow cmdfifo if TC is larger than the cmdfifo size
>    esp: don't reset async_len directly in esp_select() if cancelling
>      request
>    esp: ensure that do_cmd is set to zero before submitting an ESP select
>      command
>    tests/qtest: add tests for am53c974 device
> 
>   MAINTAINERS                 |   1 +
>   hw/scsi/esp.c               | 119 +++++++++++---------
>   tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
>   tests/qtest/meson.build     |   1 +
>   4 files changed, 285 insertions(+), 52 deletions(-)
>   create mode 100644 tests/qtest/am53c974-test.c

Hi Paolo,

Is this still a candidate for 6.0 as you suggested in your response to v1? There is 
also the related ESP fix for the SPARC acceptance test failure which I think is also 
appropriate for 6.0.

If so, who would be able to review/merge both these ESP patches? Given that we're 
already at -rc2 I'm aware that it's getting quite close to the 6.0 release.


ATB,

Mark.