[PATCH v3 00/11] esp: fix asserts/segfaults discovered by fuzzer

[PATCH v3 00/11] 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>

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 (11):
  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
  tests/qtest: add tests for am53c974 device

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

-- 
2.20.1

[PATCH v3 01/11] 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 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 v3 02/11] 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>
---
 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 v3 02/11] esp: rework write_response() to avoid using the FIFO for DMA transactions

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, 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>
> ---
>  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;

Slightly simplified:

       const uint8_t buf[2] = { s->status, 0 };

>      if (s->dma) {

           uint32_t n;

>          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);
> 

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

[PATCH v3 03/11] 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>
---
 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

Re: [PATCH v3 03/11] esp: consolidate esp_cmdfifo_push() into esp_fifo_push()

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> 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.

So the Fifo8 API is not complete / practical.

> 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>
> ---
>  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);
>  }
> -

Spurious line removal?

>  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);
>  }

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

Re: [PATCH v3 03/11] esp: consolidate esp_cmdfifo_push() into esp_fifo_push()

[Mark Cave-Ayland]

On 01/04/2021 09:15, Philippe Mathieu-Daudé wrote:

> On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
>> 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.
> 
> So the Fifo8 API is not complete / practical.

I guess it depends what you consider to be public and private to Fifo8: what is 
arguably missing is something like:

int fifo8_capacity(Fifo8 *fifo)
{
     return fifo->capacity;
}

But given that most other users access fifo->capacity directly then I might as well 
do the same and save myself requiring 2 separate implementations of esp_fifo_pop_buf() :)

>> 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>
>> ---
>>   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);
>>   }
>> -
> 
> Spurious line removal?

Ooops yes. I'm not too worried about this, but if Paolo has any other changes then I 
can roll this into a v4.

>>   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);
>>   }
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


ATB,

Mark.

Re: [PATCH v3 03/11] esp: consolidate esp_cmdfifo_push() into esp_fifo_push()

[Philippe Mathieu-Daudé]

On 4/1/21 10:50 AM, Mark Cave-Ayland wrote:
> On 01/04/2021 09:15, Philippe Mathieu-Daudé wrote:
> 
>> On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
>>> 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.
>>
>> So the Fifo8 API is not complete / practical.
> 
> I guess it depends what you consider to be public and private to Fifo8:
> what is arguably missing is something like:
> 
> int fifo8_capacity(Fifo8 *fifo)
> {
>     return fifo->capacity;
> }
> 
> But given that most other users access fifo->capacity directly then I
> might as well do the same and save myself requiring 2 separate
> implementations of esp_fifo_pop_buf() :)

Sorry, I should have been more explicit by precising this was not
a comment directed to your patch, but I was thinking loudly about
the Fifo8 API; and as you mentioned the other models do that so your
kludge is acceptable.
>>> 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>
>>> ---
>>>   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);
>>>   }
>>> -
>>
>> Spurious line removal?
> 
> Ooops yes. I'm not too worried about this, but if Paolo has any other
> changes then I can roll this into a v4.

Actually it reappears in patch 05/11 ;)

[PATCH v3 04/11] 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>
---
 hw/scsi/esp.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 16aaf8be93..ce88866803 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -107,22 +107,13 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
 
     fifo8_push(fifo, val);
 }
-static uint8_t esp_fifo_pop(ESPState *s)
+static uint8_t esp_fifo_pop(Fifo8 *fifo)
 {
-    if (fifo8_is_empty(&s->fifo)) {
+    if (fifo8_is_empty(fifo)) {
         return 0;
     }
 
-    return fifo8_pop(&s->fifo);
-}
-
-static uint8_t esp_cmdfifo_pop(ESPState *s)
-{
-    if (fifo8_is_empty(&s->cmdfifo)) {
-        return 0;
-    }
-
-    return fifo8_pop(&s->cmdfifo);
+    return fifo8_pop(fifo);
 }
 
 static uint32_t esp_get_tc(ESPState *s)
@@ -159,9 +150,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 +878,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

Re: [PATCH v3 04/11] esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop()

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> 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>
> ---
>  hw/scsi/esp.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)

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

