[PATCH v3 00/17] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[PATCH v3 00/17] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[Mark Cave-Ayland]

[MCA: Since v1 I've received a few reports of FIFO assert()s being triggered and a
cmdfifo buffer overflow discovered by fuzzing the updated ESP code. The updating of
all FIFO push/pop operations to use the esp_fifo_*() functions in this series
provides protection against this, and in conjunction with new patches 9-11 is
enough to prevent all qtest reproducers that I have been sent.

For these reasons I would recommend that this series be applied for 9.0. Many thanks
to Chuhong Yuan <hslester96@gmail.com> for reporting the issues and providing qtest
reproducers.]

The ESP device has a DRQ (DMA request) signal that is used to handle flow control
during DMA transfers. At the moment the DRQ signal is explicitly raised and
lowered at various points in the ESP state machine as required, rather than
implementing the logic described in the datasheet:

"DREQ will remain true as long as either the FIFO contains at least one word (or
one byte if 8-bit mode) to send to memory during DMA read, or has room for one more
word (or byte if 8-bit mode) in the FIFO during DMA write."

This series updates the ESP device to use the same logic as described above which
also fixes a couple of outstanding GitLab issues related to recovery handling
during FIFO overflow on 68k Macs using PDMA.

Patches 1-4 update existing users of esp_fifo_pop_buf() and esp_fifo_pop() with
the internal cmdfifo to use the underlying Fifo8 device directly. The aim is
to ensure that all the esp_fifo_*() functions only operate on the ESP's hardware
FIFO.

Patches 5-6 update esp_fifo_push() and esp_fifo_pop() to take ESPState directly
as a parameter to prevent any usage that doesn't reference the ESP hardware
FIFO.

Patch 7 ensures that any usage of fifo8_push() for the ESP hardware FIFO is
updated to use esp_fifo_push() instead.

Patch 8 updates esp_fifo_pop_buf() to take ESPState directly as a parameter
to prevent any usage that doesn't reference the ESP hardware FIFO.

Patch 9 introduces the esp_fifo_push_buf() function for pushing multiple bytes
to the ESP hardware FIFO, and updates callers to use it accordingly.

Patches 10-12 incorporate additional protection to prevent FIFO assert()s and a
cmdfifo buffer overflow discovered via fuzzing.

Patch 13 is just code movement which avoids the use of a forward declaration whilst
also making it easier to locate the mapping between ESP SCSI phases and their
names.

Patches 14 introduce a new esp_update_drq() function that implements the above
DRQ logic which is called by both esp_fifo_{push,pop}_buf().

Patch 15 updates esp_fifo_push() and esp_fifo_pop() to use the new esp_update_drq()
function. At this point all reads/writes to the ESP FIFO use the esp_fifo_*()
functions and will set DRQ correctly.

Patch 16 is a small update to the logic in esp_pdma_write() to ensure that
esp_fifo_push() is always called for PDMA writes to the FIFO, thereby ensuring
that esp_update_drq() remains correct even in the case of FIFO overflow.

Finally patch 17 removes all manual calls to esp_raise_drq() and esp_lower_drq()
since the DRQ signal is now updated correctly upon each FIFO read/write access.

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

v3:
- Rebase onto master
- Add patch 1 to move the internals of esp_fifo_pop_buf() to a new
  esp_fifo8_pop_buf() function. This allows the FIFO wrap logic to still be
  used for managing cmdfifo
- Rework esp_cdb_length() into esp_cdb_ready() as suggested by Paolo in patch
  11
- Update the logic in patch 12 to use the new esp_cdb_ready() function

v2:
- Rebase onto master
- Add patches 9-12 to handle FIFO assert()s and cmdfifo overflow as reported by
  Chuhong Yuan <hslester96@gmail.com>


Mark Cave-Ayland (17):
  esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf()
    function
  esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
    do_command_phase()
  esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
    do_message_phase()
  esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
  esp.c: change esp_fifo_push() to take ESPState
  esp.c: change esp_fifo_pop() to take ESPState
  esp.c: use esp_fifo_push() instead of fifo8_push()
  esp.c: change esp_fifo_pop_buf() to take ESPState
  esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
  esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
  esp.c: rework esp_cdb_length() into esp_cdb_ready()
  esp.c: prevent cmdfifo overflow in esp_cdb_ready()
  esp.c: move esp_set_phase() and esp_get_phase() towards the beginning
    of the file
  esp.c: introduce esp_update_drq() and update esp_fifo_{push,pop}_buf()
    to use it
  esp.c: update esp_fifo_{push,pop}() to call esp_update_drq()
  esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
  esp.c: remove explicit setting of DRQ within ESP state machine

 hw/scsi/esp.c | 223 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 142 insertions(+), 81 deletions(-)

-- 
2.39.2

[PATCH v3 01/17] esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf() function

[Mark Cave-Ayland]

Update esp_fifo_pop_buf() to be a simple wrapper onto the new esp_fifo8_pop_buf()
function.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 590ff99744..1b7b118a0b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -125,7 +125,7 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo)
     return fifo8_pop(fifo);
 }
 
-static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
 {
     const uint8_t *buf;
     uint32_t n, n2;
@@ -155,6 +155,11 @@ static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
     return n;
 }
 
