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 <jamin_lin@aspeedtech.com>
---
 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;
-- 
2.43.0

On Thu, 13 Aug 2026 at 08:24, Jamin Lin <jamin_lin@aspeedtech.com> 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 <jamin_lin@aspeedtech.com>
> ---
>  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

On 14/8/26 11:06, Peter Maydell wrote:
> On Thu, 13 Aug 2026 at 08:24, Jamin Lin <jamin_lin@aspeedtech.com> 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 <jamin_lin@aspeedtech.com>
>> ---
>>   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...

Oh good point, I missed that. We really should qualify
dma_memory_write() & co with G_GNUC_WARN_UNUSED_RESULT, that'd
help us preventing such mistakes.

Hi Peter,

> > 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...
> 

Thanks for the review and the suggestion.

I have sent a v2 that drops the dead NULL check, checks the MemTxResult
of dma_memory_read()/dma_memory_write() instead, and makes put_dwords()
return void since no caller can act on a failed writeback.

https://patchwork.kernel.org/project/qemu-devel/list/?series=1146899 

Thanks,
Jamin

> -- PMM

On 13/8/26 09:23, Jamin Lin 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 <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/hcd-ehci.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)

Fixes: 2b3de6ada5d ("ehci: writeback_async_complete_packet: verify qh 
and qtd")
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

+ Cédric Le Goater <clg@kaod.org>

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> Sent: Thursday, August 13, 2026 5:43 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; clg@redhat.com; Philippe
> Mathieu-Daudé <philmd@mailo.com>; Paolo Bonzini <pbonzini@redhat.com>;
> open list:All patches CC here <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v1] hw/usb/hcd-ehci: Handle get_dwords() failures in async
> writeback
> 
> On 13/8/26 09:23, Jamin Lin 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 <jamin_lin@aspeedtech.com>
> > ---
> >   hw/usb/hcd-ehci.c | 10 +++++-----
> >   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> Fixes: 2b3de6ada5d ("ehci: writeback_async_complete_packet: verify qh and
> qtd")
> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

Thanks for adding the Fixes tag.
I’ve also added the Resolves tag.

Resolves: Coverity CID 1685236

Jamin