[PATCH v3 05/11] 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 sanitisers 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>
---
 hw/scsi/esp.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ce88866803..1aa2caf57d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -107,6 +107,7 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
 
     fifo8_push(fifo, val);
 }
+
 static uint8_t esp_fifo_pop(Fifo8 *fifo)
 {
     if (fifo8_is_empty(fifo)) {
@@ -116,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;
@@ -240,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);
 
@@ -257,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);
@@ -299,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;
     }
 
@@ -483,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;
@@ -491,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;
@@ -667,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) {
@@ -708,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 v3 05/11] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf()

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, 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 sanitisers enabled shows that if the

Typo "sanitizers"

> 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.

Why isn't it a problem with the other models? Because the pointed
buffer is consumed directly?

> 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.

This is OK for your ESP model.

Now thinking loudly about the Fifo8 API, shouldn't this be part of it?

Something prototype like:

  /**
   * fifo8_pop_buf:
   * @do_copy: If %true, also copy data to @bufptr.
   */
  size_t fifo8_pop_buf(Fifo8 *fifo,
                       void **bufptr,
                       size_t buflen,
                       bool do_copy);

> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1909247
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index ce88866803..1aa2caf57d 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -107,6 +107,7 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
>  
>      fifo8_push(fifo, val);
>  }
> +
>  static uint8_t esp_fifo_pop(Fifo8 *fifo)
>  {
>      if (fifo8_is_empty(fifo)) {
> @@ -116,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;
> +}

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

Re: [PATCH v3 05/11] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf()

[Mark Cave-Ayland]

On 01/04/2021 10:34, Philippe Mathieu-Daudé wrote:

> On 4/1/21 9:49 AM, 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 sanitisers enabled shows that if the
> 
> Typo "sanitizers"

Ha. It's definitely "sanitiser" here in the UK (UK English) as opposed to "sanitizer" 
(US English). I don't really mind either way, but I can fix this if it needs a v4 
following Paolo's comments.

>> 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.
> 
> Why isn't it a problem with the other models? Because the pointed
> buffer is consumed directly?

Yes that's correct, which is why Fifo8 currently doesn't support wraparound. I 
haven't analysed how other devices have used it but I would imagine there would be an 
ASan hit if it were being misused this way.

>> 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.
> 
> This is OK for your ESP model.
> 
> Now thinking loudly about the Fifo8 API, shouldn't this be part of it?
> 
> Something prototype like:
> 
>    /**
>     * fifo8_pop_buf:
>     * @do_copy: If %true, also copy data to @bufptr.
>     */
>    size_t fifo8_pop_buf(Fifo8 *fifo,
>                         void **bufptr,
>                         size_t buflen,
>                         bool do_copy);

That could work, and may even allow support for wraparound in future. I suspect 
things would become clearer after looking at the other Fifo8 users to see if this is 
worth an API change/alternative API.


ATB,

Mark.

Re: [PATCH v3 05/11] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf()

[Philippe Mathieu-Daudé]

On 4/1/21 12:51 PM, Mark Cave-Ayland wrote:
> On 01/04/2021 10:34, Philippe Mathieu-Daudé wrote:
>> On 4/1/21 9:49 AM, 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 sanitisers enabled shows
>>> that if the
>>
>> Typo "sanitizers"
> 
> Ha. It's definitely "sanitiser" here in the UK (UK English) as opposed
> to "sanitizer" (US English).

Oh OK, TIL :)

> I don't really mind either way, but I can
> fix this if it needs a v4 following Paolo's comments.

Not needed since it is correct.

>>> 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.
>>
>> Why isn't it a problem with the other models? Because the pointed
>> buffer is consumed directly?
> 
> Yes that's correct, which is why Fifo8 currently doesn't support
> wraparound. I haven't analysed how other devices have used it but I
> would imagine there would be an ASan hit if it were being misused this way.
> 
>>> 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.
>>
>> This is OK for your ESP model.
>>
>> Now thinking loudly about the Fifo8 API, shouldn't this be part of it?
>>
>> Something prototype like:
>>
>>    /**
>>     * fifo8_pop_buf:
>>     * @do_copy: If %true, also copy data to @bufptr.
>>     */
>>    size_t fifo8_pop_buf(Fifo8 *fifo,
>>                         void **bufptr,
>>                         size_t buflen,
>>                         bool do_copy);
> 
> That could work, and may even allow support for wraparound in future. I
> suspect things would become clearer after looking at the other Fifo8
> users to see if this is worth an API change/alternative API.

