Re: [PATCH v3 12/17] esp.c: prevent cmdfifo overflow in esp_cdb_ready()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 25/3/24 13:41, Mark Cave-Ayland wrote:
> On 25/03/2024 10:26, Philippe Mathieu-Daudé wrote:
> 
>> On 24/3/24 20:17, Mark Cave-Ayland wrote:
>>> During normal use the cmdfifo will never wrap internally and 
>>> cmdfifo_cdb_offset
>>> will always indicate the start of the SCSI CDB. However it is 
>>> possible that a
>>> malicious guest could issue an invalid ESP command sequence such that 
>>> cmdfifo
>>> wraps internally and cmdfifo_cdb_offset could point beyond the end of 
>>> the FIFO
>>> data buffer.
>>>
>>> Add an extra check to fifo8_peek_buf() to ensure that if the cmdfifo 
>>> has wrapped
>>> internally then esp_cdb_ready() will exit rather than allow 
>>> scsi_cdb_length() to
>>> access data outside the cmdfifo data buffer.
>>>
>>> Reported-by: Chuhong Yuan <hslester96@gmail.com>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/scsi/esp.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>>> index f47abc36d6..d8db33b921 100644
>>> --- a/hw/scsi/esp.c
>>> +++ b/hw/scsi/esp.c
>>> @@ -429,13 +429,23 @@ static bool esp_cdb_ready(ESPState *s)
>>>   {
>>>       int len = fifo8_num_used(&s->cmdfifo) - s->cmdfifo_cdb_offset;
>>>       const uint8_t *pbuf;
>>> +    uint32_t n;
>>>       int cdblen;
>>>       if (len <= 0) {
>>>           return false;
>>>       }
>>> -    pbuf = fifo8_peek_buf(&s->cmdfifo, len, NULL);
>>> +    pbuf = fifo8_peek_buf(&s->cmdfifo, len, &n);
>>> +    if (n < len) {
>>> +        /*
>>> +         * In normal use the cmdfifo should never wrap, but include 
>>> this check
>>> +         * to prevent a malicious guest from reading past the end of 
>>> the
>>> +         * cmdfifo data buffer below
>>> +         */
>>
>> Can we qemu_log_mask(LOG_GUEST_ERROR) something here?
> 
> I'm not sure that this makes sense here? The cmdfifo wrapping is 
> internal artifact of the Fifo8 implementation rather than being directly 
> affected by writes to the ESP hardware FIFO (i.e. this is not the same 
> as the ESP hardware FIFO overflow).

Still this check "prevent[s from] a malicious guest", but I don't
mind, better be safe here.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>