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

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

[P J P]

From: Prasad J Pandit <pjp@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.

Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/usb/hcd-xhci.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 188f954..17dba2a 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -140,6 +140,7 @@
 #define ERDP_EHB        (1<<3)
 
 #define TRB_SIZE 16
+#define TRB_MAX  4000
 typedef struct XHCITRB {
     uint64_t parameter;
     uint32_t status;
@@ -999,9 +1000,10 @@ static void xhci_ring_init(XHCIState *xhci, XHCIRing *ring,
 static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
                                dma_addr_t *addr)
 {
+    uint16_t trbcnt = 0;
     PCIDevice *pci_dev = PCI_DEVICE(xhci);
 
-    while (1) {
+    while (trbcnt++ < TRB_MAX) {
         TRBType type;
         pci_dma_read(pci_dev, ring->dequeue, trb, TRB_SIZE);
         trb->addr = ring->dequeue;
@@ -1032,6 +1034,8 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing *ring, XHCITRB *trb,
             }
         }
     }
+
+    return 0;
 }
 
 static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
-- 
2.5.5

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

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

On Do, 2016-10-06 at 11:20 +0530, P J P wrote:
> From: Prasad J Pandit <pjp@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


[-- Attachment #2: 0001-xhci-limit-the-number-of-link-trbs-we-are-willing-to.patch --]
[-- Type: text/x-patch, Size: 2099 bytes --]

From 20009bdaf95d10bf748fa69b104672d3cfaceddf Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@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 <kraxel@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

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

[P J P]

  Hello Gerd,

+-- On Fri, 7 Oct 2016, Gerd Hoffmann wrote --+
| 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. 

  Okay.

| 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?

  Yes, the attached patch does fix this issue. 

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F