+static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+{
+    return esp_fifo8_pop_buf(fifo, dest, maxlen);
+}
+
 static uint32_t esp_get_tc(ESPState *s)
 {
     uint32_t dmalen;
-- 
2.39.2

[PATCH v3 02/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_command_phase()

[Mark Cave-Ayland]

The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
underlying esp_fifo8_pop_buf() function directly.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1b7b118a0b..ff51145da7 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -280,7 +280,7 @@ static void do_command_phase(ESPState *s)
     if (!cmdlen || !s->current_dev) {
         return;
     }
-    esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen);
+    esp_fifo8_pop_buf(&s->cmdfifo, buf, cmdlen);
 
     current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, s->lun);
     if (!current_lun) {
-- 
2.39.2

[PATCH v3 03/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_message_phase()

[Mark Cave-Ayland]

The aim is to restrict the esp_fifo_*() functions so that they only operate on
the hardware FIFO. When reading from cmdfifo in do_message_phase() use the
underlying esp_fifo8_pop_buf() function directly.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ff51145da7..9386704a58 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -325,7 +325,7 @@ static void do_message_phase(ESPState *s)
     /* Ignore extended messages for now */
     if (s->cmdfifo_cdb_offset) {
         int len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
-        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
+        esp_fifo8_pop_buf(&s->cmdfifo, NULL, len);
         s->cmdfifo_cdb_offset = 0;
     }
 }
-- 
2.39.2

[PATCH v3 04/17] esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9386704a58..5b169b3720 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -315,7 +315,8 @@ static void do_command_phase(ESPState *s)
 static void do_message_phase(ESPState *s)
 {
     if (s->cmdfifo_cdb_offset) {
-        uint8_t message = esp_fifo_pop(&s->cmdfifo);
+        uint8_t message = fifo8_is_empty(&s->cmdfifo) ? 0 :
+                          fifo8_pop(&s->cmdfifo);
 
         trace_esp_do_identify(message);
         s->lun = message & 7;
-- 
2.39.2

[PATCH v3 05/17] esp.c: change esp_fifo_push() to take ESPState

[Mark Cave-Ayland]

Now that all users of esp_fifo_push() operate on the main FIFO there is no need
to pass the FIFO explicitly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 5b169b3720..7e3338815b 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -106,14 +106,14 @@ void esp_request_cancelled(SCSIRequest *req)
     }
 }
 
-static void esp_fifo_push(Fifo8 *fifo, uint8_t val)
+static void esp_fifo_push(ESPState *s, uint8_t val)
 {
-    if (fifo8_num_used(fifo) == fifo->capacity) {
+    if (fifo8_num_used(&s->fifo) == s->fifo.capacity) {
         trace_esp_error_fifo_overrun();
         return;
     }
 
-    fifo8_push(fifo, val);
+    fifo8_push(&s->fifo, val);
 }
 
 static uint8_t esp_fifo_pop(Fifo8 *fifo)
@@ -229,7 +229,7 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
         return;
     }
 
-    esp_fifo_push(&s->fifo, val);
+    esp_fifo_push(s, val);
 
     dmalen--;
     esp_set_tc(s, dmalen);
@@ -1240,7 +1240,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val)
         break;
     case ESP_FIFO:
         if (!fifo8_is_full(&s->fifo)) {
-            esp_fifo_push(&s->fifo, val);
+            esp_fifo_push(s, val);
         }
         esp_do_nodma(s);
         break;
-- 
2.39.2

[PATCH v3 06/17] esp.c: change esp_fifo_pop() to take ESPState

[Mark Cave-Ayland]

Now that all users of esp_fifo_pop() operate on the main FIFO there is no need
to pass the FIFO explicitly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 7e3338815b..d474268438 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -116,13 +116,13 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
     fifo8_push(&s->fifo, val);
 }
 
-static uint8_t esp_fifo_pop(Fifo8 *fifo)
+static uint8_t esp_fifo_pop(ESPState *s)
 {
-    if (fifo8_is_empty(fifo)) {
+    if (fifo8_is_empty(&s->fifo)) {
         return 0;
     }
 
-    return fifo8_pop(fifo);
+    return fifo8_pop(&s->fifo);
 }
 
 static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
@@ -217,7 +217,7 @@ static uint8_t esp_pdma_read(ESPState *s)
 {
     uint8_t val;
 
-    val = esp_fifo_pop(&s->fifo);
+    val = esp_fifo_pop(s);
     return val;
 }
 
@@ -1184,7 +1184,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
 
     switch (saddr) {
     case ESP_FIFO:
-        s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo);
+        s->rregs[ESP_FIFO] = esp_fifo_pop(s);
         val = s->rregs[ESP_FIFO];
         break;
     case ESP_RINTR:
-- 
2.39.2

[PATCH v3 07/17] esp.c: use esp_fifo_push() instead of fifo8_push()

[Mark Cave-Ayland]

There are still a few places that use fifo8_push() instead of esp_fifo_push() in
order to push a value into the FIFO. Update those places to use esp_fifo_push()
instead.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d474268438..8d2d36d56c 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -858,7 +858,7 @@ static void esp_do_nodma(ESPState *s)
             return;
         }
         if (fifo8_is_empty(&s->fifo)) {
-            fifo8_push(&s->fifo, s->async_buf[0]);
+            esp_fifo_push(s, s->async_buf[0]);
             s->async_buf++;
             s->async_len--;
             s->ti_size--;
@@ -881,7 +881,7 @@ static void esp_do_nodma(ESPState *s)
     case STAT_ST:
         switch (s->rregs[ESP_CMD]) {
         case CMD_ICCS:
-            fifo8_push(&s->fifo, s->status);
+            esp_fifo_push(s, s->status);
             esp_set_phase(s, STAT_MI);
 
             /* Process any message in phase data */
@@ -893,7 +893,7 @@ static void esp_do_nodma(ESPState *s)
     case STAT_MI:
         switch (s->rregs[ESP_CMD]) {
         case CMD_ICCS:
-            fifo8_push(&s->fifo, 0);
+            esp_fifo_push(s, 0);
 
             /* Raise end of command interrupt */
             s->rregs[ESP_RINTR] |= INTR_FC;
-- 
2.39.2

[PATCH v3 08/17] esp.c: change esp_fifo_pop_buf() to take ESPState

[Mark Cave-Ayland]

Now that all users of esp_fifo_pop_buf() operate on the main FIFO there is no
need to pass the FIFO explicitly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 8d2d36d56c..83b621ee0f 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -155,9 +155,9 @@ static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
     return n;
 }
 
-static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
+static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
 {
-    return esp_fifo8_pop_buf(fifo, dest, maxlen);
+    return esp_fifo8_pop_buf(&s->fifo, dest, maxlen);
 }
 
 static uint32_t esp_get_tc(ESPState *s)
@@ -459,7 +459,7 @@ static void esp_do_dma(ESPState *s)
             s->dma_memory_read(s->dma_opaque, buf, len);
             esp_set_tc(s, esp_get_tc(s) - len);
         } else {
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             esp_raise_drq(s);
         }
@@ -515,7 +515,7 @@ static void esp_do_dma(ESPState *s)
             fifo8_push_all(&s->cmdfifo, buf, len);
             esp_set_tc(s, esp_get_tc(s) - len);
         } else {
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
             esp_raise_drq(s);
@@ -549,7 +549,7 @@ static void esp_do_dma(ESPState *s)
                 /* Copy FIFO data to device */
                 len = MIN(s->async_len, ESP_FIFO_SZ);
                 len = MIN(len, fifo8_num_used(&s->fifo));
-                len = esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
+                len = esp_fifo_pop_buf(s, s->async_buf, len);
                 esp_raise_drq(s);
             }
 
