On Thu, 13 Aug 2026 at 08:24, Jamin Lin <[email protected]> wrote: > > Coverity reports that ehci_writeback_async_complete_packet() ignores > the return value of get_dwords() when reading the QH and qTD. > > Handle read failures in the same way as QH and qTD verification > failures by freeing the packet and returning early. > > Signed-off-by: Jamin Lin <[email protected]> > --- > hw/usb/hcd-ehci.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c > index 451a918e9f..d43951975a 100644 > --- a/hw/usb/hcd-ehci.c > +++ b/hw/usb/hcd-ehci.c > @@ -533,11 +533,11 @@ static void > ehci_writeback_async_complete_packet(EHCIPacket *p) > /* Verify the qh + qtd, like we do when going through fetchqh & fetchqtd > */ > memset(&qh, 0, sizeof(qh)); > memset(&qtd, 0, sizeof(qtd)); > - get_dwords(q->ehci, NLPTR_GET(q->qhaddr), > - (uint32_t *) &qh, ehci_qh_dwords(q->ehci)); > - get_dwords(q->ehci, NLPTR_GET(q->qtdaddr), > - (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci)); > - if (!ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) { > + if (get_dwords(q->ehci, NLPTR_GET(q->qhaddr), > + (uint32_t *) &qh, ehci_qh_dwords(q->ehci)) < 0 || > + get_dwords(q->ehci, NLPTR_GET(q->qtdaddr), > + (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci)) < 0 || > + !ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) { > p->async = EHCI_ASYNC_INITIALIZED; > ehci_free_packet(p); > return;
Looking at the coverity report, my question is whether echi->as can ever actually be NULL. This is the address space we use to do DMA, so it feels like every EHCI device must set that up somehow. ehci_sysbus_init() does. So does usb_ehci_pci_realize(). usb_ehci_pci_write_config() can change it, but never to NULL. If echi->as is always non-NULL then we could change get_dwords() and put_dwords() to return "void". Alternatively, maybe get_dwords() and put_dwords() should be checking the return value from dma_memory_write() and dma_memory_read() so that they fail if the DMA fails... -- PMM
