On Thu, Aug 4, 2022 at 3:13 PM Thomas Huth <th...@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 <th...@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 <mcasc...@redhat.com> Thanks, -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0