From ff4a1daba6adc8811efb5046483feb3af6bd8d83 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 13:48:42 +0100 Subject: 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 c3d3dab..9db10b1 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 507ab36..d87e1a6 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 9508849..aada368 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 -- cgit v1.1 From 0db895361b8a82e1114372ff9f4857abea605701 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:50 +0100 Subject: 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 d87e1a6..a79196f 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; -- cgit v1.1 From e392255766071c8cac480da3a9ae4f94e56d7cbc Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:51 +0100 Subject: 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 a79196f..2584ec6 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); -- cgit v1.1 From e5455b8c1c6170c788f3c0fd577cc3be53539a99 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:52 +0100 Subject: 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 2584ec6..b3471e0 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 */ -- cgit v1.1 From c5fef9112b15c4b5494791cdf8bbb40bc1938dd3 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:53 +0100 Subject: 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 | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c index b3471e0..89cc795 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) -{ - if (fifo8_is_empty(&s->fifo)) { - return 0; - } - - return fifo8_pop(&s->fifo); -} -static uint8_t esp_cmdfifo_pop(ESPState *s) +static uint8_t esp_fifo_pop(Fifo8 *fifo) { - if (fifo8_is_empty(&s->cmdfifo)) { + if (fifo8_is_empty(fifo)) { 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; -- cgit v1.1 From 7b320a8e67a534925048cbabfa51431e0349dafd Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:54 +0100 Subject: 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 89cc795..bf22785 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; -- cgit v1.1 From 99545751734035b76bd372c4e7215bb337428d89 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:55 +0100 Subject: 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 bf22785..904fa31 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); -- cgit v1.1 From fa7505c154d4d00ad89a747be2eda556643ce00e Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:56 +0100 Subject: 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 904fa31..d3b105b 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; } -- cgit v1.1 From fbc6510e3379fa8f8370bf71198f0ce733bf07f9 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:57 +0100 Subject: 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 d3b105b..9d3fdb4 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); -- cgit v1.1 From 0ebb5fd80589835153a0c2baa1b8cc7a04e67a93 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:58 +0100 Subject: 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 9d3fdb4..a26a109 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 { -- cgit v1.1 From 324c8809897c8c53ad05c3a7147d272f1711cd5e Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:57:59 +0100 Subject: 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 a26a109..0037197 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); -- cgit v1.1 From 607206948cacda4a80be5b976dba490970a18a76 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:58:00 +0100 Subject: 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 0037197..b668ace 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; -- cgit v1.1 From ce94fa7aa646a18e9b9105a32eea2152b202b431 Mon Sep 17 00:00:00 2001 From: Mark Cave-Ayland Date: Wed, 7 Apr 2021 20:58:01 +0100 Subject: 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 04beb34..36055f1 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 0000000..d996866 --- /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 420cd99..0c76738 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', -- cgit v1.1