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 <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.

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