[PATCH v2 0/2] hw/usb/hcd-ehci: Fix Coverity CID 1685236 and check DMA errors

[PATCH v2 0/2] hw/usb/hcd-ehci: Fix Coverity CID 1685236 and check DMA errors

[Jamin Lin]

v1:
 1. Fix Fix Coverity CID 1685236
 
v2:
 1. Drops the dead NULL test and checks the actual accesses instead, and makes put_dwords() return void
    since no caller can act on a failed writeback.

Jamin Lin (2):
  hw/usb/hcd-ehci: Handle get_dwords() failures in async writeback
  hw/usb/hcd-ehci: Check for DMA errors in get_dwords()/put_dwords()

 hw/usb/hcd-ehci.c | 48 +++++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] hw/usb/hcd-ehci: Handle get_dwords() failures in async writeback

[Jamin Lin]

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>
Resolves: Coverity CID 1685236
Fixes: 2b3de6ada5d ("ehci: writeback_async_complete_packet: verify qh and qtd")
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.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.53.0

[PATCH v2 2/2] hw/usb/hcd-ehci: Check for DMA errors in get_dwords()/put_dwords()

[Jamin Lin]

get_dwords() and put_dwords() return -1 when ehci->as is NULL, but that
can no longer happen. The sysbus variants set it in instance_init
(ehci_sysbus_init()), the PCI variant sets it in realize
(usb_ehci_pci_realize()), and usb_ehci_pci_write_config() only switches
between the bus master address space and address_space_memory.

Meanwhile the results of dma_memory_read() and dma_memory_write() are
ignored, so a failed guest memory access is silently treated as success.

Drop the dead NULL test and check the MemTxResult of each access
instead, so that the existing -1 error path reports real DMA errors:
raise USBSTS_HSE, clear USBCMD_RUNSTOP and stop processing the
descriptor, as the NULL path used to do. All 11 get_dwords() callers
already check the return value.

put_dwords() now returns void: none of its four callers (ehci_flush_qh(),
ehci_state_fetchitd() and twice in ehci_state_writeback()) looks at the
status.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/hcd-ehci.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index d43951975a..daab5783bc 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -421,16 +421,14 @@ static inline int get_dwords(EHCIState *ehci, uint64_t addr,
 {
     int i;
 
-    if (!ehci->as) {
-        ehci_raise_irq(ehci, USBSTS_HSE);
-        ehci->usbcmd &= ~USBCMD_RUNSTOP;
-        trace_usb_ehci_dma_error();
-        return -1;
-    }
-
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
-        dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
-                        MEMTXATTRS_UNSPECIFIED);
+        if (dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
+                            MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+            ehci_raise_irq(ehci, USBSTS_HSE);
+            ehci->usbcmd &= ~USBCMD_RUNSTOP;
+            trace_usb_ehci_dma_error();
+            return -1;
+        }
         *buf = le32_to_cpu(*buf);
     }
 
@@ -438,25 +436,21 @@ static inline int get_dwords(EHCIState *ehci, uint64_t addr,
 }
 
 /* Put an array of dwords in to main memory */
-static inline int put_dwords(EHCIState *ehci, uint64_t addr,
-                             uint32_t *buf, int num)
+static inline void put_dwords(EHCIState *ehci, uint64_t addr,
+                              uint32_t *buf, int num)
 {
     int i;
 
-    if (!ehci->as) {
-        ehci_raise_irq(ehci, USBSTS_HSE);
-        ehci->usbcmd &= ~USBCMD_RUNSTOP;
-        trace_usb_ehci_dma_error();
-        return -1;
-    }
-
     for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
         uint32_t tmp = cpu_to_le32(*buf);
-        dma_memory_write(ehci->as, addr, &tmp, sizeof(tmp),
-                         MEMTXATTRS_UNSPECIFIED);
+        if (dma_memory_write(ehci->as, addr, &tmp, sizeof(tmp),
+                             MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
+            ehci_raise_irq(ehci, USBSTS_HSE);
+            ehci->usbcmd &= ~USBCMD_RUNSTOP;
+            trace_usb_ehci_dma_error();
+            return;
+        }
     }
-
-    return num;
 }
 
 static int ehci_get_pid(EHCIqtd *qtd)
-- 
2.53.0

Re: [PATCH v2 2/2] hw/usb/hcd-ehci: Check for DMA errors in get_dwords()/put_dwords()

[Philippe Mathieu-Daudé]