Thanks for the feedback!

Phil.

[PATCH v3 06/11] 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 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

Re: [PATCH v3 06/11] esp: ensure cmdfifo is not empty and current_dev is non-NULL

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, 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(+)

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

[PATCH v3 07/11] 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>
---
 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

Re: [PATCH v3 07/11] esp: don't underflow cmdfifo in do_cmd()

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> 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>
> ---
>  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));

Do we want to log(GUEST_ERRORS) this?

> +        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
>          s->cmdfifo_cdb_offset = 0;
>      }
>  
> 

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

Re: [PATCH v3 07/11] esp: don't underflow cmdfifo in do_cmd()

[Mark Cave-Ayland]

On 01/04/2021 09:19, Philippe Mathieu-Daudé wrote:

> On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
>> 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>
>> ---
>>   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));
> 
> Do we want to log(GUEST_ERRORS) this?

Not for this case, since a guest OS may legitimately try a SDTR negotiation using 
extended messages which the ESP implementation currently ignores.

>> +        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
>>           s->cmdfifo_cdb_offset = 0;
>>       }
>>   
>>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


ATB,

Mark.

[PATCH v3 08/11] 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>
---
 hw/scsi/esp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 7f49522e1d..c547c60395 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->fifo), dmalen);
             fifo8_push_all(&s->cmdfifo, buf, dmalen);
         } else {
             if (esp_select(s) < 0) {
-- 
2.20.1

Re: [PATCH v3 08/11] esp: don't overflow cmdfifo in get_cmd()

[Philippe Mathieu-Daudé]

On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> 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>
> ---
>  hw/scsi/esp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 7f49522e1d..c547c60395 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->fifo), dmalen);

Ditto, GUEST_ERRORS?

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

>              fifo8_push_all(&s->cmdfifo, buf, dmalen);
>          } else {
>              if (esp_select(s) < 0) {
> 

Re: [PATCH v3 08/11] esp: don't overflow cmdfifo in get_cmd()

[Mark Cave-Ayland]

On 01/04/2021 09:19, Philippe Mathieu-Daudé wrote:

> On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
>> 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>
>> ---
>>   hw/scsi/esp.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 7f49522e1d..c547c60395 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->fifo), dmalen);
> 
> Ditto, GUEST_ERRORS?

Possibly? But then there are several other places where this could also happen. The 
ESP uses the FIFO as a buffer for the SCSI bus in DMA mode, and so at the start of a 
DMA transfer the FIFO can contain leftover junk. This is why all the guest OSs I've 
seen send an explicit "Flush FIFO" command before each command to ensure that any 
junk is removed from the FIFO before sending the message out/CDB.

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>>               fifo8_push_all(&s->cmdfifo, buf, dmalen);
>>           } else {
>>               if (esp_select(s) < 0) {
>>

ATB,

Mark.

[PATCH v3 09/11] 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 c547c60395..b7f2680617 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -577,6 +577,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 v3 10/11] 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>
---
 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 b7f2680617..ca062a0400 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

[PATCH v3 11/11] 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>
---
 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 554be84b32..675f35d3af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1776,6 +1776,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..9c4285d0c0
--- /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(
+        "-m 512M -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(
+        "-m 512M -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(
+        "-m 512M -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 v3 11/11] tests/qtest: add tests for am53c974 device

[Alexander Bulekov]

On 210401 0849, 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>
> ---
>  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 554be84b32..675f35d3af 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1776,6 +1776,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..9c4285d0c0
> --- /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);
> +}
> +

Hi Mark,
> +/* Reported as crash_1548bd10e7 */
                        ^^^
These numbers were just the filename/hash of the crashing test-case. I'm
not sure if they are useful to keep them around - I just needed some way
to name a bunch of functions :)
-Alex

Re: [PATCH v3 11/11] tests/qtest: add tests for am53c974 device

[Mark Cave-Ayland]

On 01/04/2021 17:55, Alexander Bulekov wrote:

