If a guest sends a TI (Transfer Information) command without previously sending
any message out/command phase data then cmdfifo will underflow triggering an
assert reading the IDENTIFY byte.

Buglink: https://bugs.launchpad.net/qemu/+bug/1919035
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 507ab363bc..5d3f1ccbc8 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -318,18 +318,24 @@ 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;
     uint32_t n;
 
-    s->cmdfifo_cdb_offset--;
+    if (fifo8_num_used(&s->cmdfifo)) {
+        busid = fifo8_pop(&s->cmdfifo);
 
-    /* Ignore extended messages for now */
-    if (s->cmdfifo_cdb_offset) {
-        fifo8_pop_buf(&s->cmdfifo, s->cmdfifo_cdb_offset, &n);
-        s->cmdfifo_cdb_offset = 0;
-    }
+        if (s->cmdfifo_cdb_offset) {
+            s->cmdfifo_cdb_offset--;
+
+            /* Ignore extended messages for now */
+            if (s->cmdfifo_cdb_offset) {
+                fifo8_pop_buf(&s->cmdfifo, s->cmdfifo_cdb_offset, &n);
+                s->cmdfifo_cdb_offset = 0;
+            }
+        }
 
-    do_busid_cmd(s, busid);
+        do_busid_cmd(s, busid);
+    }
 }
 
 static void satn_pdma_cb(ESPState *s)
-- 
2.20.1

On 210316 2330, Mark Cave-Ayland wrote:
> If a guest sends a TI (Transfer Information) command without previously sending
> any message out/command phase data then cmdfifo will underflow triggering an
> assert reading the IDENTIFY byte.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1919035
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)

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

On 210317 1114, Alexander Bulekov wrote:
> On 210316 2330, Mark Cave-Ayland wrote:
> > If a guest sends a TI (Transfer Information) command without previously sending
> > any message out/command phase data then cmdfifo will underflow triggering an
> > assert reading the IDENTIFY byte.
> > 
> > Buglink: https://bugs.launchpad.net/qemu/+bug/1919035
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >  hw/scsi/esp.c | 22 ++++++++++++++--------
> >  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> Tested-by: Alexander Bulekov <alxndr@bu.edu>

Oops please disregard this. It was meant for PATCH 2/4

On 210316 2330, Mark Cave-Ayland wrote:
> If a guest sends a TI (Transfer Information) command without previously sending
> any message out/command phase data then cmdfifo will underflow triggering an
> assert reading the IDENTIFY byte.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1919035
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Hi Mark,
The original reproducer no longer asserts, but I ran through the fuzz
corpus, and there is another one, that still seems to trigger the same
bug:

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 0xc008 0x0a
outl 0xc009 0x41000000
outl 0xc009 0x41000000
outl 0xc00b 0x1000
EOF

C Code testcase below.
Thanks
-Alex

/*
 * Autogenerated Fuzzer Test Case
 *
 * Copyright (c) 2021 <name of author>
 *
 * 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"

/*
 * 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 0xc008 0x0a
 * outl 0xc009 0x41000000
 * outl 0xc009 0x41000000
 * outl 0xc00b 0x1000
 * EOF
 */
static void test_fuzz(void)
{
    QTestState *s = qtest_init(
        "-display none , -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, 0xc008, 0x0a);
    qtest_outl(s, 0xc009, 0x41000000);
    qtest_outl(s, 0xc009, 0x41000000);
    qtest_outl(s, 0xc00b, 0x1000);
    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("fuzz/test_fuzz", test_fuzz);
    }

    return g_test_run();
}