On 17/8/26 04:36, Jamin Lin wrote:
> get_dwords() and put_dwords() return -1 when ehci->as is NULL, but that
> can no longer happen. The sysbus variants set it in instance_init
> (ehci_sysbus_init()), the PCI variant sets it in realize
> (usb_ehci_pci_realize()), and usb_ehci_pci_write_config() only switches
> between the bus master address space and address_space_memory.
> 
> Meanwhile the results of dma_memory_read() and dma_memory_write() are
> ignored, so a failed guest memory access is silently treated as success.
> 
> Drop the dead NULL test and check the MemTxResult of each access
> instead, so that the existing -1 error path reports real DMA errors:
> raise USBSTS_HSE, clear USBCMD_RUNSTOP and stop processing the
> descriptor, as the NULL path used to do. All 11 get_dwords() callers
> already check the return value.
> 
> put_dwords() now returns void: none of its four callers (ehci_flush_qh(),
> ehci_state_fetchitd() and twice in ehci_state_writeback()) looks at the
> status.
> 

Suggested-by: Peter Maydell <peter.maydell@linaro.org>

> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/hcd-ehci.c | 38 ++++++++++++++++----------------------
>   1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index d43951975a..daab5783bc 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -421,16 +421,14 @@ static inline int get_dwords(EHCIState *ehci, uint64_t addr,
>   {
>       int i;
>   
> -    if (!ehci->as) {
> -        ehci_raise_irq(ehci, USBSTS_HSE);
> -        ehci->usbcmd &= ~USBCMD_RUNSTOP;
> -        trace_usb_ehci_dma_error();
> -        return -1;
> -    }
> -
>       for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
> -        dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
> -                        MEMTXATTRS_UNSPECIFIED);
> +        if (dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
> +                            MEMTXATTRS_UNSPECIFIED) != MEMTX_OK) {
> +            ehci_raise_irq(ehci, USBSTS_HSE);
> +            ehci->usbcmd &= ~USBCMD_RUNSTOP;
> +            trace_usb_ehci_dma_error();
> +            return -1;
> +        }
>           *buf = le32_to_cpu(*buf);
>       }

Maybe reorder before previous patch?

Note, for EHCI get_dwords() is only used as returning a boolean,
which would match with your conversion of put_dwords to void. While
the change involves some code churn, the result would be clearer
IMHO. Anyway can be done later.

Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>

RE: [PATCH v2 2/2] hw/usb/hcd-ehci: Check for DMA errors in get_dwords()/put_dwords()

[Jamin Lin]

Hi Philippe

> Subject: Re: [PATCH v2 2/2] hw/usb/hcd-ehci: Check for DMA errors in
> get_dwords()/put_dwords()
> 
> On 17/8/26 04:36, Jamin Lin wrote:
> > get_dwords() and put_dwords() return -1 when ehci->as is NULL, but
> > that can no longer happen. The sysbus variants set it in instance_init
> > (ehci_sysbus_init()), the PCI variant sets it in realize
> > (usb_ehci_pci_realize()), and usb_ehci_pci_write_config() only
> > switches between the bus master address space and
> address_space_memory.
> >
> > Meanwhile the results of dma_memory_read() and dma_memory_write()
> are
> > ignored, so a failed guest memory access is silently treated as success.
> >
> > Drop the dead NULL test and check the MemTxResult of each access
> > instead, so that the existing -1 error path reports real DMA errors:
> > raise USBSTS_HSE, clear USBCMD_RUNSTOP and stop processing the
> > descriptor, as the NULL path used to do. All 11 get_dwords() callers
> > already check the return value.
> >
> > put_dwords() now returns void: none of its four callers
> > (ehci_flush_qh(),
> > ehci_state_fetchitd() and twice in ehci_state_writeback()) looks at
> > the status.
> >
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> 
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/usb/hcd-ehci.c | 38 ++++++++++++++++----------------------
> >   1 file changed, 16 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index
> > d43951975a..daab5783bc 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -421,16 +421,14 @@ static inline int get_dwords(EHCIState *ehci,
> uint64_t addr,
> >   {
> >       int i;
> >
> > -    if (!ehci->as) {
> > -        ehci_raise_irq(ehci, USBSTS_HSE);
> > -        ehci->usbcmd &= ~USBCMD_RUNSTOP;
> > -        trace_usb_ehci_dma_error();
> > -        return -1;
> > -    }
> > -
> >       for (i = 0; i < num; i++, buf++, addr += sizeof(*buf)) {
> > -        dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
> > -                        MEMTXATTRS_UNSPECIFIED);
> > +        if (dma_memory_read(ehci->as, addr, buf, sizeof(*buf),
> > +                            MEMTXATTRS_UNSPECIFIED) !=
> MEMTX_OK) {
> > +            ehci_raise_irq(ehci, USBSTS_HSE);
> > +            ehci->usbcmd &= ~USBCMD_RUNSTOP;
> > +            trace_usb_ehci_dma_error();
> > +            return -1;
> > +        }
> >           *buf = le32_to_cpu(*buf);
> >       }
> 
> Maybe reorder before previous patch?
> 
Will resend v3.

> Note, for EHCI get_dwords() is only used as returning a boolean, which would
> match with your conversion of put_dwords to void. While the change involves
> some code churn, the result would be clearer IMHO. Anyway can be done later.
> 
Will add new patch in v3 and change get_dwords return type to a boolean.
Thanks,
Jamin

> Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>