Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)

[Thomas Huth]

On 04/08/2022 10.45, Mauro Matteo Cascella wrote:
On Tue, Aug 2, 2022 at 3:48 PM Thomas Huth <th...@redhat.com> wrote:

The XHCI code could enter an endless loop in case the guest points
QEMU to fetch TRBs from invalid memory areas. Fix it by properly
checking the return value of dma_memory_read().

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/646
Signed-off-by: Thomas Huth <th...@redhat.com>
---
  hw/usb/hcd-xhci.c | 17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 296cc6c8e6..63d428a444 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"
@@ -679,8 +680,12 @@ static TRBType xhci_ring_fetch(XHCIState *xhci, XHCIRing 
*ring, XHCITRB *trb,

      while (1) {
          TRBType type;
-        dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
-                        MEMTXATTRS_UNSPECIFIED);
+        if (dma_memory_read(xhci->as, ring->dequeue, trb, TRB_SIZE,
+                            MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: DMA memory access failed!\n",
+                          __func__);
+            return 0;
+        }
          trb->addr = ring->dequeue;
          trb->ccs = ring->ccs;
          le64_to_cpus(&trb->parameter);
@@ -727,8 +732,12 @@ static int xhci_ring_chain_length(XHCIState *xhci, const 
XHCIRing *ring)

      while (1) {
          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 -length;

Not strictly related to this issue, but what's the point of returning
-length instead of e.g. -1? Apart from that, LGTM. Thank you.

Yeah, that's a little bit weird, but it's also what the other spots in this 
function are doing, so I didn't want to diverge from that.

 Thomas