> On 210401 0849, 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>
>> ---
>>   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 554be84b32..675f35d3af 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1776,6 +1776,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..9c4285d0c0
>> --- /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);
>> +}
>> +
> 
> Hi Mark,
>> +/* Reported as crash_1548bd10e7 */
>                          ^^^
> These numbers were just the filename/hash of the crashing test-case. I'm
> not sure if they are useful to keep them around - I just needed some way
> to name a bunch of functions :)
> -Alex

Hi Alex,

No worries, I figured as much but thought it was worth keeping them in case anyone 
was curious enough to want to read the original LP bug report :)


ATB,

Mark.

Re: [PATCH v3 00/11] esp: fix asserts/segfaults discovered by fuzzer

[Alexander Bulekov]

On 210401 0849, 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>
> 
> 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
> 

Hi Mark,
I applied this and ran through the whole fuzzer corpus, and all I'm
seeing are just a few assertion failures:
handle_satn_stop -> get_cmd -> util/fifo8.c:43:5 and
hw/scsi/esp.c:790:5

Tested-by: Alexander Bulekov <alxndr@bu.edu>

Thank you
-Alex

> 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 (11):
>   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
>   tests/qtest: add tests for am53c974 device
> 
>  MAINTAINERS                 |   1 +
>  hw/scsi/esp.c               | 116 ++++++++++---------
>  tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
>  tests/qtest/meson.build     |   1 +
>  4 files changed, 282 insertions(+), 52 deletions(-)
>  create mode 100644 tests/qtest/am53c974-test.c
> 
> -- 
> 2.20.1
> 

Re: [PATCH v3 00/11] esp: fix asserts/segfaults discovered by fuzzer

[Mark Cave-Ayland]

On 01/04/2021 18:00, Alexander Bulekov wrote:

> On 210401 0849, 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>
>>
>> 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
>>
> 
> Hi Mark,
> I applied this and ran through the whole fuzzer corpus, and all I'm
> seeing are just a few assertion failures:
> handle_satn_stop -> get_cmd -> util/fifo8.c:43:5 and
> hw/scsi/esp.c:790:5
> 
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> 
> Thank you
> -Alex

Thanks for the testing! I've just realised that there is an error in the get_cmd() 
MIN() check (it should be cmdfifo, not fifo) and also the limit is missing from the 
non-DMA path.

