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

[Mark Cave-Ayland]

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.