@@ -713,7 +713,7 @@ static void esp_nodma_ti_dataout(ESPState *s)
     }
     len = MIN(s->async_len, ESP_FIFO_SZ);
     len = MIN(len, fifo8_num_used(&s->fifo));
-    esp_fifo_pop_buf(&s->fifo, s->async_buf, len);
+    esp_fifo_pop_buf(s, s->async_buf, len);
     s->async_buf += len;
     s->async_len -= len;
     s->ti_size += len;
@@ -738,7 +738,7 @@ static void esp_do_nodma(ESPState *s)
         switch (s->rregs[ESP_CMD]) {
         case CMD_SELATN:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -757,7 +757,7 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_SELATNS:
             /* Copy one byte from FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, 1);
+            len = esp_fifo_pop_buf(s, buf, 1);
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -774,7 +774,7 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_TI:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -792,7 +792,7 @@ static void esp_do_nodma(ESPState *s)
         switch (s->rregs[ESP_CMD]) {
         case CMD_TI:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -821,7 +821,7 @@ static void esp_do_nodma(ESPState *s)
         case CMD_SEL | CMD_DMA:
         case CMD_SELATN | CMD_DMA:
             /* Copy FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
@@ -836,7 +836,7 @@ static void esp_do_nodma(ESPState *s)
         case CMD_SEL:
         case CMD_SELATN:
             /* FIFO already contain entire CDB: copy to cmdfifo and execute */
-            len = esp_fifo_pop_buf(&s->fifo, buf, fifo8_num_used(&s->fifo));
+            len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
-- 
2.39.2

[PATCH v3 09/17] esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO

[Mark Cave-Ayland]

Instead of pushing data into the FIFO directly with fifo8_push_all(), add a new
esp_fifo_push_buf() function and use it accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 83b621ee0f..1aac8f5564 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -116,6 +116,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
     fifo8_push(&s->fifo, val);
 }
 
+static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
+{
+    fifo8_push_all(&s->fifo, buf, len);
+}
+
 static uint8_t esp_fifo_pop(ESPState *s)
 {
     if (fifo8_is_empty(&s->fifo)) {
@@ -601,7 +606,7 @@ static void esp_do_dma(ESPState *s)
             } else {
                 /* Copy device data to FIFO */
                 len = MIN(len, fifo8_num_free(&s->fifo));
-                fifo8_push_all(&s->fifo, s->async_buf, len);
+                esp_fifo_push_buf(s, s->async_buf, len);
                 esp_raise_drq(s);
             }
 
@@ -650,7 +655,7 @@ static void esp_do_dma(ESPState *s)
                 if (s->dma_memory_write) {
                     s->dma_memory_write(s->dma_opaque, buf, len);
                 } else {
-                    fifo8_push_all(&s->fifo, buf, len);
+                    esp_fifo_push_buf(s, buf, len);
                 }
 
                 esp_set_tc(s, esp_get_tc(s) - len);
@@ -685,7 +690,7 @@ static void esp_do_dma(ESPState *s)
                 if (s->dma_memory_write) {
                     s->dma_memory_write(s->dma_opaque, buf, len);
                 } else {
-                    fifo8_push_all(&s->fifo, buf, len);
+                    esp_fifo_push_buf(s, buf, len);
                 }
 
                 esp_set_tc(s, esp_get_tc(s) - len);
-- 
2.39.2

[PATCH v3 10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS

[Mark Cave-Ayland]

The current logic assumes that at least 1 byte is present in the FIFO when
executing a non-DMA SELATNS command, but this may not be the case if the
guest executes an invalid ESP command sequence.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 1aac8f5564..f3aa5364cf 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s)
 
         case CMD_SELATNS:
             /* Copy one byte from FIFO into cmdfifo */
-            len = esp_fifo_pop_buf(s, buf, 1);
+            len = esp_fifo_pop_buf(s, buf,
+                                   MIN(fifo8_num_used(&s->fifo), 1));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
 
-- 
2.39.2

[PATCH v3 11/17] esp.c: rework esp_cdb_length() into esp_cdb_ready()

[Mark Cave-Ayland]

The esp_cdb_length() function is only used as part of a calculation to determine
whether the cmdfifo contains an entire SCSI CDB. Rework esp_cdb_length() into a
new esp_cdb_ready() function which both enables us to handle the case where
scsi_cdb_length() returns -1, plus simplify the logic for its callers.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f3aa5364cf..f47abc36d6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -425,20 +425,20 @@ static void write_response(ESPState *s)
     }
 }
 
-static int esp_cdb_length(ESPState *s)
+static bool esp_cdb_ready(ESPState *s)
 {
+    int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
     const uint8_t *pbuf;
-    int cmdlen, len;
+    int cdblen;
 
-    cmdlen = fifo8_num_used(&s->cmdfifo);
-    if (cmdlen < s->cmdfifo_cdb_offset) {
-        return 0;
+    if (len <= 0) {
+        return false;
     }
 
-    pbuf = fifo8_peek_buf(&s->cmdfifo, cmdlen, NULL);
-    len = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
+    pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL);
+    cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
 
-    return len;
+    return cdblen < 0 ? false : (len >= cdblen);
 }
 
 static void esp_dma_ti_check(ESPState *s)
@@ -806,10 +806,9 @@ static void esp_do_nodma(ESPState *s)
             trace_esp_handle_ti_cmd(cmdlen);
 
             /* CDB may be transferred in one or more TI commands */
