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>