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

[Mauro Matteo Cascella]

On Tue, Aug 2, 2022 at 3:48 PM Thomas Huth <thuth@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 <thuth@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.

> +        }
>          le64_to_cpus(&trb.parameter);
>          le32_to_cpus(&trb.status);
>          le32_to_cpus(&trb.control);
> --
> 2.31.1
>

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