diff mbox series

[v3,07/11] esp: don't underflow cmdfifo in do_cmd()

Message ID 20210401074933.9923-8-mark.cave-ayland@ilande.co.uk
State New
Headers show
Series esp: fix asserts/segfaults discovered by fuzzer | expand

Commit Message

Mark Cave-Ayland April 1, 2021, 7:49 a.m. UTC
If the guest tries to execute a CDB when cmdfifo is not empty before the start
of the message out phase then clearing the message out phase data will cause
cmdfifo to underflow due to cmdfifo_cdb_offset being larger than the amount of
data within.

Since this can only occur by issuing deliberately incorrect instruction
sequences, ensure that the maximum length of esp_fifo_pop_buf() is limited to
the size of the data 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 | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé April 1, 2021, 8:19 a.m. UTC | #1
On 4/1/21 9:49 AM, Mark Cave-Ayland wrote:
> If the guest tries to execute a CDB when cmdfifo is not empty before the start
> of the message out phase then clearing the message out phase data will cause
> cmdfifo to underflow due to cmdfifo_cdb_offset being larger than the amount of
> data within.
> 
> Since this can only occur by issuing deliberately incorrect instruction
> sequences, ensure that the maximum length of esp_fifo_pop_buf() is limited to
> the size of the data 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 | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 4decbbfc29..7f49522e1d 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -319,13 +319,15 @@ static void do_busid_cmd(ESPState *s, uint8_t busid)
>  
>  static void do_cmd(ESPState *s)
>  {
> -    uint8_t busid = fifo8_pop(&s->cmdfifo);
> +    uint8_t busid = esp_fifo_pop(&s->cmdfifo);
> +    int len;
>  
>      s->cmdfifo_cdb_offset--;
>  
>      /* Ignore extended messages for now */
>      if (s->cmdfifo_cdb_offset) {
> -        esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset);
> +        len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));

Do we want to log(GUEST_ERRORS) this?

> +        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
>          s->cmdfifo_cdb_offset = 0;
>      }
>  
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Mark Cave-Ayland April 1, 2021, 8:51 a.m. UTC | #2
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 execute a CDB when cmdfifo is not empty before the start
>> of the message out phase then clearing the message out phase data will cause
>> cmdfifo to underflow due to cmdfifo_cdb_offset being larger than the amount of
>> data within.
>>
>> Since this can only occur by issuing deliberately incorrect instruction
>> sequences, ensure that the maximum length of esp_fifo_pop_buf() is limited to
>> the size of the data 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 | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
>> index 4decbbfc29..7f49522e1d 100644
>> --- a/hw/scsi/esp.c
>> +++ b/hw/scsi/esp.c
>> @@ -319,13 +319,15 @@ static void do_busid_cmd(ESPState *s, uint8_t busid)
>>   
>>   static void do_cmd(ESPState *s)
>>   {
>> -    uint8_t busid = fifo8_pop(&s->cmdfifo);
>> +    uint8_t busid = esp_fifo_pop(&s->cmdfifo);
>> +    int len;
>>   
>>       s->cmdfifo_cdb_offset--;
>>   
>>       /* Ignore extended messages for now */
>>       if (s->cmdfifo_cdb_offset) {
>> -        esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset);
>> +        len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
> 
> Do we want to log(GUEST_ERRORS) this?

Not for this case, since a guest OS may legitimately try a SDTR negotiation using 
extended messages which the ESP implementation currently ignores.

>> +        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
>>           s->cmdfifo_cdb_offset = 0;
>>       }
>>   
>>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>


ATB,

Mark.
diff mbox series

Patch

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4decbbfc29..7f49522e1d 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -319,13 +319,15 @@  static void do_busid_cmd(ESPState *s, uint8_t busid)
 
 static void do_cmd(ESPState *s)
 {
-    uint8_t busid = fifo8_pop(&s->cmdfifo);
+    uint8_t busid = esp_fifo_pop(&s->cmdfifo);
+    int len;
 
     s->cmdfifo_cdb_offset--;
 
     /* Ignore extended messages for now */
     if (s->cmdfifo_cdb_offset) {
-        esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset);
+        len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo));
+        esp_fifo_pop_buf(&s->cmdfifo, NULL, len);
         s->cmdfifo_cdb_offset = 0;
     }