| Message ID | 20210401074933.9923-9-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | esp: fix asserts/segfaults discovered by fuzzer | expand |
On 4/1/21 9:49 AM, Mark Cave-Ayland wrote: > If the guest tries to read a CDB using DMA and cmdfifo is not empty then it is > possible to overflow cmdfifo. > > Since this can only occur by issuing deliberately incorrect instruction > sequences, ensure that the maximum length of the CDB transferred to cmdfifo is > limited to the available free space within cmdfifo. > > Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > hw/scsi/esp.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 7f49522e1d..c547c60395 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -243,6 +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); Ditto, GUEST_ERRORS? Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > fifo8_push_all(&s->cmdfifo, buf, dmalen); > } else { > if (esp_select(s) < 0) { >
On 01/04/2021 09:19, Philippe Mathieu-Daudé wrote: > On 4/1/21 9:49 AM, Mark Cave-Ayland wrote: >> If the guest tries to read a CDB using DMA and cmdfifo is not empty then it is >> possible to overflow cmdfifo. >> >> Since this can only occur by issuing deliberately incorrect instruction >> sequences, ensure that the maximum length of the CDB transferred to cmdfifo is >> limited to the available free space within cmdfifo. >> >> Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> hw/scsi/esp.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c >> index 7f49522e1d..c547c60395 100644 >> --- a/hw/scsi/esp.c >> +++ b/hw/scsi/esp.c >> @@ -243,6 +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); > > Ditto, GUEST_ERRORS? Possibly? But then there are several other places where this could also happen. The ESP uses the FIFO as a buffer for the SCSI bus in DMA mode, and so at the start of a DMA transfer the FIFO can contain leftover junk. This is why all the guest OSs I've seen send an explicit "Flush FIFO" command before each command to ensure that any junk is removed from the FIFO before sending the message out/CDB. > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> fifo8_push_all(&s->cmdfifo, buf, dmalen); >> } else { >> if (esp_select(s) < 0) { >> ATB, Mark.
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 7f49522e1d..c547c60395 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -243,6 +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); fifo8_push_all(&s->cmdfifo, buf, dmalen); } else { if (esp_select(s) < 0) {
If the guest tries to read a CDB using DMA and cmdfifo is not empty then it is possible to overflow cmdfifo. Since this can only occur by issuing deliberately incorrect instruction sequences, ensure that the maximum length of the CDB transferred to cmdfifo is limited to the available free space within cmdfifo. Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 1 + 1 file changed, 1 insertion(+)