Does the following patch on v3 fix the outstanding get_cmd() asserts?

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ca062a0400..3b9037e4f4 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -243,7 +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->fifo), dmalen);
+            dmalen = MIN(fifo8_num_free(&s->cmdfifo), dmalen);
              fifo8_push_all(&s->cmdfifo, buf, dmalen);
          } else {
              if (esp_select(s) < 0) {
@@ -263,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);

Given that there is going to be a v4 now, if you are able to provide a handful of 
test cases for hw/scsi/esp.c:790:5 as a diff on v3 like you did before then I can 
take a quick look.


ATB,

Mark.

[PATCH] tests/qtest: add one more test for the am53c974

[Alexander Bulekov]

Original crash:
qemu-fuzz-i386: ../hw/scsi/esp.c:791: void esp_transfer_data(SCSIRequest *, uint32_t): Assertion `!s->do_cmd' failed.
==257532== ERROR: libFuzzer: deadly signal
__assert_fail assert/assert.c:101:3
esp_transfer_data hw/scsi/esp.c:791:5
scsi_req_data hw/scsi/scsi-bus.c:1412:9
scsi_disk_emulate_read_data hw/scsi/scsi-disk.c:1407:9
scsi_req_continue hw/scsi/scsi-bus.c:1394:9
do_busid_cmd hw/scsi/esp.c:317:9
handle_s_without_atn hw/scsi/esp.c:393:9
esp_reg_write hw/scsi/esp.c:1029:13
esp_pci_io_write hw/scsi/esp-pci.c:215:9
memory_region_write_accessor softmmu/memory.c:491:5
access_with_adjusted_size softmmu/memory.c:552:18
memory_region_dispatch_write softmmu/memory.c:1502:16
flatview_write_continue softmmu/physmem.c:2746:23
flatview_write softmmu/physmem.c:2786:14
address_space_write softmmu/physmem.c:2878:18
cpu_outl softmmu/ioport.c:80:5

Based-on: <20210401074933.9923-1-mark.cave-ayland@ilande.co.uk>
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
 tests/qtest/am53c974-test.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

The patch took care of the handle_satn_stop assert. Here's a test case
for the other assert.

Pasteable:

cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
512M -device am53c974,id=scsi -device scsi-hd,drive=disk0 -drive \
id=disk0,if=none,file=null-co://,format=raw -nodefaults -qtest stdio
outl 0xcf8 0x80001010
outl 0xcfc 0xc000
outl 0xcf8 0x80001004
outw 0xcfc 0x01
outl 0xc00b 0x4100
outb 0xc008 0x42
outw 0xc03f 0x0300
outl 0xc00b 0xc100
EOF


diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
index 9c4285d0c0..506276677a 100644
--- a/tests/qtest/am53c974-test.c
+++ b/tests/qtest/am53c974-test.c
@@ -9,6 +9,22 @@
 
 #include "libqos/libqtest.h"
 
+static void test_do_cmd_assert(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_outb(s, 0xc008, 0x42);
+    qtest_outw(s, 0xc03f, 0x0300);
+    qtest_outl(s, 0xc00b, 0xc100);
+    qtest_quit(s);
+}
 
 static void test_cmdfifo_underflow_ok(void)
 {
@@ -194,6 +210,8 @@ int main(int argc, char **argv)
     g_test_init(&argc, &argv, NULL);
 
     if (strcmp(arch, "i386") == 0) {
+        qtest_add_func("am53c974/test_do_cmd_asser",
+                       test_do_cmd_assert);
         qtest_add_func("am53c974/test_cmdfifo_underflow_ok",
                        test_cmdfifo_underflow_ok);
         qtest_add_func("am53c974/test_cmdfifo_underflow2_ok",
-- 
2.28.0

Re: [PATCH] tests/qtest: add one more test for the am53c974

[Mark Cave-Ayland]

On 02/04/2021 17:20, Alexander Bulekov wrote:

> Original crash:
> qemu-fuzz-i386: ../hw/scsi/esp.c:791: void esp_transfer_data(SCSIRequest *, uint32_t): Assertion `!s->do_cmd' failed.
> ==257532== ERROR: libFuzzer: deadly signal
> __assert_fail assert/assert.c:101:3
> esp_transfer_data hw/scsi/esp.c:791:5
> scsi_req_data hw/scsi/scsi-bus.c:1412:9
> scsi_disk_emulate_read_data hw/scsi/scsi-disk.c:1407:9
> scsi_req_continue hw/scsi/scsi-bus.c:1394:9
> do_busid_cmd hw/scsi/esp.c:317:9
> handle_s_without_atn hw/scsi/esp.c:393:9
> esp_reg_write hw/scsi/esp.c:1029:13
> esp_pci_io_write hw/scsi/esp-pci.c:215:9
> memory_region_write_accessor softmmu/memory.c:491:5
> access_with_adjusted_size softmmu/memory.c:552:18
> memory_region_dispatch_write softmmu/memory.c:1502:16
> flatview_write_continue softmmu/physmem.c:2746:23
> flatview_write softmmu/physmem.c:2786:14
> address_space_write softmmu/physmem.c:2878:18
> cpu_outl softmmu/ioport.c:80:5
> 
> Based-on: <20210401074933.9923-1-mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>   tests/qtest/am53c974-test.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> The patch took care of the handle_satn_stop assert. Here's a test case
> for the other assert.

Great! I've squashed the get_cmd() changes into a v4 version of the patchset.

> Pasteable:
> 
> cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
> 512M -device am53c974,id=scsi -device scsi-hd,drive=disk0 -drive \
> id=disk0,if=none,file=null-co://,format=raw -nodefaults -qtest stdio
> outl 0xcf8 0x80001010
> outl 0xcfc 0xc000
> outl 0xcf8 0x80001004
> outw 0xcfc 0x01
> outl 0xc00b 0x4100
> outb 0xc008 0x42
> outw 0xc03f 0x0300
> outl 0xc00b 0xc100
> EOF
> 
> 
> diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
> index 9c4285d0c0..506276677a 100644
> --- a/tests/qtest/am53c974-test.c
> +++ b/tests/qtest/am53c974-test.c
> @@ -9,6 +9,22 @@
>   
>   #include "libqos/libqtest.h"
>   
> +static void test_do_cmd_assert(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_outb(s, 0xc008, 0x42);
> +    qtest_outw(s, 0xc03f, 0x0300);
> +    qtest_outl(s, 0xc00b, 0xc100);
> +    qtest_quit(s);
> +}
>   
>   static void test_cmdfifo_underflow_ok(void)
>   {
> @@ -194,6 +210,8 @@ int main(int argc, char **argv)
>       g_test_init(&argc, &argv, NULL);
>   
>       if (strcmp(arch, "i386") == 0) {
> +        qtest_add_func("am53c974/test_do_cmd_asser",
> +                       test_do_cmd_assert);
>           qtest_add_func("am53c974/test_cmdfifo_underflow_ok",
>                          test_cmdfifo_underflow_ok);
>           qtest_add_func("am53c974/test_cmdfifo_underflow2_ok",

When I try this patch on top of the v4 patchset I don't get an assert() in 
esp_transfer_data here?


ATB,

Mark.

Re: [PATCH] tests/qtest: add one more test for the am53c974

[Mark Cave-Ayland]

On 03/04/2021 15:38, Mark Cave-Ayland wrote:

> On 02/04/2021 17:20, Alexander Bulekov wrote:
> 
>> Original crash:
>> qemu-fuzz-i386: ../hw/scsi/esp.c:791: void esp_transfer_data(SCSIRequest *, 
>> uint32_t): Assertion `!s->do_cmd' failed.
>> ==257532== ERROR: libFuzzer: deadly signal
>> __assert_fail assert/assert.c:101:3
>> esp_transfer_data hw/scsi/esp.c:791:5
>> scsi_req_data hw/scsi/scsi-bus.c:1412:9
>> scsi_disk_emulate_read_data hw/scsi/scsi-disk.c:1407:9
>> scsi_req_continue hw/scsi/scsi-bus.c:1394:9
>> do_busid_cmd hw/scsi/esp.c:317:9
>> handle_s_without_atn hw/scsi/esp.c:393:9
>> esp_reg_write hw/scsi/esp.c:1029:13
>> esp_pci_io_write hw/scsi/esp-pci.c:215:9
>> memory_region_write_accessor softmmu/memory.c:491:5
>> access_with_adjusted_size softmmu/memory.c:552:18
>> memory_region_dispatch_write softmmu/memory.c:1502:16
>> flatview_write_continue softmmu/physmem.c:2746:23
>> flatview_write softmmu/physmem.c:2786:14
>> address_space_write softmmu/physmem.c:2878:18
>> cpu_outl softmmu/ioport.c:80:5
>>
>> Based-on: <20210401074933.9923-1-mark.cave-ayland@ilande.co.uk>
>> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
>> ---
>>   tests/qtest/am53c974-test.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> The patch took care of the handle_satn_stop assert. Here's a test case
>> for the other assert.
> 
> Great! I've squashed the get_cmd() changes into a v4 version of the patchset.
> 
>> Pasteable:
>>
>> cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
>> 512M -device am53c974,id=scsi -device scsi-hd,drive=disk0 -drive \
>> id=disk0,if=none,file=null-co://,format=raw -nodefaults -qtest stdio
>> outl 0xcf8 0x80001010
>> outl 0xcfc 0xc000
>> outl 0xcf8 0x80001004
>> outw 0xcfc 0x01
>> outl 0xc00b 0x4100
>> outb 0xc008 0x42
>> outw 0xc03f 0x0300
>> outl 0xc00b 0xc100
>> EOF
>>
>>
>> diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
>> index 9c4285d0c0..506276677a 100644
>> --- a/tests/qtest/am53c974-test.c
>> +++ b/tests/qtest/am53c974-test.c
>> @@ -9,6 +9,22 @@
>>   #include "libqos/libqtest.h"
>> +static void test_do_cmd_assert(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_outb(s, 0xc008, 0x42);
>> +    qtest_outw(s, 0xc03f, 0x0300);
>> +    qtest_outl(s, 0xc00b, 0xc100);
>> +    qtest_quit(s);
>> +}
>>   static void test_cmdfifo_underflow_ok(void)
>>   {
>> @@ -194,6 +210,8 @@ int main(int argc, char **argv)
>>       g_test_init(&argc, &argv, NULL);
>>       if (strcmp(arch, "i386") == 0) {
>> +        qtest_add_func("am53c974/test_do_cmd_asser",
>> +                       test_do_cmd_assert);
>>           qtest_add_func("am53c974/test_cmdfifo_underflow_ok",
>>                          test_cmdfifo_underflow_ok);
>>           qtest_add_func("am53c974/test_cmdfifo_underflow2_ok",
> 
> When I try this patch on top of the v4 patchset I don't get an assert() in 
> esp_transfer_data here?

If I also revert the suggested updates for get_cmd() then I don't get the assert() 
either, so I believe this particular case is already covered by the existing tests. 
I'll drop this change for v4.


ATB,

Mark.

Re: [PATCH] tests/qtest: add one more test for the am53c974

[Mark Cave-Ayland]

On 02/04/2021 17:20, Alexander Bulekov wrote:

> Original crash:
> qemu-fuzz-i386: ../hw/scsi/esp.c:791: void esp_transfer_data(SCSIRequest *, uint32_t): Assertion `!s->do_cmd' failed.
> ==257532== ERROR: libFuzzer: deadly signal
> __assert_fail assert/assert.c:101:3
> esp_transfer_data hw/scsi/esp.c:791:5
> scsi_req_data hw/scsi/scsi-bus.c:1412:9
> scsi_disk_emulate_read_data hw/scsi/scsi-disk.c:1407:9
> scsi_req_continue hw/scsi/scsi-bus.c:1394:9
> do_busid_cmd hw/scsi/esp.c:317:9
> handle_s_without_atn hw/scsi/esp.c:393:9
> esp_reg_write hw/scsi/esp.c:1029:13
> esp_pci_io_write hw/scsi/esp-pci.c:215:9
> memory_region_write_accessor softmmu/memory.c:491:5
> access_with_adjusted_size softmmu/memory.c:552:18
> memory_region_dispatch_write softmmu/memory.c:1502:16
> flatview_write_continue softmmu/physmem.c:2746:23
> flatview_write softmmu/physmem.c:2786:14
> address_space_write softmmu/physmem.c:2878:18
> cpu_outl softmmu/ioport.c:80:5
> 
> Based-on: <20210401074933.9923-1-mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>   tests/qtest/am53c974-test.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> The patch took care of the handle_satn_stop assert. Here's a test case
> for the other assert.

Even though I can't reproduce the assert() here, looking at the code I think I can 
see how do_cmd is not being reset when a DMA command is issued. Does the following 
solve the outstanding fuzzer asserts?

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 0037197bdb..b668acef82 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;


ATB,

Mark.

Re: [PATCH] tests/qtest: add one more test for the am53c974

[Alexander Bulekov]

On 210407 1404, Mark Cave-Ayland wrote:
> 
> Even though I can't reproduce the assert() here, looking at the code I think
> I can see how do_cmd is not being reset when a DMA command is issued. Does
> the following solve the outstanding fuzzer asserts?

Hi Mark,
I guess there must have been something timing-sensitive in the
reproducer... Too bad it didn't work.

> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 0037197bdb..b668acef82 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;
> 

With this applied, I don't see either of those asserts anymore.
Thank you!
-Alex

> 
> ATB,
> 
> Mark.

Re: [PATCH] tests/qtest: add one more test for the am53c974

[Mark Cave-Ayland]

On 07/04/2021 15:49, Alexander Bulekov wrote:

> Hi Mark,
> I guess there must have been something timing-sensitive in the
> reproducer... Too bad it didn't work.

Yeah, it would have been nice to have something that could be triggered directly by a 
test but never mind.

>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 0037197bdb..b668acef82 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;
>>
> 
> With this applied, I don't see either of those asserts anymore.
> Thank you!
> -Alex

Awesome! I'll include this in v4. BTW does this now mean that the am53c974 survives a 
run through your fuzzer corpus?


ATB,

Mark.