| Message ID | 20220802134834.454749-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/usb/hcd-xhci: Fix endless loop in case the DMA access fails (CVE-2020-14394) | expand |
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! 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. thanks -- PMM
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. Since dma_memory_read() was not able to fetch the memory, the "trb" struct does not get updated, i.e. continues to contain data that does not cause the function to return. Thus the while(1) loop in xhci_ring_chain_length() happily runs through the whole 64-bit address space range, failing to read memory and thus leaving the loop. Maybe the loop is not really endless if it wrap-arounds the 64-bit address space at one point in time and finally finds another chunk of memory in the real RAM that causes the loop to exit, but it would still certainly take a long time where QEMU is just completely unresponsive and busy stepping through the no-mans-land. Does that make sense to you now? If so, I can respin a v2 with an improved patch description if you like. Thomas
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 >
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
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)? thanks -- PMM
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
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. thanks -- PMM
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? Thomas
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
On Thu, Aug 04, 2022 at 01:43:14PM +0200, Thomas Huth wrote: > On 04/08/2022 12.17, Peter Maydell wrote: > > 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? Hmm, dunno. Typical workflow is that the driver allocates a page (or multiple physically contiguous pages) for the TRBs, and when it reaches the end of the allocation it gets a new block and chains it using TRB_LINK. Right now we have a limit for type=TRB_LINK only, having another one for all TRBs makes sense. The number of TRBs for a transfer can be quite large (think usb-storage reads/writes), so don't set that too low, 64k maybe? take care, Gerd
On 16/08/2022 10.37, Gerd Hoffmann wrote: > On Thu, Aug 04, 2022 at 01:43:14PM +0200, Thomas Huth wrote: >> On 04/08/2022 12.17, Peter Maydell wrote: >>> 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? > > Hmm, dunno. Typical workflow is that the driver allocates a page > (or multiple physically contiguous pages) for the TRBs, and when > it reaches the end of the allocation it gets a new block and chains > it using TRB_LINK. > > Right now we have a limit for type=TRB_LINK only, having another one for > all TRBs makes sense. The number of TRBs for a transfer can be quite > large (think usb-storage reads/writes), so don't set that too low, 64k > maybe? Yes, after some more reading, I found a remark in the spec (in chapter 6) saying that each segment should be limited by 64 kb. I've sent a v2 with that limit check here (it has a slightly different subject, so it's hard to relate it to this v1 here): https://patchwork.kernel.org/project/qemu-devel/patch/20220804131300.96368-1-thuth@redhat.com/ Thomas
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; + } le64_to_cpus(&trb.parameter); le32_to_cpus(&trb.status); le32_to_cpus(&trb.control);
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(-)