From ff4a1daba6adc8811efb5046483feb3af6bd8d83 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 13:48:42 +0100 Subject: [PATCH 01/13] esp: fix setting of ESPState mig_version_id when launching QEMU with -S option If QEMU is launched with the -S option then the ESPState mig_version_id property is left unset due to the ordering of the VMState fields in the VMStateDescription for sysbusespscsi and pciespscsi. If the VM is migrated and restored in this stopped state, the version tests in the vmstate_esp VMStateDescription and esp_post_load() become confused causing the migration to fail. Fix the ordering problem by moving the setting of mig_version_id to a common esp_pre_save() function which is invoked first by both sysbusespscsi and pciespscsi rather than at the point where ESPState is itself serialised into the migration stream. Buglink: https://bugs.launchpad.net/qemu/+bug/1922611 Fixes: 0bd005be78 ("esp: add vmstate_esp version to embedded ESPState") Signed-off-by: Mark Cave-Ayland Reviewed-by: Thomas Huth Message-Id: <20210407124842.32695-1-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp-pci.c | 1 + hw/scsi/esp.c | 7 ++++--- include/hw/scsi/esp.h | 1 + 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c index c3d3dab05e..9db10b1a48 100644 --- a/hw/scsi/esp-pci.c +++ b/hw/scsi/esp-pci.c @@ -332,6 +332,7 @@ static const VMStateDescription vmstate_esp_pci_scsi = { .name = "pciespscsi", .version_id = 2, .minimum_version_id = 1, + .pre_save = esp_pre_save, .fields = (VMStateField[]) { VMSTATE_PCI_DEVICE(parent_obj, PCIESPState), VMSTATE_BUFFER_UNSAFE(dma_regs, PCIESPState, 0, 8 * sizeof(uint32_t)), diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 507ab363bc..d87e1a63db 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -1076,9 +1076,10 @@ static bool esp_is_version_5(void *opaque, int version_id) return version_id == 5; } -static int esp_pre_save(void *opaque) +int esp_pre_save(void *opaque) { - ESPState *s = ESP(opaque); + ESPState *s = ESP(object_resolve_path_component( + OBJECT(opaque), "esp")); s->mig_version_id = vmstate_esp.version_id; return 0; @@ -1114,7 +1115,6 @@ const VMStateDescription vmstate_esp = { .name = "esp", .version_id = 5, .minimum_version_id = 3, - .pre_save = esp_pre_save, .post_load = esp_post_load, .fields = (VMStateField[]) { VMSTATE_BUFFER(rregs, ESPState), @@ -1304,6 +1304,7 @@ static const VMStateDescription vmstate_sysbus_esp_scsi = { .name = "sysbusespscsi", .version_id = 2, .minimum_version_id = 1, + .pre_save = esp_pre_save, .fields = (VMStateField[]) { VMSTATE_UINT8_V(esp.mig_version_id, SysBusESPState, 2), VMSTATE_STRUCT(esp, SysBusESPState, 0, vmstate_esp, ESPState), diff --git a/include/hw/scsi/esp.h b/include/hw/scsi/esp.h index 95088490aa..aada3680b7 100644 --- a/include/hw/scsi/esp.h +++ b/include/hw/scsi/esp.h @@ -157,5 +157,6 @@ void esp_hard_reset(ESPState *s); uint64_t esp_reg_read(ESPState *s, uint32_t saddr); void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val); extern const VMStateDescription vmstate_esp; +int esp_pre_save(void *opaque); #endif From 0db895361b8a82e1114372ff9f4857abea605701 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:50 +0100 Subject: [PATCH 02/13] esp: always check current_req is not NULL before use in DMA callbacks After issuing a SCSI command the SCSI layer can call the SCSIBusInfo .cancel callback which resets both current_req and current_dev to NULL. If any data is left in the transfer buffer (async_len != 0) then the next TI (Transfer Information) command will attempt to reference the NULL pointer causing a segfault. Buglink: https://bugs.launchpad.net/qemu/+bug/1910723 Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-2-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index d87e1a63db..a79196f3f3 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -496,6 +496,10 @@ static void do_dma_pdma_cb(ESPState *s) return; } + if (!s->current_req) { + return; + } + if (to_device) { /* Copy FIFO data to device */ len = MIN(s->async_len, ESP_FIFO_SZ); @@ -527,11 +531,9 @@ static void do_dma_pdma_cb(ESPState *s) return; } else { if (s->async_len == 0) { - if (s->current_req) { - /* Defer until the scsi layer has completed */ - scsi_req_continue(s->current_req); - s->data_in_ready = false; - } + /* Defer until the scsi layer has completed */ + scsi_req_continue(s->current_req); + s->data_in_ready = false; return; } @@ -604,6 +606,9 @@ static void esp_do_dma(ESPState *s) } return; } + if (!s->current_req) { + return; + } if (s->async_len == 0) { /* Defer until data is available. */ return; @@ -713,6 +718,10 @@ static void esp_do_nodma(ESPState *s) return; } + if (!s->current_req) { + return; + } + if (s->async_len == 0) { /* Defer until data is available. */ return; From e392255766071c8cac480da3a9ae4f94e56d7cbc Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:51 +0100 Subject: [PATCH 03/13] esp: rework write_response() to avoid using the FIFO for DMA transactions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code for write_response() has always used the FIFO to store the data for the status/message in phases, even for DMA transactions. Switch to using a separate buffer that can be used directly for DMA transactions and restrict the FIFO use to the non-DMA case. Signed-off-by: Mark Cave-Ayland Tested-by: Alexander Bulekov Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210407195801.685-3-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index a79196f3f3..2584ec6fb1 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -445,18 +445,16 @@ static void write_response_pdma_cb(ESPState *s) static void write_response(ESPState *s) { - uint32_t n; + uint8_t buf[2]; trace_esp_write_response(s->status); - fifo8_reset(&s->fifo); - esp_fifo_push(s, s->status); - esp_fifo_push(s, 0); + buf[0] = s->status; + buf[1] = 0; if (s->dma) { if (s->dma_memory_write) { - s->dma_memory_write(s->dma_opaque, - (uint8_t *)fifo8_pop_buf(&s->fifo, 2, &n), 2); + s->dma_memory_write(s->dma_opaque, buf, 2); s->rregs[ESP_RSTAT] = STAT_TC | STAT_ST; s->rregs[ESP_RINTR] |= INTR_BS | INTR_FC; s->rregs[ESP_RSEQ] = SEQ_CD; @@ -466,7 +464,8 @@ static void write_response(ESPState *s) return; } } else { - s->ti_size = 2; + fifo8_reset(&s->fifo); + fifo8_push_all(&s->fifo, buf, 2); s->rregs[ESP_RFLAGS] = 2; } esp_raise_irq(s); From e5455b8c1c6170c788f3c0fd577cc3be53539a99 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:52 +0100 Subject: [PATCH 04/13] esp: consolidate esp_cmdfifo_push() into esp_fifo_push() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each FIFO currently has its own push functions with the only difference being the capacity check. The original reason for this was that the fifo8 implementation doesn't have a formal API for retrieving the FIFO capacity, however there are multiple examples within QEMU where the capacity field is accessed directly. Change esp_fifo_push() to access the FIFO capacity directly and then consolidate esp_cmdfifo_push() into esp_fifo_push(). Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-4-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 2584ec6fb1..b3471e0333 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -98,16 +98,15 @@ void esp_request_cancelled(SCSIRequest *req) } } -static void esp_fifo_push(ESPState *s, uint8_t val) +static void esp_fifo_push(Fifo8 *fifo, uint8_t val) { - if (fifo8_num_used(&s->fifo) == ESP_FIFO_SZ) { + if (fifo8_num_used(fifo) == fifo->capacity) { trace_esp_error_fifo_overrun(); return; } - fifo8_push(&s->fifo, val); + fifo8_push(fifo, val); } - static uint8_t esp_fifo_pop(ESPState *s) { if (fifo8_is_empty(&s->fifo)) { @@ -117,16 +116,6 @@ static uint8_t esp_fifo_pop(ESPState *s) return fifo8_pop(&s->fifo); } -static void esp_cmdfifo_push(ESPState *s, uint8_t val) -{ - if (fifo8_num_used(&s->cmdfifo) == ESP_CMDFIFO_SZ) { - trace_esp_error_fifo_overrun(); - return; - } - - fifo8_push(&s->cmdfifo, val); -} - static uint8_t esp_cmdfifo_pop(ESPState *s) { if (fifo8_is_empty(&s->cmdfifo)) { @@ -187,9 +176,9 @@ static void esp_pdma_write(ESPState *s, uint8_t val) } if (s->do_cmd) { - esp_cmdfifo_push(s, val); + esp_fifo_push(&s->cmdfifo, val); } else { - esp_fifo_push(s, val); + esp_fifo_push(&s->fifo, val); } dmalen--; @@ -645,7 +634,7 @@ static void esp_do_dma(ESPState *s) */ if (len < esp_get_tc(s) && esp_get_tc(s) <= ESP_FIFO_SZ) { while (fifo8_num_used(&s->fifo) < ESP_FIFO_SZ) { - esp_fifo_push(s, 0); + esp_fifo_push(&s->fifo, 0); len++; } } @@ -947,9 +936,9 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t val) break; case ESP_FIFO: if (s->do_cmd) { - esp_cmdfifo_push(s, val); + esp_fifo_push(&s->cmdfifo, val); } else { - esp_fifo_push(s, val); + esp_fifo_push(&s->fifo, val); } /* Non-DMA transfers raise an interrupt after every byte */ From c5fef9112b15c4b5494791cdf8bbb40bc1938dd3 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:53 +0100 Subject: [PATCH 05/13] esp: consolidate esp_cmdfifo_pop() into esp_fifo_pop() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each FIFO currently has its own pop functions with the only difference being the capacity check. The original reason for this was that the fifo8 implementation doesn't have a formal API for retrieving the FIFO capacity, however there are multiple examples within QEMU where the capacity field is accessed directly. Change esp_fifo_pop() to access the FIFO capacity directly and then consolidate esp_cmdfifo_pop() into esp_fifo_pop(). Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-5-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index b3471e0333..89cc795960 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -107,22 +107,14 @@ static void esp_fifo_push(Fifo8 *fifo, uint8_t val) fifo8_push(fifo, val); } -static uint8_t esp_fifo_pop(ESPState *s) + +static uint8_t esp_fifo_pop(Fifo8 *fifo) { - if (fifo8_is_empty(&s->fifo)) { + if (fifo8_is_empty(fifo)) { return 0; } - return fifo8_pop(&s->fifo); -} - -static uint8_t esp_cmdfifo_pop(ESPState *s) -{ - if (fifo8_is_empty(&s->cmdfifo)) { - return 0; - } - - return fifo8_pop(&s->cmdfifo); + return fifo8_pop(fifo); } static uint32_t esp_get_tc(ESPState *s) @@ -159,9 +151,9 @@ static uint8_t esp_pdma_read(ESPState *s) uint8_t val; if (s->do_cmd) { - val = esp_cmdfifo_pop(s); + val = esp_fifo_pop(&s->cmdfifo); } else { - val = esp_fifo_pop(s); + val = esp_fifo_pop(&s->fifo); } return val; @@ -887,7 +879,7 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr) qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n"); s->rregs[ESP_FIFO] = 0; } else { - s->rregs[ESP_FIFO] = esp_fifo_pop(s); + s->rregs[ESP_FIFO] = esp_fifo_pop(&s->fifo); } val = s->rregs[ESP_FIFO]; break; From 7b320a8e67a534925048cbabfa51431e0349dafd Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:54 +0100 Subject: [PATCH 06/13] esp: introduce esp_fifo_pop_buf() and use it instead of fifo8_pop_buf() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The const pointer returned by fifo8_pop_buf() lies directly within the array used to model the FIFO. Building with address sanitizers enabled shows that if the caller expects a minimum number of bytes present then if the FIFO is nearly full, the caller may unexpectedly access past the end of the array. Introduce esp_fifo_pop_buf() which takes a destination buffer and performs a memcpy() in it to guarantee that the caller cannot overwrite the FIFO array and update all callers to use it. Similarly add underflow protection similar to esp_fifo_push() and esp_fifo_pop() so that instead of triggering an assert() the operation becomes a no-op. Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland Tested-by: Alexander Bulekov Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Peter Maydell Message-Id: <20210407195801.685-6-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 89cc795960..bf22785b79 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -117,6 +117,23 @@ static uint8_t esp_fifo_pop(Fifo8 *fifo) return fifo8_pop(fifo); } +static uint32_t esp_fifo_pop_buf(Fifo8 *fifo, uint8_t *dest, int maxlen) +{ + const uint8_t *buf; + uint32_t n; + + if (maxlen == 0) { + return 0; + } + + buf = fifo8_pop_buf(fifo, maxlen, &n); + if (dest) { + memcpy(dest, buf, n); + } + + return n; +} + static uint32_t esp_get_tc(ESPState *s) { uint32_t dmalen; @@ -241,11 +258,11 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen) if (dmalen == 0) { return 0; } - memcpy(buf, fifo8_pop_buf(&s->fifo, dmalen, &n), dmalen); - if (dmalen >= 3) { + n = esp_fifo_pop_buf(&s->fifo, buf, dmalen); + if (n >= 3) { buf[0] = buf[2] >> 5; } - fifo8_push_all(&s->cmdfifo, buf, dmalen); + fifo8_push_all(&s->cmdfifo, buf, n); } trace_esp_get_cmd(dmalen, target); @@ -258,16 +275,16 @@ static uint32_t get_cmd(ESPState *s, uint32_t maxlen) static void do_busid_cmd(ESPState *s, uint8_t busid) { - uint32_t n, cmdlen; + uint32_t cmdlen; int32_t datalen; int lun; SCSIDevice *current_lun; - uint8_t *buf; + uint8_t buf[ESP_CMDFIFO_SZ]; trace_esp_do_busid_cmd(busid); lun = busid & 7; cmdlen = fifo8_num_used(&s->cmdfifo); - buf = (uint8_t *)fifo8_pop_buf(&s->cmdfifo, cmdlen, &n); + esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen); current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun); s->current_req = scsi_req_new(current_lun, 0, lun, buf, s); @@ -300,13 +317,12 @@ static void do_busid_cmd(ESPState *s, uint8_t busid) static void do_cmd(ESPState *s) { uint8_t busid = fifo8_pop(&s->cmdfifo); - uint32_t n; s->cmdfifo_cdb_offset--; /* Ignore extended messages for now */ if (s->cmdfifo_cdb_offset) { - fifo8_pop_buf(&s->cmdfifo, s->cmdfifo_cdb_offset, &n); + esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset); s->cmdfifo_cdb_offset = 0; } @@ -484,7 +500,7 @@ static void do_dma_pdma_cb(ESPState *s) /* Copy FIFO data to device */ len = MIN(s->async_len, ESP_FIFO_SZ); len = MIN(len, fifo8_num_used(&s->fifo)); - memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); + n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len); s->async_buf += n; s->async_len -= n; s->ti_size += n; @@ -492,7 +508,7 @@ static void do_dma_pdma_cb(ESPState *s) if (n < len) { /* Unaligned accesses can cause FIFO wraparound */ len = len - n; - memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); + n = esp_fifo_pop_buf(&s->fifo, s->async_buf, len); s->async_buf += n; s->async_len -= n; s->ti_size += n; @@ -668,7 +684,7 @@ static void esp_do_dma(ESPState *s) static void esp_do_nodma(ESPState *s) { int to_device = ((s->rregs[ESP_RSTAT] & 7) == STAT_DO); - uint32_t cmdlen, n; + uint32_t cmdlen; int len; if (s->do_cmd) { @@ -709,7 +725,7 @@ static void esp_do_nodma(ESPState *s) if (to_device) { len = MIN(fifo8_num_used(&s->fifo), ESP_FIFO_SZ); - memcpy(s->async_buf, fifo8_pop_buf(&s->fifo, len, &n), len); + esp_fifo_pop_buf(&s->fifo, s->async_buf, len); s->async_buf += len; s->async_len -= len; s->ti_size += len; From 99545751734035b76bd372c4e7215bb337428d89 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:55 +0100 Subject: [PATCH 07/13] esp: ensure cmdfifo is not empty and current_dev is non-NULL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When about to execute a SCSI command, ensure that cmdfifo is not empty and current_dev is non-NULL. This can happen if the guest tries to execute a TI (Transfer Information) command without issuing one of the select commands first. Buglink: https://bugs.launchpad.net/qemu/+bug/1910723 Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-7-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index bf22785b79..904fa3179c 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -284,6 +284,9 @@ static void do_busid_cmd(ESPState *s, uint8_t busid) trace_esp_do_busid_cmd(busid); lun = busid & 7; cmdlen = fifo8_num_used(&s->cmdfifo); + if (!cmdlen || !s->current_dev) { + return; + } esp_fifo_pop_buf(&s->cmdfifo, buf, cmdlen); current_lun = scsi_device_find(&s->bus, 0, s->current_dev->id, lun); From fa7505c154d4d00ad89a747be2eda556643ce00e Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:56 +0100 Subject: [PATCH 08/13] esp: don't underflow cmdfifo in do_cmd() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the guest tries to execute a CDB when cmdfifo is not empty before the start of the message out phase then clearing the message out phase data will cause cmdfifo to underflow due to cmdfifo_cdb_offset being larger than the amount of data within. Since this can only occur by issuing deliberately incorrect instruction sequences, ensure that the maximum length of esp_fifo_pop_buf() is limited to the size of the data within cmdfifo. Buglink: https://bugs.launchpad.net/qemu/+bug/1909247 Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-8-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 904fa3179c..d3b105b703 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -319,13 +319,15 @@ static void do_busid_cmd(ESPState *s, uint8_t busid) static void do_cmd(ESPState *s) { - uint8_t busid = fifo8_pop(&s->cmdfifo); + uint8_t busid = esp_fifo_pop(&s->cmdfifo); + int len; s->cmdfifo_cdb_offset--; /* Ignore extended messages for now */ if (s->cmdfifo_cdb_offset) { - esp_fifo_pop_buf(&s->cmdfifo, NULL, s->cmdfifo_cdb_offset); + len = MIN(s->cmdfifo_cdb_offset, fifo8_num_used(&s->cmdfifo)); + esp_fifo_pop_buf(&s->cmdfifo, NULL, len); s->cmdfifo_cdb_offset = 0; } From fbc6510e3379fa8f8370bf71198f0ce733bf07f9 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:57 +0100 Subject: [PATCH 09/13] esp: don't overflow cmdfifo in get_cmd() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-9-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index d3b105b703..9d3fdb4398 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->cmdfifo), dmalen); fifo8_push_all(&s->cmdfifo, buf, dmalen); } else { if (esp_select(s) < 0) { @@ -262,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); From 0ebb5fd80589835153a0c2baa1b8cc7a04e67a93 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:58 +0100 Subject: [PATCH 10/13] esp: don't overflow cmdfifo if TC is larger than the cmdfifo size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a guest transfers the message out/command phase data using DMA with a TC that is larger than the cmdfifo size then the cmdfifo overflows triggering an assert. Limit the size of the transfer to the free space available in cmdfifo. Buglink: https://bugs.launchpad.net/qemu/+bug/1919036 Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-10-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 9d3fdb4398..a26a109166 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -578,6 +578,7 @@ static void esp_do_dma(ESPState *s) cmdlen = fifo8_num_used(&s->cmdfifo); trace_esp_do_dma(cmdlen, len); if (s->dma_memory_read) { + len = MIN(len, fifo8_num_free(&s->cmdfifo)); s->dma_memory_read(s->dma_opaque, buf, len); fifo8_push_all(&s->cmdfifo, buf, len); } else { From 324c8809897c8c53ad05c3a7147d272f1711cd5e Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:59 +0100 Subject: [PATCH 11/13] esp: don't reset async_len directly in esp_select() if cancelling request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead let the SCSI layer invoke the .cancel callback itself to cancel and reset the request state. Signed-off-by: Mark Cave-Ayland Tested-by: Alexander Bulekov Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210407195801.685-11-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index a26a109166..0037197bdb 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -95,6 +95,7 @@ void esp_request_cancelled(SCSIRequest *req) scsi_req_unref(s->current_req); s->current_req = NULL; s->current_dev = NULL; + s->async_len = 0; } } @@ -206,7 +207,6 @@ static int esp_select(ESPState *s) if (s->current_req) { /* Started a new command before the old one finished. Cancel it. */ scsi_req_cancel(s->current_req); - s->async_len = 0; } s->current_dev = scsi_device_find(&s->bus, 0, target, 0); From 607206948cacda4a80be5b976dba490970a18a76 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:58:00 +0100 Subject: [PATCH 12/13] esp: ensure that do_cmd is set to zero before submitting an ESP select command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a CDB has been received and is about to be submitted to the SCSI layer via one of the ESP select commands, ensure that do_cmd is set to zero before executing the command. Otherwise a guest executing 2 valid CDBs in quick sequence can invoke the SCSI .transfer_data callback again before do_cmd is set to zero by the callback function triggering an assert at the start of esp_transfer_data(). Signed-off-by: Mark Cave-Ayland Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20210407195801.685-12-mark.cave-ayland@ilande.co.uk> --- hw/scsi/esp.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index 0037197bdb..b668acef82 100644 --- a/hw/scsi/esp.c +++ b/hw/scsi/esp.c @@ -357,6 +357,7 @@ static void handle_satn(ESPState *s) cmdlen = get_cmd(s, ESP_CMDFIFO_SZ); if (cmdlen > 0) { s->cmdfifo_cdb_offset = 1; + s->do_cmd = 0; do_cmd(s); } else if (cmdlen == 0) { s->do_cmd = 1; @@ -390,6 +391,7 @@ static void handle_s_without_atn(ESPState *s) cmdlen = get_cmd(s, ESP_CMDFIFO_SZ); if (cmdlen > 0) { s->cmdfifo_cdb_offset = 0; + s->do_cmd = 0; do_busid_cmd(s, 0); } else if (cmdlen == 0) { s->do_cmd = 1; From ce94fa7aa646a18e9b9105a32eea2152b202b431 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:58:01 +0100 Subject: [PATCH 13/13] tests/qtest: add tests for am53c974 device Use the autogenerated fuzzer test cases as the basis for a set of am53c974 regression tests. Signed-off-by: Mark Cave-Ayland Tested-by: Alexander Bulekov Message-Id: <20210407195801.685-13-mark.cave-ayland@ilande.co.uk> --- MAINTAINERS | 1 + tests/qtest/am53c974-test.c | 218 ++++++++++++++++++++++++++++++++++++ tests/qtest/meson.build | 1 + 3 files changed, 220 insertions(+) create mode 100644 tests/qtest/am53c974-test.c diff --git a/MAINTAINERS b/MAINTAINERS index 04beb34e7e..36055f14c5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1772,6 +1772,7 @@ F: include/hw/scsi/* F: hw/scsi/* F: tests/qtest/virtio-scsi-test.c F: tests/qtest/fuzz-virtio-scsi-test.c +F: tests/qtest/am53c974-test.c T: git https://github.com/bonzini/qemu.git scsi-next SSI diff --git a/tests/qtest/am53c974-test.c b/tests/qtest/am53c974-test.c new file mode 100644 index 0000000000..d996866cd4 --- /dev/null +++ b/tests/qtest/am53c974-test.c @@ -0,0 +1,218 @@ +/* + * QTest testcase for am53c974 + * + * Copyright (c) 2021 Mark Cave-Ayland + * + * This work is licensed under the terms of the GNU GPL, version 2 or + * later. See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" + +#include "libqos/libqtest.h" + + +static void test_cmdfifo_underflow_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outl(s, 0xcf8, 0x8000100e); + qtest_outl(s, 0xcfc, 0x8a000000); + qtest_outl(s, 0x8a09, 0x42000000); + qtest_outl(s, 0x8a0d, 0x00); + qtest_outl(s, 0x8a0b, 0x1000); + qtest_quit(s); +} + +/* Reported as crash_1548bd10e7 */ +static void test_cmdfifo_underflow2_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi -device scsi-hd,drive=disk0 " + "-drive id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001010); + qtest_outl(s, 0xcfc, 0xc000); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outw(s, 0xc00c, 0x41); + qtest_outw(s, 0xc00a, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00c, 0x43); + qtest_outw(s, 0xc00b, 0x00); + qtest_outw(s, 0xc00b, 0x00); + qtest_outw(s, 0xc00c, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00a, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00c, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00a, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00c, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00a, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00c, 0x00); + qtest_outl(s, 0xc00a, 0x00); + qtest_outl(s, 0xc006, 0x00); + qtest_outl(s, 0xc00b, 0x00); + qtest_outw(s, 0xc00b, 0x0800); + qtest_outw(s, 0xc00b, 0x00); + qtest_outw(s, 0xc00b, 0x00); + qtest_outl(s, 0xc006, 0x00); + qtest_outl(s, 0xc00b, 0x00); + qtest_outw(s, 0xc00b, 0x0800); + qtest_outw(s, 0xc00b, 0x00); + qtest_outw(s, 0xc00b, 0x4100); + qtest_outw(s, 0xc00a, 0x00); + qtest_outl(s, 0xc00a, 0x100000); + qtest_outl(s, 0xc00a, 0x00); + qtest_outw(s, 0xc00c, 0x43); + qtest_outl(s, 0xc00a, 0x100000); + qtest_outl(s, 0xc00a, 0x100000); + qtest_quit(s); +} + +static void test_cmdfifo_overflow_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outl(s, 0xcf8, 0x8000100e); + qtest_outl(s, 0xcfc, 0x0e000000); + qtest_outl(s, 0xe40, 0x03); + qtest_outl(s, 0xe0b, 0x4100); + qtest_outl(s, 0xe0b, 0x9000); + qtest_quit(s); +} + +/* Reported as crash_530ff2e211 */ +static void test_cmdfifo_overflow2_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi -device scsi-hd,drive=disk0 " + "-drive id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001010); + qtest_outl(s, 0xcfc, 0xc000); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outl(s, 0xc00b, 0x4100); + qtest_outw(s, 0xc00b, 0xc200); + qtest_outl(s, 0xc03f, 0x0300); + qtest_quit(s); +} + +/* Reported as crash_0900379669 */ +static void test_fifo_pop_buf(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi -device scsi-hd,drive=disk0 " + "-drive id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001010); + qtest_outl(s, 0xcfc, 0xc000); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outb(s, 0xc000, 0x4); + qtest_outb(s, 0xc008, 0xa0); + qtest_outl(s, 0xc03f, 0x0300); + qtest_outl(s, 0xc00b, 0xc300); + qtest_outw(s, 0xc00b, 0x9000); + qtest_outl(s, 0xc00b, 0xc300); + qtest_outl(s, 0xc00b, 0xc300); + qtest_outl(s, 0xc00b, 0xc300); + qtest_outw(s, 0xc00b, 0x9000); + qtest_outw(s, 0xc00b, 0x1000); + qtest_quit(s); +} + +static void test_target_selected_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001001); + qtest_outl(s, 0xcfc, 0x01000000); + qtest_outl(s, 0xcf8, 0x8000100e); + qtest_outl(s, 0xcfc, 0xef800000); + qtest_outl(s, 0xef8b, 0x4100); + qtest_outw(s, 0xef80, 0x01); + qtest_outl(s, 0xefc0, 0x03); + qtest_outl(s, 0xef8b, 0xc100); + qtest_outl(s, 0xef8b, 0x9000); + qtest_quit(s); +} + +static void test_fifo_underflow_on_write_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001010); + qtest_outl(s, 0xcfc, 0xc000); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x01); + qtest_outl(s, 0xc008, 0x0a); + qtest_outl(s, 0xc009, 0x41000000); + qtest_outl(s, 0xc009, 0x41000000); + qtest_outl(s, 0xc00b, 0x1000); + qtest_quit(s); +} + +static void test_cancelled_request_ok(void) +{ + QTestState *s = qtest_init( + "-device am53c974,id=scsi " + "-device scsi-hd,drive=disk0 -drive " + "id=disk0,if=none,file=null-co://,format=raw -nodefaults"); + qtest_outl(s, 0xcf8, 0x80001010); + qtest_outl(s, 0xcfc, 0xc000); + qtest_outl(s, 0xcf8, 0x80001004); + qtest_outw(s, 0xcfc, 0x05); + qtest_outb(s, 0xc046, 0x02); + qtest_outl(s, 0xc00b, 0xc100); + qtest_outl(s, 0xc040, 0x03); + qtest_outl(s, 0xc040, 0x03); + qtest_bufwrite(s, 0x0, "\x41", 0x1); + qtest_outl(s, 0xc00b, 0xc100); + qtest_outw(s, 0xc040, 0x02); + qtest_outw(s, 0xc040, 0x81); + qtest_outl(s, 0xc00b, 0x9000); + qtest_quit(s); +} + +int main(int argc, char **argv) +{ + const char *arch = qtest_get_arch(); + + g_test_init(&argc, &argv, NULL); + + if (strcmp(arch, "i386") == 0) { + qtest_add_func("am53c974/test_cmdfifo_underflow_ok", + test_cmdfifo_underflow_ok); + qtest_add_func("am53c974/test_cmdfifo_underflow2_ok", + test_cmdfifo_underflow2_ok); + qtest_add_func("am53c974/test_cmdfifo_overflow_ok", + test_cmdfifo_overflow_ok); + qtest_add_func("am53c974/test_cmdfifo_overflow2_ok", + test_cmdfifo_overflow2_ok); + qtest_add_func("am53c974/test_fifo_pop_buf", + test_fifo_pop_buf); + qtest_add_func("am53c974/test_target_selected_ok", + test_target_selected_ok); + qtest_add_func("am53c974/test_fifo_underflow_on_write_ok", + test_fifo_underflow_on_write_ok); + qtest_add_func("am53c974/test_cancelled_request_ok", + test_cancelled_request_ok); + } + + return g_test_run(); +} diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index 420cd9986e..0c76738921 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -65,6 +65,7 @@ qtests_i386 = \ (config_all_devices.has_key('CONFIG_TPM_TIS_ISA') ? ['tpm-tis-swtpm-test'] : []) + \ (config_all_devices.has_key('CONFIG_RTL8139_PCI') ? ['rtl8139-test'] : []) + \ (config_all_devices.has_key('CONFIG_E1000E_PCI_EXPRESS') ? ['fuzz-e1000e-test'] : []) + \ + (config_all_devices.has_key('CONFIG_ESP_PCI') ? ['am53c974-test'] : []) + \ qtests_pci + \ ['fdc-test', 'ide-test',