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

[PATCH v3 0/3] 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.

v3:
 1. Make get_dwords() return bool

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

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

-- 
2.53.0

[PATCH v3 1/3] 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.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.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 451a918e9f..ecf98c4e19 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

[PATCH v3 2/3] hw/usb/hcd-ehci: Make get_dwords() return bool

[Jamin Lin]

get_dwords() returns the number of dwords it read, but every caller only
tests it for failure and none of them uses the count. Return a plain
bool instead, which matches how the function is actually used.

Suggested-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/usb/hcd-ehci.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index ecf98c4e19..5187ecc7e4 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -416,8 +416,8 @@ static inline bool ehci_periodic_enabled(EHCIState *s)
 }
 
 /* Get an array of dwords from main memory */
-static inline int get_dwords(EHCIState *ehci, uint64_t addr,
-                             uint32_t *buf, int num)
+static inline bool get_dwords(EHCIState *ehci, uint64_t addr,
+                              uint32_t *buf, int num)
 {
     int i;
 
@@ -427,12 +427,12 @@ static inline int get_dwords(EHCIState *ehci, uint64_t addr,
             ehci_raise_irq(ehci, USBSTS_HSE);
             ehci->usbcmd &= ~USBCMD_RUNSTOP;
             trace_usb_ehci_dma_error();
-            return -1;
+            return false;
         }
         *buf = le32_to_cpu(*buf);
     }
 
-    return num;
+    return true;
 }
 
 /* Put an array of dwords in to main memory */
@@ -1598,8 +1598,8 @@ static int ehci_state_waitlisthead(EHCIState *ehci,  int async)
     /*  Find the head of the list (4.9.1.1) */
     memset(&qh, 0, sizeof(qh));
     for (i = 0; i < MAX_QH; i++) {
-        if (get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &qh,
-                       ehci_qh_dwords(ehci)) < 0) {
+        if (!get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &qh,
+                        ehci_qh_dwords(ehci))) {
             return 0;
         }
         ehci_trace_qh(NULL, NLPTR_GET(entry), &qh);
@@ -1701,8 +1701,8 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
     }
 
     memset(&qh, 0, sizeof(qh));
-    if (get_dwords(ehci, NLPTR_GET(q->qhaddr),
-                   (uint32_t *) &qh, ehci_qh_dwords(ehci)) < 0) {
+    if (!get_dwords(ehci, NLPTR_GET(q->qhaddr),
+                    (uint32_t *) &qh, ehci_qh_dwords(ehci))) {
         q = NULL;
         goto out;
     }
@@ -1779,8 +1779,8 @@ static int ehci_state_fetchitd(EHCIState *ehci, int async)
     entry = ehci_get_fetch_addr(ehci, async);
 
     memset(&itd, 0, sizeof(itd));
-    if (get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &itd,
-                   ehci_itd_dwords(ehci)) < 0) {
+    if (!get_dwords(ehci, NLPTR_GET(entry), (uint32_t *) &itd,
+                    ehci_itd_dwords(ehci))) {
         return -1;
     }
     ehci_trace_itd(ehci, entry, &itd);
@@ -1805,8 +1805,8 @@ static int ehci_state_fetchsitd(EHCIState *ehci, int async)
     assert(!async);
     entry = ehci_get_fetch_addr(ehci, async);
 
-    if (get_dwords(ehci, NLPTR_GET(entry), (uint32_t *)&sitd,
-                   sizeof(EHCIsitd) >> 2) < 0) {
+    if (!get_dwords(ehci, NLPTR_GET(entry), (uint32_t *)&sitd,
+                    sizeof(EHCIsitd) >> 2)) {
         return 0;
     }
     ehci_trace_sitd(ehci, entry, &sitd);
@@ -1866,18 +1866,18 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     uint64_t addr;
 
     addr = NLPTR_GET(q->qtdaddr);
-    if (get_dwords(q->ehci, addr +  8, &qtd.token,   1) < 0) {
+    if (!get_dwords(q->ehci, addr +  8, &qtd.token,   1)) {
         return 0;
     }
     barrier();
     memset(qtd.bufptr_hi, 0, sizeof(qtd.bufptr_hi));
-    if (get_dwords(q->ehci, addr +  0, &qtd.next,    1) < 0 ||
-        get_dwords(q->ehci, addr +  4, &qtd.altnext, 1) < 0 ||
-        get_dwords(q->ehci, addr + 12, qtd.bufptr,
-                   ARRAY_SIZE(qtd.bufptr)) < 0 ||
+    if (!get_dwords(q->ehci, addr +  0, &qtd.next,    1) ||
+        !get_dwords(q->ehci, addr +  4, &qtd.altnext, 1) ||
+        !get_dwords(q->ehci, addr + 12, qtd.bufptr,
+                    ARRAY_SIZE(qtd.bufptr)) ||
         (q->ehci->caps_64bit_addr &&
-         get_dwords(q->ehci, addr + offsetof(EHCIqtd, bufptr_hi),
-                    qtd.bufptr_hi, ARRAY_SIZE(qtd.bufptr_hi)) < 0)) {
+         !get_dwords(q->ehci, addr + offsetof(EHCIqtd, bufptr_hi),
+                     qtd.bufptr_hi, ARRAY_SIZE(qtd.bufptr_hi)))) {
         return 0;
     }
     ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd);
@@ -1969,8 +1969,8 @@ static int ehci_fill_queue(EHCIPacket *p)
             }
         }
         memset(qtd.bufptr_hi, 0, sizeof(qtd.bufptr_hi));
-        if (get_dwords(q->ehci, NLPTR_GET(qtdaddr),
-                       (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci)) < 0) {
+        if (!get_dwords(q->ehci, NLPTR_GET(qtdaddr),
+                        (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci))) {
             return -1;
         }
         ehci_trace_qtd(q, NLPTR_GET(qtdaddr), &qtd);
@@ -2290,7 +2290,7 @@ static void ehci_advance_periodic_state(EHCIState *ehci)
         }
         list |= ((ehci->frindex & 0x1ff8) >> 1);
         list64 = ehci_get_desc_addr(ehci, list);
-        if (get_dwords(ehci, list64, &entry, 1) < 0) {
+        if (!get_dwords(ehci, list64, &entry, 1)) {
             break;
         }
         entry64 = ehci_get_desc_addr(ehci, entry);
-- 
2.53.0

Re: [PATCH v3 2/3] hw/usb/hcd-ehci: Make get_dwords() return bool

[Philippe Mathieu-Daudé]

On 17/8/26 07:53, Jamin Lin wrote:
> get_dwords() returns the number of dwords it read, but every caller only
> tests it for failure and none of them uses the count. Return a plain
> bool instead, which matches how the function is actually used.
> 
> Suggested-by: Philippe Mathieu-Daudé <philmd@oss.qualcomm.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/usb/hcd-ehci.c | 44 ++++++++++++++++++++++----------------------
>   1 file changed, 22 insertions(+), 22 deletions(-)

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

[PATCH v3 3/3] 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 5187ecc7e4..f371e567f3 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -527,11 +527,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)) ||
+        !get_dwords(q->ehci, NLPTR_GET(q->qtdaddr),
+                    (uint32_t *) &qtd, ehci_qtd_dwords(q->ehci)) ||
+        !ehci_verify_qh(q, &qh) || !ehci_verify_qtd(p, &qtd)) {
         p->async = EHCI_ASYNC_INITIALIZED;
         ehci_free_packet(p);
         return;
-- 
2.53.0