[PATCH 0/4] esp: fix asserts/segfaults discovered by fuzzer

[PATCH 0/4] 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>


Mark Cave-Ayland (4):
  esp: don't underflow cmdfifo if no message out/command data is present
  esp: don't overflow cmdfifo if TC is larger than the cmdfifo size
  esp: ensure cmdfifo is not empty and current_dev is non-NULL
  esp: always check current_req is not NULL before use in DMA callbacks

 hw/scsi/esp.c | 56 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 20 deletions(-)

-- 
2.20.1

[PATCH 1/4] esp: don't underflow cmdfifo if no message out/command data is present

[Mark Cave-Ayland]

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

Re: [PATCH 1/4] esp: don't underflow cmdfifo if no message out/command data is present

[Alexander Bulekov]

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>

Re: [PATCH 1/4] esp: don't underflow cmdfifo if no message out/command data is present

[Alexander Bulekov]

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

Re: [PATCH 1/4] esp: don't underflow cmdfifo if no message out/command data is present

[Alexander Bulekov]

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

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5d3f1ccbc8..bbcbfa4a91 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -579,6 +579,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

Re: [PATCH 2/4] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size

[Philippe Mathieu-Daudé]

On 3/17/21 12:30 AM, Mark Cave-Ayland wrote:
> 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>
> ---
>  hw/scsi/esp.c | 1 +
>  1 file changed, 1 insertion(+)

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

Re: [PATCH 2/4] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size

[Alexander Bulekov]

On 210316 2330, Mark Cave-Ayland wrote:
> 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>
> ---
>  hw/scsi/esp.c | 1 +
>  1 file changed, 1 insertion(+)

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

[PATCH 3/4] 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 bbcbfa4a91..ae362c9dfb 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -286,6 +286,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;
+    }
     buf = (uint8_t *)fifo8_pop_buf(&s->cmdfifo, cmdlen, &n);
 
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun);
-- 
2.20.1

Re: [PATCH 3/4] esp: ensure cmdfifo is not empty and current_dev is non-NULL

[Philippe Mathieu-Daudé]

On 3/17/21 12:30 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>

Re: [PATCH 3/4] esp: ensure cmdfifo is not empty and current_dev is non-NULL

[Alexander Bulekov]

Hi Mark,

On 210316 2330, 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

^ Can't reproduce this one anymore

> Buglink: https://bugs.launchpad.net/qemu/+bug/1909247

However, this still seems to cause a UAF:
https://bugs.launchpad.net/qemu/+bug/1909247/comments/6

-Alex

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/scsi/esp.c | 3 +++
>  1 file changed, 3 insertions(+)

[PATCH 4/4] 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 | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ae362c9dfb..7216903ce0 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -524,7 +524,9 @@ static void do_dma_pdma_cb(ESPState *s)
         }
 
         if (s->async_len == 0) {
-            scsi_req_continue(s->current_req);
+            if (s->current_req) {
+                scsi_req_continue(s->current_req);
+            }
             return;
         }
 
@@ -674,14 +676,16 @@ static void esp_do_dma(ESPState *s)
         s->ti_size -= len;
     }
     if (s->async_len == 0) {
-        scsi_req_continue(s->current_req);
-        /*
-         * If there is still data to be read from the device then
-         * complete the DMA operation immediately.  Otherwise defer
-         * until the scsi layer has completed.
-         */
-        if (to_device || esp_get_tc(s) != 0 || s->ti_size == 0) {
-            return;
+        if (s->current_req) {
+            scsi_req_continue(s->current_req);
+            /*
+             * If there is still data to be read from the device then
+             * complete the DMA operation immediately.  Otherwise defer
+             * until the scsi layer has completed.
+             */
+            if (to_device || esp_get_tc(s) != 0 || s->ti_size == 0) {
+                return;
+            }
         }
     }
 
@@ -744,10 +748,12 @@ static void esp_do_nodma(ESPState *s)
     }
 
     if (s->async_len == 0) {
-        scsi_req_continue(s->current_req);
+        if (s->current_req) {
+            scsi_req_continue(s->current_req);
 
-        if (to_device || s->ti_size == 0) {
-            return;
+            if (to_device || s->ti_size == 0) {
+                return;
+            }
         }
     }
 
-- 
2.20.1

Re: [PATCH 0/4] esp: fix asserts/segfaults discovered by fuzzer

[Philippe Mathieu-Daudé]

+Laurent for 1 & 4.

On 3/17/21 12:30 AM, 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>
> 
> 
> Mark Cave-Ayland (4):
>   esp: don't underflow cmdfifo if no message out/command data is present
>   esp: don't overflow cmdfifo if TC is larger than the cmdfifo size
>   esp: ensure cmdfifo is not empty and current_dev is non-NULL
>   esp: always check current_req is not NULL before use in DMA callbacks
> 
>  hw/scsi/esp.c | 56 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 20 deletions(-)
> 

Re: [PATCH 0/4] esp: fix asserts/segfaults discovered by fuzzer

[Paolo Bonzini]

On 17/03/21 00:30, 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>

They are certainly something that we can fix for 6.0.  However, please 
include the testcases even if they are ugly, they can be cleaned up 
later or (if never cleaned up) they still count as regression tests.

Paolo