| Message ID | 20240324191707.623175-11-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | esp: avoid explicit setting of DRQ within ESP state machine | expand |
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); >
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.
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>
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);
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(-)