[PATCH v4 for-6.0 00/12] esp: fix asserts/segfaults discovered by fuzzer

Mark Cave-Ayland posted 12 patches 6 days, 11 hours ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210407195801.685-1-mark.cave-ayland@ilande.co.uk
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, Laurent Vivier <lvivier@redhat.com>, Thomas Huth <thuth@redhat.com>
MAINTAINERS                 |   1 +
hw/scsi/esp.c               | 119 +++++++++++---------
tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
tests/qtest/meson.build     |   1 +
4 files changed, 285 insertions(+), 52 deletions(-)
create mode 100644 tests/qtest/am53c974-test.c

[PATCH v4 for-6.0 00/12] esp: fix asserts/segfaults discovered by fuzzer

Posted by Mark Cave-Ayland 6 days, 11 hours ago
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>

v4:
- Rebase onto master
- Add R-B tags from Phil
- Fix accidental line space removal in patch 3 discovered by Phil
- Change spelling of sanitiser -> sanitizer in patch 5 as suggested by Phil
- Fix up cmdfifo length checks in patch 8
- Add T-B tags from Alex
- Add patch 11 to handle additional assert discovered by Alex during fuzzing

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

v2:
- Add Alexander's R-B tag for patch 2 and Phil's R-B for patch 3
- Add patch 4 for additional testcase provided in Alexander's patch 1 comment
- Move current_req NULL checks forward in DMA functions (fixes ASAN bug reported
  at https://bugs.launchpad.net/qemu/+bug/1909247/comments/6) in patch 3
- Add qtest for am53c974 containing a basic set of regression tests using the
  automatic test cases generated by the fuzzer as requested by Paolo


Mark Cave-Ayland (12):
  esp: always check current_req is not NULL before use in DMA callbacks
  esp: rework write_response() to avoid using the FIFO for DMA
    transactions
  esp: consolidate esp_cmdfifo_push() into esp_fifo_push()
  esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop()
  esp: introduce esp_fifo_pop_buf() and use it instead of
    fifo8_pop_buf()
  esp: ensure cmdfifo is not empty and current_dev is non-NULL
  esp: don't underflow cmdfifo in do_cmd()
  esp: don't overflow cmdfifo in get_cmd()
  esp: don't overflow cmdfifo if TC is larger than the cmdfifo size
  esp: don't reset async_len directly in esp_select() if cancelling
    request
  esp: ensure that do_cmd is set to zero before submitting an ESP select
    command
  tests/qtest: add tests for am53c974 device

 MAINTAINERS                 |   1 +
 hw/scsi/esp.c               | 119 +++++++++++---------
 tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
 tests/qtest/meson.build     |   1 +
 4 files changed, 285 insertions(+), 52 deletions(-)
 create mode 100644 tests/qtest/am53c974-test.c

-- 
2.20.1


Re: [PATCH v4 for-6.0 00/12] esp: fix asserts/segfaults discovered by fuzzer

Posted by Mark Cave-Ayland 4 days, 19 hours ago
On 07/04/2021 20:57, Mark Cave-Ayland wrote:

(added Peter to CC)

> 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>
> 
> v4:
> - Rebase onto master
> - Add R-B tags from Phil
> - Fix accidental line space removal in patch 3 discovered by Phil
> - Change spelling of sanitiser -> sanitizer in patch 5 as suggested by Phil
> - Fix up cmdfifo length checks in patch 8
> - Add T-B tags from Alex
> - Add patch 11 to handle additional assert discovered by Alex during fuzzing
> 
> 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
> 
> v2:
> - Add Alexander's R-B tag for patch 2 and Phil's R-B for patch 3
> - Add patch 4 for additional testcase provided in Alexander's patch 1 comment
> - Move current_req NULL checks forward in DMA functions (fixes ASAN bug reported
>    at https://bugs.launchpad.net/qemu/+bug/1909247/comments/6) in patch 3
> - Add qtest for am53c974 containing a basic set of regression tests using the
>    automatic test cases generated by the fuzzer as requested by Paolo
> 
> 
> Mark Cave-Ayland (12):
>    esp: always check current_req is not NULL before use in DMA callbacks
>    esp: rework write_response() to avoid using the FIFO for DMA
>      transactions
>    esp: consolidate esp_cmdfifo_push() into esp_fifo_push()
>    esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop()
>    esp: introduce esp_fifo_pop_buf() and use it instead of
>      fifo8_pop_buf()
>    esp: ensure cmdfifo is not empty and current_dev is non-NULL
>    esp: don't underflow cmdfifo in do_cmd()
>    esp: don't overflow cmdfifo in get_cmd()
>    esp: don't overflow cmdfifo if TC is larger than the cmdfifo size
>    esp: don't reset async_len directly in esp_select() if cancelling
>      request
>    esp: ensure that do_cmd is set to zero before submitting an ESP select
>      command
>    tests/qtest: add tests for am53c974 device
> 
>   MAINTAINERS                 |   1 +
>   hw/scsi/esp.c               | 119 +++++++++++---------
>   tests/qtest/am53c974-test.c | 216 ++++++++++++++++++++++++++++++++++++
>   tests/qtest/meson.build     |   1 +
>   4 files changed, 285 insertions(+), 52 deletions(-)
>   create mode 100644 tests/qtest/am53c974-test.c

Hi Paolo,

Is this still a candidate for 6.0 as you suggested in your response to v1? There is 
also the related ESP fix for the SPARC acceptance test failure which I think is also 
appropriate for 6.0.

If so, who would be able to review/merge both these ESP patches? Given that we're 
already at -rc2 I'm aware that it's getting quite close to the 6.0 release.


ATB,

Mark.