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 <hsleste...@gmail.com>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@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é <phi...@linaro.org>
