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