diff mbox series

[v3,10/17] esp.c: don't assert() if FIFO empty when executing non-DMA SELATNS

Message ID 20240324191707.623175-11-mark.cave-ayland@ilande.co.uk (mailing list archive)
State New, archived
Headers show
Series esp: avoid explicit setting of DRQ within ESP state machine | expand

Commit Message

Mark Cave-Ayland March 24, 2024, 7:16 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé March 25, 2024, 10:49 a.m. UTC | #1
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);
>
Mark Cave-Ayland March 25, 2024, 12:57 p.m. UTC | #2
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.
Philippe Mathieu-Daudé April 2, 2024, 11:34 a.m. UTC | #3
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 mbox series

Patch

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);