[Qemu-devel] [PATCH 1/2] ehci: apply limit to itd/sidt descriptors

[Qemu-devel] [PATCH 1/2] ehci: apply limit to itd/sidt descriptors

[Gerd Hoffmann]

Commit "156a2e4 ehci: make idt processing more robust" tries to avoid a
DoS by the guest (create a circular itd queue and let qemu ehci
emulation run in circles forever).  Unfortunaly this has two problems:
First it misses the case of sitds, and second it reportly breaks
freebsd.

So lets go for a different approach: just count the number of itds and
sitds we have seen per frame and apply a limit.  That should really
catch all cases now.

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

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 159f58d..923f110 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2011,6 +2011,7 @@ static int ehci_state_writeback(EHCIQueue *q)
 static void ehci_advance_state(EHCIState *ehci, int async)
 {
     EHCIQueue *q = NULL;
+    int idt_count = 0;
     int again;
 
     do {
@@ -2035,10 +2036,12 @@ static void ehci_advance_state(EHCIState *ehci, int async)
 
         case EST_FETCHITD:
             again = ehci_state_fetchitd(ehci, async);
+            idt_count++;
             break;
 
         case EST_FETCHSITD:
             again = ehci_state_fetchsitd(ehci, async);
+            idt_count++;
             break;
 
         case EST_ADVANCEQUEUE:
@@ -2092,6 +2095,11 @@ static void ehci_advance_state(EHCIState *ehci, int async)
             ehci_reset(ehci);
             again = 0;
         }
+
+        /* limit the amout of idts we are willing to process each frame */
+        if (idt_count > 16) {
+            again = 0;
+        }
     }
     while (again);
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] Revert "ehci: make idt processing more robust"

[Gerd Hoffmann]

This reverts commit 156a2e4dbffa85997636a7a39ef12da6f1b40254.

Breaks freebsd.
---
 hw/usb/hcd-ehci.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 923f110..7add81c 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1397,7 +1397,7 @@ static int ehci_process_itd(EHCIState *ehci,
 {
     USBDevice *dev;
     USBEndpoint *ep;
-    uint32_t i, len, pid, dir, devaddr, endp, xfers = 0;
+    uint32_t i, len, pid, dir, devaddr, endp;
     uint32_t pg, off, ptr1, ptr2, max, mult;
 
     ehci->periodic_sched_active = PERIODIC_ACTIVE;
@@ -1489,10 +1489,9 @@ static int ehci_process_itd(EHCIState *ehci,
                 ehci_raise_irq(ehci, USBSTS_INT);
             }
             itd->transact[i] &= ~ITD_XACT_ACTIVE;
-            xfers++;
         }
     }
-    return xfers ? 0 : -1;
+    return 0;
 }
 
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] ehci: apply limit to itd/sidt descriptors

[P J P]

+-- On Mon, 18 Apr 2016, Gerd Hoffmann wrote --+
| Commit "156a2e4 ehci: make idt processing more robust" tries to avoid a
| DoS by the guest (create a circular itd queue and let qemu ehci
| emulation run in circles forever).  Unfortunaly this has two problems:
| First it misses the case of sitds, and second it reportly breaks
| freebsd.
| 
| So lets go for a different approach: just count the number of itds and
| sitds we have seen per frame and apply a limit.  That should really
| catch all cases now.

  idt -> iTD
  sidt -> siTD
  Unfortualy -> Unfortunately
  reportly -> reportedly
  freebsd -> FreeBSD

Perhaps it'll help to add "Fixes: 156a2e4(CVE-2015-8558)" to the commit log? 
(just a thought)

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