[PATCH for-6.2 0/2] esp: add fix for reset before transfer

[PATCH for-6.2 0/2] esp: add fix for reset before transfer

[Mark Cave-Ayland]

This is the fix for Gitlab issue #724 discovered by fuzzing which I think is
worth including in 6.2 for 2 reasons: firstly the fix is to zero out
an extra field during chip reset which normally only occurs during driver
initialisation and durring IO timeouts, and secondly the bug causes a stale
SCSI data buffer pointer dereference rather than triggering a FIFO assert.

The first patch contains the very simple fix, whilst the second patch adds a
qtest based upon the original Gitlab issue.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (2):
  esp: ensure that async_len is reset to 0 during esp_hard_reset()
  qtest/am53c974-test: add test for reset before transfer

 hw/scsi/esp.c               |  1 +
 tests/qtest/am53c974-test.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

-- 
2.20.1

[PATCH for-6.2 1/2] esp: ensure that async_len is reset to 0 during esp_hard_reset()

[Mark Cave-Ayland]

If a reset command is sent after data has been transferred into the SCSI buffer
ensure that async_len is reset to 0. Otherwise a subsequent TI command assumes
the SCSI buffer contains data to be transferred to the device causing it to
dereference the stale async_buf pointer.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/724
---
 hw/scsi/esp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 84f935b549..58d0edbd56 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -894,6 +894,7 @@ void esp_hard_reset(ESPState *s)
     memset(s->wregs, 0, ESP_REGS);
     s->tchi_written = 0;
     s->ti_size = 0;
+    s->async_len = 0;
     fifo8_reset(&s->fifo);
     fifo8_reset(&s->cmdfifo);
     s->dma = 0;
-- 
2.20.1

[PATCH for-6.2 2/2] qtest/am53c974-test: add test for reset before transfer

[Mark Cave-Ayland]

Based upon the qtest reproducer posted to Gitlab issue #724 at
https://gitlab.com/qemu-project/qemu/-/issues/724.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 tests/qtest/am53c974-test.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
index 9b1e4211bd..d214a912b3 100644
--- a/tests/qtest/am53c974-test.c
+++ b/tests/qtest/am53c974-test.c
@@ -223,6 +223,34 @@ static void test_inflight_cancel_ok(void)
     qtest_quit(s);
 }
 
+static void test_reset_before_transfer_ok(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, 0xc007, 0x2500);
+    qtest_outl(s, 0xc00a, 0x410000);
+    qtest_outl(s, 0xc00a, 0x410000);
+    qtest_outw(s, 0xc00b, 0x0200);
+    qtest_outw(s, 0xc040, 0x03);
+    qtest_outw(s, 0xc009, 0x00);
+    qtest_outw(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc009, 0x00);
+    qtest_outw(s, 0xc00b, 0x00);
+    qtest_outw(s, 0xc009, 0x00);
+    qtest_outw(s, 0xc003, 0x1000);
+    qtest_outw(s, 0xc00b, 0x1000);
+    qtest_outl(s, 0xc00b, 0x9000);
+    qtest_outw(s, 0xc00b, 0x1000);
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -248,6 +276,8 @@ int main(int argc, char **argv)
                        test_cancelled_request_ok);
         qtest_add_func("am53c974/test_inflight_cancel_ok",
                        test_inflight_cancel_ok);
+        qtest_add_func("am53c974/test_reset_before_transfer_ok",
+                       test_reset_before_transfer_ok);
     }
 
     return g_test_run();
-- 
2.20.1

Re: [PATCH for-6.2 2/2] qtest/am53c974-test: add test for reset before transfer

[Thomas Huth]

On 18/11/2021 11.03, Mark Cave-Ayland wrote:
> Based upon the qtest reproducer posted to Gitlab issue #724 at
> https://gitlab.com/qemu-project/qemu/-/issues/724.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   tests/qtest/am53c974-test.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c
> index 9b1e4211bd..d214a912b3 100644
> --- a/tests/qtest/am53c974-test.c
> +++ b/tests/qtest/am53c974-test.c
> @@ -223,6 +223,34 @@ static void test_inflight_cancel_ok(void)
>       qtest_quit(s);
>   }
>   
> +static void test_reset_before_transfer_ok(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, 0xc007, 0x2500);
> +    qtest_outl(s, 0xc00a, 0x410000);
> +    qtest_outl(s, 0xc00a, 0x410000);
> +    qtest_outw(s, 0xc00b, 0x0200);
> +    qtest_outw(s, 0xc040, 0x03);
> +    qtest_outw(s, 0xc009, 0x00);
> +    qtest_outw(s, 0xc00b, 0x00);
> +    qtest_outw(s, 0xc009, 0x00);
> +    qtest_outw(s, 0xc00b, 0x00);
> +    qtest_outw(s, 0xc009, 0x00);
> +    qtest_outw(s, 0xc003, 0x1000);
> +    qtest_outw(s, 0xc00b, 0x1000);
> +    qtest_outl(s, 0xc00b, 0x9000);
> +    qtest_outw(s, 0xc00b, 0x1000);
> +    qtest_quit(s);
> +}
> +
>   int main(int argc, char **argv)
>   {
>       const char *arch = qtest_get_arch();
> @@ -248,6 +276,8 @@ int main(int argc, char **argv)
>                          test_cancelled_request_ok);
>           qtest_add_func("am53c974/test_inflight_cancel_ok",
>                          test_inflight_cancel_ok);
> +        qtest_add_func("am53c974/test_reset_before_transfer_ok",
> +                       test_reset_before_transfer_ok);
>       }
>   
>       return g_test_run();
> 

Acked-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH for-6.2 1/2] esp: ensure that async_len is reset to 0 during esp_hard_reset()

[Philippe Mathieu-Daudé]

On 11/18/21 11:03, Mark Cave-Ayland wrote:
> If a reset command is sent after data has been transferred into the SCSI buffer
> ensure that async_len is reset to 0. Otherwise a subsequent TI command assumes
> the SCSI buffer contains data to be transferred to the device causing it to
> dereference the stale async_buf pointer.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/724
> ---
>  hw/scsi/esp.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH for-6.2 0/2] esp: add fix for reset before transfer

[Paolo Bonzini]

On 11/18/21 11:03, Mark Cave-Ayland wrote:
> This is the fix for Gitlab issue #724 discovered by fuzzing which I think is
> worth including in 6.2 for 2 reasons: firstly the fix is to zero out
> an extra field during chip reset which normally only occurs during driver
> initialisation and durring IO timeouts, and secondly the bug causes a stale
> SCSI data buffer pointer dereference rather than triggering a FIFO assert.
> 
> The first patch contains the very simple fix, whilst the second patch adds a
> qtest based upon the original Gitlab issue.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> Mark Cave-Ayland (2):
>    esp: ensure that async_len is reset to 0 during esp_hard_reset()
>    qtest/am53c974-test: add test for reset before transfer
> 
>   hw/scsi/esp.c               |  1 +
>   tests/qtest/am53c974-test.c | 30 ++++++++++++++++++++++++++++++
>   2 files changed, 31 insertions(+)
> 

Queued, thanks.

Paolo