Re: [PATCH v3 00/11] esp: fix asserts/segfaults discovered by fuzzer

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

On 01/04/2021 18:00, Alexander Bulekov wrote:

> On 210401 0849, Mark Cave-Ayland wrote:
>> Recently there have been a number of issues raised on Launchpad as a result of
>> fuzzing the am53c974 (ESP) device. I spent some time over the past couple of
>> days checking to see if anything had improved since my last patchset: from
>> what I can tell the issues are still present, but the cmdfifo related failures
>> now assert rather than corrupting memory.
>>
>> This patchset applied to master passes my local tests using the qtest fuzz test
>> cases added by Alexander for the following Launchpad bugs:
>>
>>    https://bugs.launchpad.net/qemu/+bug/1919035
>>    https://bugs.launchpad.net/qemu/+bug/1919036
>>    https://bugs.launchpad.net/qemu/+bug/1910723
>>    https://bugs.launchpad.net/qemu/+bug/1909247
>>    
>> I'm posting this now just before soft freeze since I see that some of the issues
>> have recently been allocated CVEs and so it could be argued that even though
>> they have existed for some time, it is worth fixing them for 6.0.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>
>> v3:
>> - Rebase onto master
>> - Rearrange patch ordering (move patch 5 to the front) to help reduce cross-talk
>>    between the regression tests
>> - Introduce patch 2 to remove unnecessary FIFO usage
>> - Introduce patches 3-4 to consolidate esp_fifo_pop()/esp_fifo_push() wrapper
>>    functions to avoid having to introduce 2 variants of esp_fifo_pop_buf()
>> - Introduce esp_fifo_pop_buf() in patch 5 to prevent callers from overflowing
>>    the array used to model the FIFO
>> - Introduce patch 10 to clarify cancellation logic should all occur in the .cancel
>>    SCSI callback rather than at the site of the caller
>> - Add extra qtests in patch 11 to cover addition test cases provided on LP
>>
> 
> Hi Mark,
> I applied this and ran through the whole fuzzer corpus, and all I'm
> seeing are just a few assertion failures:
> handle_satn_stop -> get_cmd -> util/fifo8.c:43:5 and
> hw/scsi/esp.c:790:5
> 
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> 
> Thank you
> -Alex

Thanks for the testing! I've just realised that there is an error in the get_cmd() 
MIN() check (it should be cmdfifo, not fifo) and also the limit is missing from the 
non-DMA path.

Does the following patch on v3 fix the outstanding get_cmd() asserts?

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index ca062a0400..3b9037e4f4 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -243,7 +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);
+            dmalen = MIN(fifo8_num_free(&s->cmdfifo), dmalen);
              fifo8_push_all(&s->cmdfifo, buf, dmalen);
          } else {
              if (esp_select(s) < 0) {
@@ -263,6 +263,7 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen)
          if (n >= 3) {
              buf[0] = buf[2] >> 5;
          }
+        n = MIN(fifo8_num_free(&s->cmdfifo), n);
          fifo8_push_all(&s->cmdfifo, buf, n);
      }
      trace_esp_get_cmd(dmalen, target);

Given that there is going to be a v4 now, if you are able to provide a handful of 
test cases for hw/scsi/esp.c:790:5 as a diff on v3 like you did before then I can 
take a quick look.


ATB,

Mark.