-            if (esp_cdb_length(s) && esp_cdb_length(s) ==
-                fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset) {
-                    /* Command has been received */
-                    do_cmd(s);
+            if (esp_cdb_ready(s)) {
+                /* Command has been received */
+                do_cmd(s);
             } else {
                 /*
                  * If data was transferred from the FIFO then raise bus
@@ -832,10 +831,9 @@ static void esp_do_nodma(ESPState *s)
             fifo8_push_all(&s->cmdfifo, buf, len);
 
             /* Handle when DMA transfer is terminated by non-DMA FIFO write */
-            if (esp_cdb_length(s) && esp_cdb_length(s) ==
-                fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset) {
-                    /* Command has been received */
-                    do_cmd(s);
+            if (esp_cdb_ready(s)) {
+                /* Command has been received */
+                do_cmd(s);
             }
             break;
 
-- 
2.39.2

[PATCH v3 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()

[Mark Cave-Ayland]

During normal use the cmdfifo will never wrap internally and cmdfifo_cdb_offset
will always indicate the start of the SCSI CDB. However it is possible that a
malicious guest could issue an invalid ESP command sequence such that cmdfifo
wraps internally and cmdfifo_cdb_offset could point beyond the end of the FIFO
data buffer.

Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo has wrapped
internally then esp_cdb_ready() will exit rather than allow scsi_cdb_length() to
access data outside the cmdfifo data buffer.

Reported-by: Chuhong Yuan <hslester96@gmail.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/scsi/esp.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index f47abc36d6..d8db33b921 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
 {
     int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
     const uint8_t *pbuf;
+    uint32_t n;
     int cdblen;
 
     if (len <= 0) {
         return false;
     }
 
-    pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL);
+    pbuf = fifo8_peek_buf(&s->cmdfifo, len, &n);
+    if (n < len) {
+        /*
+         * In normal use the cmdfifo should never wrap, but include this check
+         * to prevent a malicious guest from reading past the end of the
+         * cmdfifo data buffer below
+         */
+        return false;
+    }
+
     cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
 
     return cdblen < 0 ? false : (len >= cdblen);
-- 
2.39.2

[PATCH v3 13/17] esp.c: move esp_set_phase() and esp_get_phase() towards the beginning of the file

[Mark Cave-Ayland]

This allows these functions to be used earlier in the file without needing a
separate forward declaration.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index d8db33b921..9e35c00927 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -79,6 +79,24 @@ static void esp_lower_drq(ESPState *s)
     }
 }
 
+static const char *esp_phase_names[8] = {
+    "DATA OUT", "DATA IN", "COMMAND", "STATUS",
+    "(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN"
+};
+
+static void esp_set_phase(ESPState *s, uint8_t phase)
+{
+    s->rregs[ESP_RSTAT] &= ~7;
+    s->rregs[ESP_RSTAT] |= phase;
+
+    trace_esp_set_phase(esp_phase_names[phase]);
+}
+
+static uint8_t esp_get_phase(ESPState *s)
+{
+    return s->rregs[ESP_RSTAT] & 7;
+}
+
 void esp_dma_enable(ESPState *s, int irq, int level)
 {
     if (level) {
@@ -200,24 +218,6 @@ static uint32_t esp_get_stc(ESPState *s)
     return dmalen;
 }
 
-static const char *esp_phase_names[8] = {
-    "DATA OUT", "DATA IN", "COMMAND", "STATUS",
-    "(reserved)", "(reserved)", "MESSAGE OUT", "MESSAGE IN"
-};
-
-static void esp_set_phase(ESPState *s, uint8_t phase)
-{
-    s->rregs[ESP_RSTAT] &= ~7;
-    s->rregs[ESP_RSTAT] |= phase;
-
-    trace_esp_set_phase(esp_phase_names[phase]);
-}
-
-static uint8_t esp_get_phase(ESPState *s)
-{
-    return s->rregs[ESP_RSTAT] & 7;
-}
-
 static uint8_t esp_pdma_read(ESPState *s)
 {
     uint8_t val;
-- 
2.39.2

[PATCH v3 14/17] esp.c: introduce esp_update_drq() and update esp_fifo_{push, pop}_buf() to use it

[Mark Cave-Ayland]

This new function sets the DRQ line correctly according to the current transfer
mode, direction and FIFO contents. Update esp_fifo_push_buf() and esp_fifo_pop_buf()
to use it so that DRQ is always set correctly when reading/writing multiple bytes
to/from the FIFO.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 9e35c00927..6fd1a12f23 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -124,6 +124,48 @@ void esp_request_cancelled(SCSIRequest *req)
     }
 }
 
+static void esp_update_drq(ESPState *s)
+{
+    bool to_device;
+
+    switch (esp_get_phase(s)) {
+    case STAT_MO:
+    case STAT_CD:
+    case STAT_DO:
+        to_device = true;
+        break;
+
+    case STAT_DI:
+    case STAT_ST:
+    case STAT_MI:
+        to_device = false;
+        break;
+
+    default:
+        return;
+    }
+
+    if (s->dma) {
+        /* DMA request so update DRQ according to transfer direction */
+        if (to_device) {
+            if (fifo8_num_free(&s->fifo) < 2) {
+                esp_lower_drq(s);
+            } else {
+                esp_raise_drq(s);
+            }
+        } else {
+            if (fifo8_num_used(&s->fifo) < 2) {
+                esp_lower_drq(s);
+            } else {
+                esp_raise_drq(s);
+            }
+        }
+    } else {
+        /* Not a DMA request */
+        esp_lower_drq(s);
+    }
+}
+
 static void esp_fifo_push(ESPState *s, uint8_t val)
 {
     if (fifo8_num_used(&s->fifo) == s->fifo.capacity) {
@@ -137,6 +179,7 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
 static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
 {
     fifo8_push_all(&s->fifo, buf, len);
+    esp_update_drq(s);
 }
 
 static uint8_t esp_fifo_pop(ESPState *s)
@@ -180,7 +223,10 @@ static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
 
 static uint32_t esp_fifo_pop_buf(ESPState *s, uint8_t *dest, int maxlen)
 {
-    return esp_fifo8_pop_buf(&s->fifo, dest, maxlen);
+    uint32_t len = esp_fifo8_pop_buf(&s->fifo, dest, maxlen);
+
+    esp_update_drq(s);
+    return len;
 }
 
 static uint32_t esp_get_tc(ESPState *s)
-- 
2.39.2

[PATCH v3 15/17] esp.c: update esp_fifo_{push, pop}() to call esp_update_drq()

[Mark Cave-Ayland]

This ensures that the DRQ line is always set correctly when reading/writing
single bytes to/from the FIFO.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 6fd1a12f23..4895181ec1 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -170,10 +170,11 @@ static void esp_fifo_push(ESPState *s, uint8_t val)
 {
     if (fifo8_num_used(&s->fifo) == s->fifo.capacity) {
         trace_esp_error_fifo_overrun();
-        return;
+    } else {
+        fifo8_push(&s->fifo, val);
     }
 
-    fifo8_push(&s->fifo, val);
+    esp_update_drq(s);
 }
 
 static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
@@ -184,11 +185,16 @@ static void esp_fifo_push_buf(ESPState *s, uint8_t *buf, int len)
 
 static uint8_t esp_fifo_pop(ESPState *s)
 {
+    uint8_t val;
+
     if (fifo8_is_empty(&s->fifo)) {
-        return 0;
+        val = 0;
+    } else {
+        val = fifo8_pop(&s->fifo);
     }
 
-    return fifo8_pop(&s->fifo);
+    esp_update_drq(s);
+    return val;
 }
 
 static uint32_t esp_fifo8_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)
-- 
2.39.2

[PATCH v3 16/17] esp.c: ensure esp_pdma_write() always calls esp_fifo_push()

[Mark Cave-Ayland]

This ensures that esp_update_drq() is called via esp_fifo_push() whenever the
host uses PDMA to transfer data to a SCSI device.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/esp.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4895181ec1..04dfd90090 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -282,14 +282,12 @@ static void esp_pdma_write(ESPState *s, uint8_t val)
 {
     uint32_t dmalen = esp_get_tc(s);
 
-    if (dmalen == 0) {
-        return;
-    }
-
     esp_fifo_push(s, val);
 
-    dmalen--;
-    esp_set_tc(s, dmalen);
+    if (dmalen && s->drq_state) {
+        dmalen--;
+        esp_set_tc(s, dmalen);
+    }
 }
 
 static int esp_select(ESPState *s)
-- 
2.39.2

[PATCH v3 17/17] esp.c: remove explicit setting of DRQ within ESP state machine

[Mark Cave-Ayland]

Now the esp_update_drq() is called for all reads/writes to the FIFO, there is
no need to manually raise and lower the DRQ signal.

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

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 04dfd90090..5d9b52632e 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -506,7 +506,6 @@ static void esp_dma_ti_check(ESPState *s)
     if (esp_get_tc(s) == 0 && fifo8_num_used(&s->fifo) < 2) {
         s->rregs[ESP_RINTR] |= INTR_BS;
         esp_raise_irq(s);
-        esp_lower_drq(s);
     }
 }
 
@@ -526,7 +525,6 @@ static void esp_do_dma(ESPState *s)
         } else {
             len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
-            esp_raise_drq(s);
         }
 
         fifo8_push_all(&s->cmdfifo, buf, len);
@@ -583,7 +581,6 @@ static void esp_do_dma(ESPState *s)
             len = esp_fifo_pop_buf(s, buf, fifo8_num_used(&s->fifo));
             len = MIN(fifo8_num_free(&s->cmdfifo), len);
             fifo8_push_all(&s->cmdfifo, buf, len);
-            esp_raise_drq(s);
         }
         trace_esp_handle_ti_cmd(cmdlen);
         s->ti_size = 0;
