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

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
> 

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.

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

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.

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.

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.

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.

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.