[PATCH v2 for-7.1] hw/usb/hcd-xhci: Fix unbounded loop in xhci_ring_chain_length() (CVE-2020-14394)

[PATCH v2 for-7.1] hw/usb/hcd-xhci: Fix unbounded loop in xhci_ring_chain_length() (CVE-2020-14394)

[Thomas Huth]

The loop condition in xhci_ring_chain_length() is under control of
the guest, and additionally the code does not check for failed DMA
transfers (e.g. if reaching the end of the RAM), so the loop there
could run for a very long time or even forever. Fix it by checking
the return value of dma_memory_read() and by introducing a maximum
loop length.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/646
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2:
 - Reworded subject and commit description
 - Focus on xhci_ring_chain_length() since that's the only function
   where an endless loop can currently occur. The other spots that
   ignore the return value of dma_memory_read() should be fixed as
   well later, but that's rather something for QEMU 7.2.
 - Added an real limit for the loop, so that it also ends after a
   while in case there are no DMA errors
 - Use "return -1" instead of "return -length" since the latter
   is somewhat weird (could be sometimes 0, sometimes negative)

 hw/usb/hcd-xhci.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 5a1ddf8c3e..b5669bc234 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -21,6 +21,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/timer.h"
+#include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/queue.h"
 #include "migration/vmstate.h"
@@ -729,10 +730,14 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
     bool control_td_set = 0;
     uint32_t link_cnt = 0;
 
-    while (1) {
+    do {
         TRBType type;
-        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
-                        MEMTXATTRS_UNSPECIFIED);
+        if (dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
+                        MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
+                          __func__);
+            return -1;
+        }
         le64_to_cpus(&trb.parameter);
         le32_to_cpus(&trb.status);
         le32_to_cpus(&trb.control);
@@ -766,7 +771,17 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
         if (!control_td_set && !(trb.control & TRB_TR_CH)) {
             return length;
         }
-    }
+
+        /*
+         * According to the xHCI spec, Transfer Ring segments should have
+         * a maximum size of 64 kB (see chapter "6 Data Structures")
+         */
+    } while (length < TRB_LINK_LIMIT * 65536 / TRB_SIZE);
+
+    qemu_log_mask(LOG_GUEST_ERROR, "%s: exceeded maximum tranfer ring size!\n",
+                          __func__);
+
+    return -1;
 }
 
 static void xhci_er_reset(XHCIState *xhci, int v)
-- 
2.31.1

Re: [PATCH v2 for-7.1] hw/usb/hcd-xhci: Fix unbounded loop in xhci_ring_chain_length() (CVE-2020-14394)

[Mauro Matteo Cascella]

On Thu, Aug 4, 2022 at 3:13 PM Thomas Huth <thuth@redhat.com> wrote:
>
> The loop condition in xhci_ring_chain_length() is under control of
> the guest, and additionally the code does not check for failed DMA
> transfers (e.g. if reaching the end of the RAM), so the loop there
> could run for a very long time or even forever. Fix it by checking
> the return value of dma_memory_read() and by introducing a maximum
> loop length.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/646
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2:
>  - Reworded subject and commit description
>  - Focus on xhci_ring_chain_length() since that's the only function
>    where an endless loop can currently occur. The other spots that
>    ignore the return value of dma_memory_read() should be fixed as
>    well later, but that's rather something for QEMU 7.2.
>  - Added an real limit for the loop, so that it also ends after a
>    while in case there are no DMA errors
>  - Use "return -1" instead of "return -length" since the latter
>    is somewhat weird (could be sometimes 0, sometimes negative)
>
>  hw/usb/hcd-xhci.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 5a1ddf8c3e..b5669bc234 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -21,6 +21,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/timer.h"
> +#include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "qemu/queue.h"
>  #include "migration/vmstate.h"
> @@ -729,10 +730,14 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
>      bool control_td_set = 0;
>      uint32_t link_cnt = 0;
>
> -    while (1) {
> +    do {
>          TRBType type;
> -        dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
> -                        MEMTXATTRS_UNSPECIFIED);
> +        if (dma_memory_read(xhci->as, dequeue, &trb, TRB_SIZE,
> +                        MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
> +                          __func__);
> +            return -1;
> +        }
>          le64_to_cpus(&trb.parameter);
>          le32_to_cpus(&trb.status);
>          le32_to_cpus(&trb.control);
> @@ -766,7 +771,17 @@ static int xhci_ring_chain_length(XHCIState *xhci, const XHCIRing *ring)
>          if (!control_td_set && !(trb.control & TRB_TR_CH)) {
>              return length;
>          }
> -    }
> +
> +        /*
> +         * According to the xHCI spec, Transfer Ring segments should have
> +         * a maximum size of 64 kB (see chapter "6 Data Structures")
> +         */
> +    } while (length < TRB_LINK_LIMIT * 65536 / TRB_SIZE);
> +
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: exceeded maximum tranfer ring size!\n",
> +                          __func__);
> +
> +    return -1;
>  }
>
>  static void xhci_er_reset(XHCIState *xhci, int v)
> --
> 2.31.1
>

Reviewed-by: Mauro Matteo Cascella <mcascell@redhat.com>

Thanks,
-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

Re: [PATCH v2 for-7.1] hw/usb/hcd-xhci: Fix unbounded loop in xhci_ring_chain_length() (CVE-2020-14394)

[Gerd Hoffmann]

> +
> +        /*
> +         * According to the xHCI spec, Transfer Ring segments should have
> +         * a maximum size of 64 kB (see chapter "6 Data Structures")
> +         */
> +    } while (length < TRB_LINK_LIMIT * 65536 / TRB_SIZE);

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd