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.