tests/qtest: add one more test for the am53c974

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

Posted by Alexander Bulekov 4 years, 10 months ago

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

Posted by Mark Cave-Ayland 4 years, 10 months ago

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

Posted by Mark Cave-Ayland 4 years, 10 months ago

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

Posted by Mark Cave-Ayland 4 years, 10 months ago

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

Posted by Alexander Bulekov 4 years, 10 months ago

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

Posted by Mark Cave-Ayland 4 years, 10 months ago

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.