Re: [PATCH v3 08/11] esp: don't overflow cmdfifo in get_cmd()

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 01/04/2021 09:19, Philippe Mathieu-Daudé wrote:

> On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
>> If the guest tries to read a CDB using DMA and cmdfifo is not empty then it is
>> possible to overflow cmdfifo.
>>
>> Since this can only occur by issuing deliberately incorrect instruction
>> sequences, ensure that the maximum length of the CDB transferred to cmdfifo is
>> limited to the available free space within cmdfifo.
>>
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1909247
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/scsi/esp.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 7f49522e1d..c547c60395 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -243,6 +243,7 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
>>           }
>>           if (s->dma_memory_read) {
>>               s->dma_memory_read(s->dma_opaque, buf, dmalen);
>> +            dmalen = MIN(fifo8_num_free(&s->fifo), dmalen);
> 
> Ditto, GUEST_ERRORS?

Possibly? But then there are several other places where this could also happen. The 
ESP uses the FIFO as a buffer for the SCSI bus in DMA mode, and so at the start of a 
DMA transfer the FIFO can contain leftover junk. This is why all the guest OSs I've 
seen send an explicit "Flush FIFO" command before each command to ensure that any 
junk is removed from the FIFO before sending the message out/CDB.

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>>               fifo8_push_all(&s->cmdfifo, buf, dmalen);
>>           } else {
>>               if (esp_select(s) < 0) {
>>

ATB,

Mark.