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.56, Peter Maydell wrote:
> On Thu, 4 Aug 2022 at 09:00, Thomas Huth <thuth@redhat.com> wrote:
> > On 02/08/2022 16.09, Peter Maydell wrote:
> > > On Tue, 2 Aug 2022 at 14:53, 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().
> > >
> > > It certainly makes sense to check the return value from
> > > dma_memory_read(), but how can we end up in an infinite loop
> > > if it fails? Either:
> > >
> > > (1) there is some combination of data values which allow an
> > > infinite loop, so we can hit those if the DMA access fails and
> > > we use bogus uninitialized data. But then the guest could
> > > maliciously pass us those bad values and create an infinite
> > > loop without a failing DMA access, ie commit 05f43d44e4b
> > > missed a case. If so we need to fix that!
> >
> > No, as far as I can see, that's not the case.
> >
> > > Or (2) there isn't actually an infinite loop possible here,
> > > and we're just tidying up a case which is C undefined
> > > behaviour but in practice unlikely to have effects any
> > > worse than the guest could provoke anyway (ie running up
> > > to the TRB_LINK_LIMIT and stopping). In this case the
> > > commit message here is a bit misleading.
> >
> > So from what I understand, this is happening: The guest somehow manages to
> > trick QEMU in a state where it reads through a set of TRBs where none is
> > marked in a way that could cause the function to return. QEMU then reads all
> > memory 'till the end of RAM and then continues to loop through no-mans-land
> > since the return value of dma_memory_read() is not checked.
>
> But the point of TRB_LINK_LIMIT is that regardless of what the
> contents of the TRBs are, the loop is not supposed to
> be able to continue for more than TRB_LINK_LIMIT iterations,
> ie 32 times. In this example case, do we stop after 32 TRBs
> (case 2) or not (case 1)?

Oh, wait, I think we were maybe looking at different spots. The problem 
likely does not occur in the xhci_ring_fetch() function
(which you were likely looking at), but only in the xhci_ring_chain_length() 
function (which I was looking at)!
xhci_ring_chain_length() can certainly continue more than 32 times. In 
xhci_ring_chain_length() the TRB_LINK_LIMIT only applies if "type == 
TR_LINK", but the TRBs we're talking about here are *not* of type TR_LINK.

 Thomas