| Message ID | 20220804131300.96368-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,for-7.1] hw/usb/hcd-xhci: Fix unbounded loop in xhci_ring_chain_length() (CVE-2020-14394) | expand |
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,
> + > + /* > + * 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); Acked-by: Gerd Hoffmann <kraxel@redhat.com> take care, Gerd
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)
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(-)