qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fai


From: Thomas Huth
Subject: Re: [PATCH] hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394)
Date: Thu, 4 Aug 2022 14:29:14 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0

On 04/08/2022 13.43, Thomas Huth wrote:
On 04/08/2022 12.17, Peter Maydell wrote:
On Thu, 4 Aug 2022 at 11:07, Thomas Huth <thuth@redhat.com> wrote:

On 04/08/2022 10.56, Peter Maydell wrote:
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.

That sounds like we do still have an unbounded-loop problem,
then: there's no limit on the number of consecutive TRBs
we try to read in that function. Maybe we're missing an
error check of some kind (does the spec limit how many
consecutive TRBs there can be somehow?) or else we need
another artificial limit.

I'm not an XHCI expert at all, but while at least having a quick glance at the spec, I did not see any limit there. So I assume that we should enforce an artificial limit? What would be a good value for this? Gerd, do you maybe have any opinion?

Ok, after looking at the spec a little bit longer, I discovered chapter "6 Data Structures", where they say that Transfer Ring segments should be limited to 64kB each. That sounds like a reasonable limit. I'll update my patch accordingly.

 Thomas




reply via email to

[Prev in Thread] Current Thread [Next in Thread]