[RFC PATCH] hw/usb/hcd-xhci: Limit DMA transfers to plain memory

[RFC PATCH] hw/usb/hcd-xhci: Limit DMA transfers to plain memory

[Thomas Huth]

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 <thuth@redhat.com>
---
 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

Re: [RFC PATCH] hw/usb/hcd-xhci: Limit DMA transfers to plain memory

[Michael Tokarev]

On 9/15/26 15:32, Thomas Huth wrote:
> 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 <thuth@redhat.com>
> ---
>   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(-)

I'll queue this one up for the stable qemu series too.
Having in mind the above doubt, I'm not applying it immediately
but for the next series.  There is some risk here, but it is
more theoretical, I think.

Thanks,

/mjt