@@ -615,7 +612,6 @@ static void esp_do_dma(ESPState *s)
                 len = MIN(s->async_len, ESP_FIFO_SZ);
                 len = MIN(len, fifo8_num_used(&s->fifo));
                 len = esp_fifo_pop_buf(s, s->async_buf, len);
-                esp_raise_drq(s);
             }
 
             s->async_buf += len;
@@ -667,7 +663,6 @@ static void esp_do_dma(ESPState *s)
                 /* Copy device data to FIFO */
                 len = MIN(len, fifo8_num_free(&s->fifo));
                 esp_fifo_push_buf(s, s->async_buf, len);
-                esp_raise_drq(s);
             }
 
             s->async_buf += len;
@@ -733,7 +728,6 @@ static void esp_do_dma(ESPState *s)
             if (fifo8_num_used(&s->fifo) < 2) {
                 s->rregs[ESP_RINTR] |= INTR_BS;
                 esp_raise_irq(s);
-                esp_lower_drq(s);
             }
             break;
         }
@@ -1021,9 +1015,6 @@ void esp_command_complete(SCSIRequest *req, size_t resid)
     s->rregs[ESP_RINTR] |= INTR_BS;
     esp_raise_irq(s);
 
-    /* Ensure DRQ is set correctly for TC underflow or normal completion */
-    esp_dma_ti_check(s);
-
     if (s->current_req) {
         scsi_req_unref(s->current_req);
         s->current_req = NULL;
-- 
2.39.2

Re: [PATCH v3 01/17] esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf() function

[Philippe Mathieu-Daudé]

On 24/3/24 20:16, Mark Cave-Ayland wrote:
> Update esp_fifo_pop_buf() to be a simple wrapper onto the new esp_fifo8_pop_buf()
> function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)


> +static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen)

