Re: [Qemu-devel] [PATCH] usb: xHCI: add check to limit command TRB processing

[Gerd Hoffmann]

On Do, 2016-10-06 at 11:20 +0530, P J P wrote:
> From: Prasad J Pandit <p...@fedoraproject.org>
> 
> USB xHCI controller uses ring of Transfer Request Blocks(TRB)
> to process USB commands. These are processed by loop in
> 'xhci_ring_fetch'. A guest user could make it read and process
> a same TRB infinitely. Limit number of command TRBs to avoid it.

I think it is better to apply the limit to link trbs only (which allow
to jump to another address so the guest can build loops with it).  Also
I think the limit can be much stricter then without breaking stuff as
typically a link trb is used at the end of a page full of normal trbs,
to jump to the next page with trbs.  And we have the same problem in
both xhci_ring_fetch and xhci_ring_chain_length, so we should fix both.

Is there a reproducer?  If so, can you try the attached patch with it?

thanks,
  Gerd


From 20009bdaf95d10bf748fa69b104672d3cfaceddf Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kra...@redhat.com>
Date: Fri, 7 Oct 2016 10:15:29 +0200
Subject: [PATCH] xhci: limit the number of link trbs we are willing to process

Signed-off-by: Gerd Hoffmann <kra...@redhat.com>
---
 hw/usb/hcd-xhci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 726435c..ee4fa48 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -54,6 +54,8 @@
  * to the specs when it gets them */
 #define ER_FULL_HACK
 
+#define TRB_LINK_LIMIT  4
+
 #define LEN_CAP         0x40
 #define LEN_OPER        (0x400 + 0x10 * MAXPORTS)
 #define LEN_RUNTIME     ((MAXINTRS + 1) * 0x20)
@@ -1000,6 +1002,7 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
                                dma_addr_t *addr)
 {
     PCIDevice *pci_dev = PCI_DEVICE(xhci);
+    uint32_t link_cnt = 0;
 
     while (1) {
         TRBType type;
@@ -1026,6 +1029,9 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
             ring->dequeue += TRB_SIZE;
             return type;
         } else {
+            if (++link_cnt > TRB_LINK_LIMIT) {
+                return 0;
+            }
             ring->dequeue = xhci_mask64(trb->parameter);
             if (trb->control & TRB_LK_TC) {
                 ring->ccs = !ring->ccs;
@@ -1043,6 +1049,7 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
     bool ccs = ring->ccs;
     /* hack to bundle together the two/three TDs that make a setup transfer */
     bool control_td_set = 0;
+    uint32_t link_cnt = 0;
 
     while (1) {
         TRBType type;
@@ -1058,6 +1065,9 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
         type = TRB_TYPE(trb);
 
         if (type == TR_LINK) {
+            if (++link_cnt > TRB_LINK_LIMIT) {
+                return -length;
+            }
             dequeue = xhci_mask64(trb.parameter);
             if (trb.control & TRB_LK_TC) {
                 ccs = !ccs;
-- 
1.8.3.1