| Message ID | 20210317230223.24854-1-mark.cave-ayland@ilande.co.uk |
|---|---|
| Headers | show |
| Series | esp: fix asserts/segfaults discovered by fuzzer | expand |
On 18/03/21 00:02, 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> > > 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 (6): > esp: don't underflow cmdfifo if no message out/command data is present > esp: don't overflow cmdfifo if TC is larger than the cmdfifo size > esp: ensure cmdfifo is not empty and current_dev is non-NULL > esp: don't underflow fifo when writing to the device > esp: always check current_req is not NULL before use in DMA callbacks > tests/qtest: add tests for am53c974 device > > hw/scsi/esp.c | 73 +++++++++++++-------- > tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ > tests/qtest/meson.build | 1 + > 3 files changed, 171 insertions(+), 25 deletions(-) > create mode 100644 tests/qtest/am53c974-test.c > Queued, thanks. Paolo
On 18/03/2021 18:13, Paolo Bonzini wrote: > On 18/03/21 00:02, 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> >> >> 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 (6): >> esp: don't underflow cmdfifo if no message out/command data is present >> esp: don't overflow cmdfifo if TC is larger than the cmdfifo size >> esp: ensure cmdfifo is not empty and current_dev is non-NULL >> esp: don't underflow fifo when writing to the device >> esp: always check current_req is not NULL before use in DMA callbacks >> tests/qtest: add tests for am53c974 device >> >> hw/scsi/esp.c | 73 +++++++++++++-------- >> tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ >> tests/qtest/meson.build | 1 + >> 3 files changed, 171 insertions(+), 25 deletions(-) >> create mode 100644 tests/qtest/am53c974-test.c >> > > Queued, thanks. > > Paolo Hi Paolo, I had a quick look at Alex's updated test cases and most of them are based on an incorrect assumption I made around the behaviour of fifo8_pop_buf(). Can you drop these for now, and I will submit a v3 shortly once I've given it a full run through my test images? ATB, Mark.
On 30/03/21 09:34, Mark Cave-Ayland wrote: > Hi Paolo, > > I had a quick look at Alex's updated test cases and most of them are > based on an incorrect assumption I made around the behaviour of > fifo8_pop_buf(). Can you drop these for now, and I will submit a v3 > shortly once I've given it a full run through my test images? Hi, I also had some failures of the tests on CI, which is why I hadn't incorporated these changes yet. Thanks for the advance warning, I'll wait for your v3. Paolo
On 30/03/2021 10:59, Paolo Bonzini wrote: > Hi, > > I also had some failures of the tests on CI, which is why I hadn't incorporated these > changes yet. Thanks for the advance warning, I'll wait for your v3. > > Paolo Hi Paolo, I've just posted the latest v3 which passes all my local boot tests and the extra test cases posted to LP. There is one failure on Gitlab CI but that is for the clang-user build for tcg-tests-s390x-linux-user which is an existing issue. Apologies it took a bit longer than expected: my laptop isn't the fastest in the world and booting everything will full debug and ASAN across several targets is tremendously slow :/ Also it seems there is something wrong with the qtest dependencies: for my current build of ~2900 files, the final commit to add the qtest for am53c974 which adds the test to test/qtest/meson.build causes ~2100 of those files to be rebuilt :( ATB, Mark.
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> 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 (6): esp: don't underflow cmdfifo if no message out/command data is present esp: don't overflow cmdfifo if TC is larger than the cmdfifo size esp: ensure cmdfifo is not empty and current_dev is non-NULL esp: don't underflow fifo when writing to the device esp: always check current_req is not NULL before use in DMA callbacks tests/qtest: add tests for am53c974 device hw/scsi/esp.c | 73 +++++++++++++-------- tests/qtest/am53c974-test.c | 122 ++++++++++++++++++++++++++++++++++++ tests/qtest/meson.build | 1 + 3 files changed, 171 insertions(+), 25 deletions(-) create mode 100644 tests/qtest/am53c974-test.c