If future cleanups, maxlen can be unsigned (size_t), anyhow:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +{
> +    return esp_fifo8_pop_buf(fifo, dest, maxlen);
> +}
> +
>   static uint32_t esp_get_tc(ESPState *s)
>   {
>       uint32_t dmalen;

Re: [PATCH v3 02/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_command_phase()

[Philippe Mathieu-Daudé]

On 24/3/24 20:16, Mark Cave-Ayland wrote:
> The aim is to restrict the esp_fifo_*() functions so that they only operate on
> the hardware FIFO. When reading from cmdfifo in do_command_phase() use the
> underlying esp_fifo8_pop_buf() function directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 03/17] esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in do_message_phase()

[Philippe Mathieu-Daudé]

On 24/3/24 20:16, Mark Cave-Ayland wrote:
> The aim is to restrict the esp_fifo_*() functions so that they only operate on
> the hardware FIFO. When reading from cmdfifo in do_message_phase() use the
> underlying esp_fifo8_pop_buf() function directly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()

[Philippe Mathieu-Daudé]

On 24/3/24 20:17, Mark Cave-Ayland wrote:
> During normal use the cmdfifo will never wrap internally and cmdfifo_cdb_offset
> will always indicate the start of the SCSI CDB. However it is possible that a
> malicious guest could issue an invalid ESP command sequence such that cmdfifo
> wraps internally and cmdfifo_cdb_offset could point beyond the end of the FIFO
> data buffer.
> 
> Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo has wrapped
> internally then esp_cdb_ready() will exit rather than allow scsi_cdb_length() to
> access data outside the cmdfifo data buffer.
> 
> Reported-by: Chuhong Yuan <hslester96@gmail.com>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index f47abc36d6..d8db33b921 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
>   {
>       int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
>       const uint8_t *pbuf;
> +    uint32_t n;
>       int cdblen;
>   
>       if (len <= 0) {
>           return false;
>       }
>   
> -    pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL);
> +    pbuf = fifo8_peek_buf(&s->cmdfifo, len, &n);
> +    if (n < len) {
> +        /*
> +         * In normal use the cmdfifo should never wrap, but include this check
> +         * to prevent a malicious guest from reading past the end of the
> +         * cmdfifo data buffer below
> +         */

Can we qemu_log_mask(LOG_GUEST_ERROR) something here?

> +        return false;
> +    }
> +
>       cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
>   
>       return cdblen < 0 ? false : (len >= cdblen);

Re: [PATCH v3 10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS

[Philippe Mathieu-Daudé]

On 24/3/24 20:16, Mark Cave-Ayland wrote:
> The current logic assumes that at least 1 byte is present in the FIFO when
> executing a non-DMA SELATNS command, but this may not be the case if the
> guest executes an invalid ESP command sequence.

What is real hardware behavior here?

> 
> Reported-by: Chuhong Yuan <hslester96@gmail.com>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 1aac8f5564..f3aa5364cf 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s)
>   
>           case CMD_SELATNS:

Alternatively logging the guest abuse:

               len = fifo8_num_used(&s->fifo);
               if (len < 1) {
                   qemu_log_mask(LOG_GUEST_ERROR, ...
                   break;
               }

>               /* Copy one byte from FIFO into cmdfifo */
> -            len = esp_fifo_pop_buf(s, buf, 1);
> +            len = esp_fifo_pop_buf(s, buf,
> +                                   MIN(fifo8_num_used(&s->fifo), 1));
>               len = MIN(fifo8_num_free(&s->cmdfifo), len);
>               fifo8_push_all(&s->cmdfifo, buf, len);
>   

Re: [PATCH v3 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()

[Mark Cave-Ayland]

On 25/03/2024 10:26, Philippe Mathieu-Daudé wrote:

> On 24/3/24 20:17, Mark Cave-Ayland wrote:
>> During normal use the cmdfifo will never wrap internally and cmdfifo_cdb_offset
>> will always indicate the start of the SCSI CDB. However it is possible that a
>> malicious guest could issue an invalid ESP command sequence such that cmdfifo
>> wraps internally and cmdfifo_cdb_offset could point beyond the end of the FIFO
>> data buffer.
>>
>> Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo has wrapped
>> internally then esp_cdb_ready() will exit rather than allow scsi_cdb_length() to
>> access data outside the cmdfifo data buffer.
>>
>> Reported-by: Chuhong Yuan <hslester96@gmail.com>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index f47abc36d6..d8db33b921 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
>>   {
>>       int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
>>       const uint8_t *pbuf;
>> +    uint32_t n;
>>       int cdblen;
>>       if (len <= 0) {
>>           return false;
>>       }
>> -    pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL);
>> +    pbuf = fifo8_peek_buf(&s->cmdfifo, len, &n);
>> +    if (n < len) {
>> +        /*
>> +         * In normal use the cmdfifo should never wrap, but include this check
>> +         * to prevent a malicious guest from reading past the end of the
>> +         * cmdfifo data buffer below
>> +         */
> 
> Can we qemu_log_mask(LOG_GUEST_ERROR) something here?

I'm not sure that this makes sense here? The cmdfifo wrapping is internal artifact of 
the Fifo8 implementation rather than being directly affected by writes to the ESP 
hardware FIFO (i.e. this is not the same as the ESP hardware FIFO overflow).

>> +        return false;
>> +    }
>> +
>>       cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
>>       return cdblen < 0 ? false : (len >= cdblen);
>  


ATB,

Mark.

Re: [PATCH v3 10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS

[Mark Cave-Ayland]

On 25/03/2024 10:49, Philippe Mathieu-Daudé wrote:

> On 24/3/24 20:16, Mark Cave-Ayland wrote:
>> The current logic assumes that at least 1 byte is present in the FIFO when
>> executing a non-DMA SELATNS command, but this may not be the case if the
>> guest executes an invalid ESP command sequence.
> 
> What is real hardware behavior here?

I don't know for sure, but my guess is that if you ask to transfer a single byte from 
the FIFO to the SCSI bus and the FIFO is empty, you'll either end up with all zeros 
or a NOOP.

>> Reported-by: Chuhong Yuan <hslester96@gmail.com>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 1aac8f5564..f3aa5364cf 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s)
>>           case CMD_SELATNS:
> 
> Alternatively logging the guest abuse:
> 
>                len = fifo8_num_used(&s->fifo);
>                if (len < 1) {
>                    qemu_log_mask(LOG_GUEST_ERROR, ...
>                    break;
>                }
> 
>>               /* Copy one byte from FIFO into cmdfifo */
>> -            len = esp_fifo_pop_buf(s, buf, 1);
>> +            len = esp_fifo_pop_buf(s, buf,
>> +                                   MIN(fifo8_num_used(&s->fifo), 1));

This is similar to your previous comment in that it's an artifact of the 
implementation: when popping data using esp_fifo_pop_buf() I've always allowed the 
internal Fifo8 assert() if too much data is requested. This was a deliberate design 
choice that allowed me to catch several memory issues when working on the ESP 
emulation: it just so happened I missed a case in the last big ESP rework that was 
found by fuzzing.

It's also worth noting that it's a Fifo8 internal protective assert() that fires here 
which is different from the previous case whereby an overflow of the internal Fifo8 
data buffer actually did occur.

>>               len = MIN(fifo8_num_free(&s->cmdfifo), len);
>>               fifo8_push_all(&s->cmdfifo, buf, len);


ATB,

Mark.

Re: [PATCH v3 10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS

[Philippe Mathieu-Daudé]

On 25/3/24 13:57, Mark Cave-Ayland wrote:
> On 25/03/2024 10:49, Philippe Mathieu-Daudé wrote:
> 
>> On 24/3/24 20:16, Mark Cave-Ayland wrote:
>>> The current logic assumes that at least 1 byte is present in the FIFO 
>>> when
>>> executing a non-DMA SELATNS command, but this may not be the case if the
>>> guest executes an invalid ESP command sequence.
>>
>> What is real hardware behavior here?
> 
> I don't know for sure, but my guess is that if you ask to transfer a 
> single byte from the FIFO to the SCSI bus and the FIFO is empty, you'll 
> either end up with all zeros or a NOOP.
> 
>>> Reported-by: Chuhong Yuan <hslester96@gmail.com>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/scsi/esp.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index 1aac8f5564..f3aa5364cf 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -762,7 +762,8 @@ static void esp_do_nodma(ESPState *s)
>>>           case CMD_SELATNS:
>>
>> Alternatively logging the guest abuse:
>>
>>                len = fifo8_num_used(&s->fifo);
>>                if (len < 1) {
>>                    qemu_log_mask(LOG_GUEST_ERROR, ...
>>                    break;
>>                }
>>
>>>               /* Copy one byte from FIFO into cmdfifo */
>>> -            len = esp_fifo_pop_buf(s, buf, 1);
>>> +            len = esp_fifo_pop_buf(s, buf,
>>> +                                   MIN(fifo8_num_used(&s->fifo), 1));
> 
> This is similar to your previous comment in that it's an artifact of the 
> implementation: when popping data using esp_fifo_pop_buf() I've always 
> allowed the internal Fifo8 assert() if too much data is requested. This 
> was a deliberate design choice that allowed me to catch several memory 
> issues when working on the ESP emulation: it just so happened I missed a 
> case in the last big ESP rework that was found by fuzzing.
> 
> It's also worth noting that it's a Fifo8 internal protective assert() 
> that fires here which is different from the previous case whereby an 
> overflow of the internal Fifo8 data buffer actually did occur.

Fine then.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()

[Philippe Mathieu-Daudé]

On 25/3/24 13:41, Mark Cave-Ayland wrote:
> On 25/03/2024 10:26, Philippe Mathieu-Daudé wrote:
> 
>> On 24/3/24 20:17, Mark Cave-Ayland wrote:
>>> During normal use the cmdfifo will never wrap internally and 
>>> cmdfifo_cdb_offset
>>> will always indicate the start of the SCSI CDB. However it is 
>>> possible that a
>>> malicious guest could issue an invalid ESP command sequence such that 
>>> cmdfifo
>>> wraps internally and cmdfifo_cdb_offset could point beyond the end of 
>>> the FIFO
>>> data buffer.
>>>
>>> Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo 
>>> has wrapped
>>> internally then esp_cdb_ready() will exit rather than allow 
>>> scsi_cdb_length() to
>>> access data outside the cmdfifo data buffer.
>>>
>>> Reported-by: Chuhong Yuan <hslester96@gmail.com>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/scsi/esp.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index f47abc36d6..d8db33b921 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
>>>   {
>>>       int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
>>>       const uint8_t *pbuf;
>>> +    uint32_t n;
>>>       int cdblen;
>>>       if (len <= 0) {
>>>           return false;
>>>       }
>>> -    pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL);
>>> +    pbuf = fifo8_peek_buf(&s->cmdfifo, len, &n);
>>> +    if (n < len) {
>>> +        /*
>>> +         * In normal use the cmdfifo should never wrap, but include 
>>> this check
>>> +         * to prevent a malicious guest from reading past the end of 
>>> the
>>> +         * cmdfifo data buffer below
>>> +         */
>>
>> Can we qemu_log_mask(LOG_GUEST_ERROR) something here?
> 
> I'm not sure that this makes sense here? The cmdfifo wrapping is 
> internal artifact of the Fifo8 implementation rather than being directly 
> affected by writes to the ESP hardware FIFO (i.e. this is not the same 
> as the ESP hardware FIFO overflow).

Still this check "prevent[s from] a malicious guest", but I don't
mind, better be safe here.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 11/17] esp.c: rework esp_cdb_length() into esp_cdb_ready()

[Philippe Mathieu-Daudé]

On 24/3/24 20:17, Mark Cave-Ayland wrote:
> The esp_cdb_length() function is only used as part of a calculation to determine
> whether the cmdfifo contains an entire SCSI CDB. Rework esp_cdb_length() into a
> new esp_cdb_ready() function which both enables us to handle the case where
> scsi_cdb_length() returns -1, plus simplify the logic for its callers.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/scsi/esp.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 00/17] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[Paolo Bonzini]

On Sun, Mar 24, 2024 at 8:17 PM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
> Patches 1-4 update existing users of esp_fifo_pop_buf() and esp_fifo_pop() with
> the internal cmdfifo to use the underlying Fifo8 device directly. The aim is
> to ensure that all the esp_fifo_*() functions only operate on the ESP's hardware
> FIFO.
>
> Patches 5-6 update esp_fifo_push() and esp_fifo_pop() to take ESPState directly
> as a parameter to prevent any usage that doesn't reference the ESP hardware
> FIFO.
>
> Patch 7 ensures that any usage of fifo8_push() for the ESP hardware FIFO is
> updated to use esp_fifo_push() instead.
>
> Patch 8 updates esp_fifo_pop_buf() to take ESPState directly as a parameter
> to prevent any usage that doesn't reference the ESP hardware FIFO.
>
> Patch 9 introduces the esp_fifo_push_buf() function for pushing multiple bytes
> to the ESP hardware FIFO, and updates callers to use it accordingly.
>
> Patches 10-12 incorporate additional protection to prevent FIFO assert()s and a
> cmdfifo buffer overflow discovered via fuzzing.
>
> Patch 13 is just code movement which avoids the use of a forward declaration whilst
> also making it easier to locate the mapping between ESP SCSI phases and their
> names.
>
> Patches 14 introduce a new esp_update_drq() function that implements the above
> DRQ logic which is called by both esp_fifo_{push,pop}_buf().
>
> Patch 15 updates esp_fifo_push() and esp_fifo_pop() to use the new esp_update_drq()
> function. At this point all reads/writes to the ESP FIFO use the esp_fifo_*()
> functions and will set DRQ correctly.
>
> Patch 16 is a small update to the logic in esp_pdma_write() to ensure that
> esp_fifo_push() is always called for PDMA writes to the FIFO, thereby ensuring
> that esp_update_drq() remains correct even in the case of FIFO overflow.
>
> Finally patch 17 removes all manual calls to esp_raise_drq() and esp_lower_drq()
> since the DRQ signal is now updated correctly upon each FIFO read/write access.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> v3:
> - Rebase onto master
> - Add patch 1 to move the internals of esp_fifo_pop_buf() to a new
>   esp_fifo8_pop_buf() function. This allows the FIFO wrap logic to still be
>   used for managing cmdfifo
> - Rework esp_cdb_length() into esp_cdb_ready() as suggested by Paolo in patch
>   11
> - Update the logic in patch 12 to use the new esp_cdb_ready() function
>
> v2:
> - Rebase onto master
> - Add patches 9-12 to handle FIFO assert()s and cmdfifo overflow as reported by
>   Chuhong Yuan <hslester96@gmail.com>
>
>
> Mark Cave-Ayland (17):
>   esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf()
>     function
>   esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>     do_command_phase()
>   esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>     do_message_phase()
>   esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
>   esp.c: change esp_fifo_push() to take ESPState
>   esp.c: change esp_fifo_pop() to take ESPState
>   esp.c: use esp_fifo_push() instead of fifo8_push()
>   esp.c: change esp_fifo_pop_buf() to take ESPState
>   esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
>   esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
>   esp.c: rework esp_cdb_length() into esp_cdb_ready()
>   esp.c: prevent cmdfifo overflow in esp_cdb_ready()
>   esp.c: move esp_set_phase() and esp_get_phase() towards the beginning
>     of the file
>   esp.c: introduce esp_update_drq() and update esp_fifo_{push,pop}_buf()
>     to use it
>   esp.c: update esp_fifo_{push,pop}() to call esp_update_drq()
>   esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
>   esp.c: remove explicit setting of DRQ within ESP state machine
>
>  hw/scsi/esp.c | 223 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 142 insertions(+), 81 deletions(-)
>
> --
> 2.39.2
>

Re: [PATCH v3 00/17] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[Philippe Mathieu-Daudé]

Hi Mark,

On 24/3/24 20:16, Mark Cave-Ayland wrote:

> Mark Cave-Ayland (17):
>    esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf()
>      function
>    esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>      do_command_phase()
>    esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>      do_message_phase()
>    esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
>    esp.c: change esp_fifo_push() to take ESPState
>    esp.c: change esp_fifo_pop() to take ESPState
>    esp.c: use esp_fifo_push() instead of fifo8_push()
>    esp.c: change esp_fifo_pop_buf() to take ESPState
>    esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
>    esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
>    esp.c: rework esp_cdb_length() into esp_cdb_ready()
>    esp.c: prevent cmdfifo overflow in esp_cdb_ready()
>    esp.c: move esp_set_phase() and esp_get_phase() towards the beginning
>      of the file
>    esp.c: introduce esp_update_drq() and update esp_fifo_{push,pop}_buf()
>      to use it
>    esp.c: update esp_fifo_{push,pop}() to call esp_update_drq()
>    esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
>    esp.c: remove explicit setting of DRQ within ESP state machine

I have to prepare another PR for rc3, do you want me to take
this series?

Regards,

Phil.

Re: [PATCH v3 00/17] [for-9.0] esp: avoid explicit setting of DRQ within ESP state machine

[Mark Cave-Ayland]

On 04/04/2024 11:28, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 24/3/24 20:16, Mark Cave-Ayland wrote:
> 
>> Mark Cave-Ayland (17):
>>    esp.c: move esp_fifo_pop_buf() internals to new esp_fifo8_pop_buf()
>>      function
>>    esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>>      do_command_phase()
>>    esp.c: replace esp_fifo_pop_buf() with esp_fifo8_pop_buf() in
>>      do_message_phase()
>>    esp.c: replace cmdfifo use of esp_fifo_pop() in do_message_phase()
>>    esp.c: change esp_fifo_push() to take ESPState
>>    esp.c: change esp_fifo_pop() to take ESPState
>>    esp.c: use esp_fifo_push() instead of fifo8_push()
>>    esp.c: change esp_fifo_pop_buf() to take ESPState
>>    esp.c: introduce esp_fifo_push_buf() function for pushing to the FIFO
>>    esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS
>>    esp.c: rework esp_cdb_length() into esp_cdb_ready()
>>    esp.c: prevent cmdfifo overflow in esp_cdb_ready()
>>    esp.c: move esp_set_phase() and esp_get_phase() towards the beginning
>>      of the file
>>    esp.c: introduce esp_update_drq() and update esp_fifo_{push,pop}_buf()
>>      to use it
>>    esp.c: update esp_fifo_{push,pop}() to call esp_update_drq()
>>    esp.c: ensure esp_pdma_write() always calls esp_fifo_push()
>>    esp.c: remove explicit setting of DRQ within ESP state machine
> 
> I have to prepare another PR for rc3, do you want me to take
> this series?

Hi Phil,

Thanks for the offer, but amazingly I have a little bit of time this afternoon(!), so 
I'll try and send a PR later today.


ATB,

Mark.