diff mbox series

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

Message ID 20240324191707.623175-13-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:17 p.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé March 25, 2024, 10:26 a.m. UTC | #1
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?

> +        return false;
> +    }
> +
>       cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
>   
>       return cdblen < 0 ? false : (len >= cdblen);
Mark Cave-Ayland March 25, 2024, 12:41 p.m. UTC | #2
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).

>> +        return false;
>> +    }
>> +
>>       cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
>>       return cdblen < 0 ? false : (len >= cdblen);
>  


ATB,

Mark.
Philippe Mathieu-Daudé April 2, 2024, 11:36 a.m. UTC | #3
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>
diff mbox series

Patch

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
+         */
+        return false;
+    }
+
     cdblen = scsi_cdb_length((uint8_t *)&pbuf[s->cmdfifo_cdb_offset]);
 
     return cdblen < 0 ? false : (len >= cdblen);