XHCI is a complex controller, involving lots of descriptors that are written and read via DMA transfers. As some recent bugs like https://gitlab.com/qemu-project/qemu/-/work_items/3926 revealed, this can sometimes be exploited from the guest side to crash or stall QEMU.
The code currently does DMA writes with the MEMTXATTRS_UNSPECIFIED attribute, i.e. the controller is allowed to write to other MMIO regions, too. However, in normal operation, this should not be necessary, all descriptors should reside in normal memory. So let's decrease the attack surface a little bit and limit the DMA writes to normal memory here. Signed-off-by: Thomas Huth <[email protected]> --- This would have prevented bug 3926 from happening, too. However, I'm not sure whether there are some obscure scenarios where writes to MMIO regions could still be necessary, thus I've marked this patch as RFC. Does anybody got an opinion on this? If not, maybe we should give it a try and revert the patch if someone finds a scenario where this is causing problems? hw/usb/hcd-xhci.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index d342aa2739e..c567d173a14 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -512,6 +512,7 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, int i; uint32_t tmp[5]; uint32_t n = len / sizeof(uint32_t); + const MemTxAttrs memtx_attrs = { .memory = true }; assert((len % sizeof(uint32_t)) == 0); assert(n <= ARRAY_SIZE(tmp)); @@ -519,8 +520,7 @@ static inline void xhci_dma_write_u32s(XHCIState *xhci, dma_addr_t addr, for (i = 0; i < n; i++) { tmp[i] = cpu_to_le32(buf[i]); } - if (dma_memory_write(xhci->as, addr, tmp, len, - MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) { + if (dma_memory_write(xhci->as, addr, tmp, len, memtx_attrs) != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n", __func__); xhci_die(xhci); @@ -607,6 +607,7 @@ static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v) XHCIInterrupter *intr = &xhci->intr[v]; XHCITRB ev_trb; dma_addr_t addr; + const MemTxAttrs memtx_attrs = { .memory = true }; ev_trb.parameter = cpu_to_le64(event->ptr); ev_trb.status = cpu_to_le32(event->length | (event->ccode << 24)); @@ -623,7 +624,7 @@ static void xhci_write_event(XHCIState *xhci, XHCIEvent *event, int v) addr = intr->er_start + TRB_SIZE*intr->er_ep_idx; if (dma_memory_write(xhci->as, addr, &ev_trb, TRB_SIZE, - MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) { + memtx_attrs) != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n", __func__); xhci_die(xhci); @@ -2440,6 +2441,7 @@ static void xhci_detach_slot(XHCIState *xhci, USBPort *uport) static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx) { dma_addr_t ctx; + const MemTxAttrs memtx_attrs = { .memory = true }; DPRINTF("xhci_get_port_bandwidth()\n"); @@ -2448,9 +2450,9 @@ static TRBCCode xhci_get_port_bandwidth(XHCIState *xhci, uint64_t pctx) DPRINTF("xhci: bandwidth context at "DMA_ADDR_FMT"\n", ctx); /* TODO: actually implement real values here. This is 80% for all ports. */ - if (stb_dma(xhci->as, ctx, 0, MEMTXATTRS_UNSPECIFIED) != MEMTX_OK || + if (stb_dma(xhci->as, ctx, 0, memtx_attrs) != MEMTX_OK || dma_memory_set(xhci->as, ctx + 1, 80, xhci->numports, - MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) { + memtx_attrs) != MEMTX_OK) { qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory write failed!\n", __func__); return CC_TRB_ERROR; -- 2.55.0
