[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Mark Cave-Ayland
Subject: Re: [PATCH v3 00/11] esp: fix asserts/segfaults discovered by fuzzer
Date: Fri, 2 Apr 2021 08:35:21 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0

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:

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>

- 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 
   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

Tested-by: Alexander Bulekov <alxndr@bu.edu>

Thank you

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]