[PATCH 0/3] esp/scsi: minor fixes

[PATCH 0/3] esp/scsi: minor fixes

[Mark Cave-Ayland]

Here are a couple of ESP/SCSI fixes related to issue #1810, along with another
patch that fixes a problem with the DMA enable signal I discovered whilst
testing some future ESP changes.

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


Mark Cave-Ayland (3):
  esp: use correct type for esp_dma_enable() in sysbus_esp_gpio_demux()
  esp: restrict non-DMA transfer length to that of available data
  scsi-disk: ensure that FORMAT UNIT commands are terminated

 hw/scsi/esp.c       | 5 +++--
 hw/scsi/scsi-disk.c | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/3] esp: use correct type for esp_dma_enable() in sysbus_esp_gpio_demux()

[Mark Cave-Ayland]

The call to esp_dma_enable() was being made with the SYSBUS_ESP type instead of
the ESP type. This meant that when GPIO 1 was being used to trigger a DMA
request from an external DMA controller, the setting of ESPState's dma_enabled
field would clobber unknown memory whilst the dma_cb callback pointer would
typically return NULL so the DMA request would never start.

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 e52188d022..4218a6a960 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -1395,7 +1395,7 @@ static void sysbus_esp_gpio_demux(void *opaque, int irq, int level)
         parent_esp_reset(s, irq, level);
         break;
     case 1:
-        esp_dma_enable(opaque, irq, level);
+        esp_dma_enable(s, irq, level);
         break;
     }
 }
-- 
2.39.2

Re: [PATCH 1/3] esp: use correct type for esp_dma_enable() in sysbus_esp_gpio_demux()

[Philippe Mathieu-Daudé]

On 13/9/23 22:44, Mark Cave-Ayland wrote:
> The call to esp_dma_enable() was being made with the SYSBUS_ESP type instead of
> the ESP type. This meant that when GPIO 1 was being used to trigger a DMA
> request from an external DMA controller, the setting of ESPState's dma_enabled
> field would clobber unknown memory whilst the dma_cb callback pointer would
> typically return NULL so the DMA request would never start.
> 

Cc: qemu-stable@nongnu.org
Fixes: a391fdbc7f ("esp: split esp code into generic chip emulation and 
sysbus layer")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> 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 e52188d022..4218a6a960 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1395,7 +1395,7 @@ static void sysbus_esp_gpio_demux(void *opaque, int irq, int level)
>           parent_esp_reset(s, irq, level);
>           break;
>       case 1:
> -        esp_dma_enable(opaque, irq, level);
> +        esp_dma_enable(s, irq, level);
>           break;
>       }
>   }

Re: [PATCH 1/3] esp: use correct type for esp_dma_enable() in sysbus_esp_gpio_demux()

[Thomas Huth]

On 13/09/2023 22.44, Mark Cave-Ayland wrote:
> The call to esp_dma_enable() was being made with the SYSBUS_ESP type instead of
> the ESP type. This meant that when GPIO 1 was being used to trigger a DMA
> request from an external DMA controller, the setting of ESPState's dma_enabled
> field would clobber unknown memory whilst the dma_cb callback pointer would
> typically return NULL so the DMA request would never start.
> 
> 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 e52188d022..4218a6a960 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -1395,7 +1395,7 @@ static void sysbus_esp_gpio_demux(void *opaque, int irq, int level)
>           parent_esp_reset(s, irq, level);
>           break;
>       case 1:
> -        esp_dma_enable(opaque, irq, level);
> +        esp_dma_enable(s, irq, level);
>           break;
>       }
>   }

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

[PATCH 2/3] esp: restrict non-DMA transfer length to that of available data

[Mark Cave-Ayland]

In the case where a SCSI layer transfer is incorrectly terminated, it is
possible for a TI command to cause a SCSI buffer overflow due to the
expected transfer data length being less than the available data in the
FIFO. When this occurs the unsigned async_len variable underflows and
becomes a large offset which writes past the end of the allocated SCSI
buffer.

Restrict the non-DMA transfer length to be the smallest of the expected
transfer length and the available FIFO data to ensure that it is no longer
possible for the SCSI buffer overflow to occur.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1810
---
 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 4218a6a960..9b11d8c573 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -759,7 +759,8 @@ static void esp_do_nodma(ESPState *s)
     }
 
     if (to_device) {
-        len = MIN(fifo8_num_used(&s->fifo), ESP_FIFO_SZ);
+        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);
         s->async_buf += len;
         s->async_len -= len;
-- 
2.39.2

Re: [PATCH 2/3] esp: restrict non-DMA transfer length to that of available data

[Thomas Huth]

On 13/09/2023 22.44, Mark Cave-Ayland wrote:
> In the case where a SCSI layer transfer is incorrectly terminated, it is
> possible for a TI command to cause a SCSI buffer overflow due to the
> expected transfer data length being less than the available data in the
> FIFO. When this occurs the unsigned async_len variable underflows and
> becomes a large offset which writes past the end of the allocated SCSI
> buffer.
> 
> Restrict the non-DMA transfer length to be the smallest of the expected
> transfer length and the available FIFO data to ensure that it is no longer
> possible for the SCSI buffer overflow to occur.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1810
> ---
>   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 4218a6a960..9b11d8c573 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -759,7 +759,8 @@ static void esp_do_nodma(ESPState *s)
>       }
>   
>       if (to_device) {
> -        len = MIN(fifo8_num_used(&s->fifo), ESP_FIFO_SZ);
> +        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);
>           s->async_buf += len;
>           s->async_len -= len;

Thanks for taking care of this!

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

[PATCH 3/3] scsi-disk: ensure that FORMAT UNIT commands are terminated

[Mark Cave-Ayland]

Otherwise when a FORMAT UNIT command is issued, the SCSI layer can become
confused because it can find itself in the situation where it thinks there
is still data to be transferred which can cause the next emulated SCSI
command to fail.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Fixes: 6ab71761 ("scsi-disk: add FORMAT UNIT command")
---
 hw/scsi/scsi-disk.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index e0d79c7966..4484ee8271 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -1958,6 +1958,10 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
         scsi_disk_emulate_write_same(r, r->iov.iov_base);
         break;
 
+    case FORMAT_UNIT:
+        scsi_req_complete(&r->req, GOOD);
+        break;
+
     default:
         abort();
     }
-- 
2.39.2

Re: [PATCH 3/3] scsi-disk: ensure that FORMAT UNIT commands are terminated

[Thomas Huth]

On 13/09/2023 22.44, Mark Cave-Ayland wrote:
> Otherwise when a FORMAT UNIT command is issued, the SCSI layer can become
> confused because it can find itself in the situation where it thinks there
> is still data to be transferred which can cause the next emulated SCSI
> command to fail.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 6ab71761 ("scsi-disk: add FORMAT UNIT command")
> ---
>   hw/scsi/scsi-disk.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
> index e0d79c7966..4484ee8271 100644
> --- a/hw/scsi/scsi-disk.c
> +++ b/hw/scsi/scsi-disk.c
> @@ -1958,6 +1958,10 @@ static void scsi_disk_emulate_write_data(SCSIRequest *req)
>           scsi_disk_emulate_write_same(r, r->iov.iov_base);
>           break;
>   
> +    case FORMAT_UNIT:
> +        scsi_req_complete(&r->req, GOOD);
> +        break;
> +
>       default:
>           abort();
>       }

Thanks! I just double-checked that this fixes the crash that can be 
triggered with the reproducer from 
https://gitlab.com/qemu-project/qemu/-/issues/1810 :

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