| Message ID | 20210402162052.264952-1-alxndr@bu.edu |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest: add one more test for the am53c974 | expand |
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.
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",
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