[Qemu-devel] [PATCH v2 0/9] Dealing with Null devices in usb_ep_get()

[Qemu-devel] [PATCH v2 0/9] Dealing with Null devices in usb_ep_get()

[Liam Merwick]

This series is in response to feedback from Gerd Hoffman (below) on 
<1548859022-3969-1-git-send-email-liam.merwick@oracle.com>

Instead of checking if usb_ep_get() returns NULL, ensure it never is passed
a NULL device.

===

The usb device emulation (hw/usb/dev-*.c) never ever calls usb_ep_get()
with dev == NULL.  There are some places in usb host adapter emulation
(hw/usb/hcd-*) which might do this.  uhci for example has this ...

	[ ... ]
        USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
        USBEndpoint *ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);

        if (ep == NULL) {
	[ ... ]

... and uhci_find_device can return NULL.

So, I'd suggest to check all usb_ep_get() callers, fix them if needed,
then remove the 'if (dev== NULL)' check in usb_ep_get() and add the
assert() instead.

===

Passes QEMU 'make check' and kvm-unit-tests


Liam Merwick (9):
  usb: rearrange usb_ep_get()
  xhci: add asserts to help with static code analysis
  xhci: check device is not NULL before calling usb_ep_get()
  ehci: check device is not NULL before calling usb_ep_get()
  ohci: check device is not NULL before calling usb_ep_get()
  uhci: check device is not NULL before calling usb_ep_get()
  usb: check device is not NULL before calling usb_ep_get()
  usb: add device checks before redirector calls to usb_ep_get()
  usb: remove unnecessary NULL device check from usb_ep_get()

 hw/usb/core.c     | 6 ++----
 hw/usb/hcd-ehci.c | 7 +++++--
 hw/usb/hcd-musb.c | 8 ++++----
 hw/usb/hcd-ohci.c | 8 ++++++++
 hw/usb/hcd-uhci.c | 8 +++++---
 hw/usb/hcd-xhci.c | 6 ++++--
 hw/usb/redirect.c | 3 ++-
 7 files changed, 30 insertions(+), 16 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/9] usb: rearrange usb_ep_get()

[Liam Merwick]

There is no need to calculate the 'eps' variable in usb_ep_get()
if 'ep' is the control endpoint.  Instead the calculation should
be done after validating the input before returning an entry
indexed by the endpoint 'ep'.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
Reviewed-by: Darren Kenny <Darren.Kenny@oracle.com>
Reviewed-by: Mark Kanda <Mark.Kanda@oracle.com>
Reviewed-by: Ameya More <ameya.more@oracle.com>
---
 hw/usb/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index 241ae66b1505..bfb7ae67bbf0 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -720,12 +720,12 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep)
     if (dev == NULL) {
         return NULL;
     }
-    eps = (pid == USB_TOKEN_IN) ? dev->ep_in : dev->ep_out;
     if (ep == 0) {
         return &dev->ep_ctl;
     }
     assert(pid == USB_TOKEN_IN || pid == USB_TOKEN_OUT);
     assert(ep > 0 && ep <= USB_MAX_ENDPOINTS);
+    eps = (pid == USB_TOKEN_IN) ? dev->ep_in : dev->ep_out;
     return eps + ep - 1;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/9] xhci: add asserts to help with static code analysis

[Liam Merwick]

Most callers of xhci_port_update() and xhci_wakeup() pass in a pointer
to an array entry and can never be NULL but add two defensive asserts
to protect against future changes (e.g. adding a new port speed, etc.)
adding a path through xhci_lookup_port() that could result in the
return of a NULL XHCIPort.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/hcd-xhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 19c64f7ff421..99b83aaa9ed5 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2607,6 +2607,7 @@ static void xhci_port_update(XHCIPort *port, int is_detach)
 {
     uint32_t pls = PLS_RX_DETECT;
 
+    assert(port);
     port->portsc = PORTSC_PP;
     if (!is_detach && xhci_port_have_device(port)) {
         port->portsc |= PORTSC_CCS;
@@ -3215,6 +3216,7 @@ static void xhci_wakeup(USBPort *usbport)
     XHCIState *xhci = usbport->opaque;
     XHCIPort *port = xhci_lookup_port(xhci, usbport);
 
+    assert(port);
     if (get_field(port->portsc, PORTSC_PLS) != PLS_U3) {
         return;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/9] xhci: check device is not NULL before calling usb_ep_get()

[Liam Merwick]

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/hcd-xhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 99b83aaa9ed5..ec28bee31963 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3276,10 +3276,10 @@ static USBEndpoint *xhci_epid_to_usbep(XHCIEPContext *epctx)
         return NULL;
     }
     uport = epctx->xhci->slots[epctx->slotid - 1].uport;
-    token = (epctx->epid & 1) ? USB_TOKEN_IN : USB_TOKEN_OUT;
-    if (!uport) {
+    if (!uport || !uport->dev) {
         return NULL;
     }
+    token = (epctx->epid & 1) ? USB_TOKEN_IN : USB_TOKEN_OUT;
     return usb_ep_get(uport->dev, token, epctx->epid >> 1);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/9] ehci: check device is not NULL before calling usb_ep_get()

[Liam Merwick]

In ehci_process_itd(), the call to ehci_find_device() can return NULL
if it doesn't find a device matching 'devaddr' so explicitly check
the return value before passing it to usb_ep_get().

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/hcd-ehci.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 9b132cb0d392..62dab0592fa2 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1439,9 +1439,12 @@ static int ehci_process_itd(EHCIState *ehci,
                 qemu_sglist_add(&ehci->isgl, ptr1 + off, len);
             }
 
-            pid = dir ? USB_TOKEN_IN : USB_TOKEN_OUT;
-
             dev = ehci_find_device(ehci, devaddr);
+            if (dev == NULL) {
+                ehci_trace_guest_bug(ehci, "no device found");
+                return -1;
+            }
+            pid = dir ? USB_TOKEN_IN : USB_TOKEN_OUT;
             ep = usb_ep_get(dev, pid, endp);
             if (ep && ep->type == USB_ENDPOINT_XFER_ISOC) {
                 usb_packet_setup(&ehci->ipacket, pid, ep, 0, addr, false,
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/9] ohci: check device is not NULL before calling usb_ep_get()

[Liam Merwick]

A call to ohci_find_device() can return NULL if it doesn't find a
device matching 'addr' so for the two callers, explicitly check
the return value before passing it to usb_ep_get().

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/hcd-ohci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index c34cf5b73a3a..196a9f72002d 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -848,6 +848,10 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
         bool int_req = relative_frame_number == frame_count &&
                        OHCI_BM(iso_td.flags, TD_DI) == 0;
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
+        if (dev == NULL) {
+            trace_usb_ohci_td_dev_error();
+            return 1;
+        }
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
         usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, false, int_req);
         usb_packet_addbuf(&ohci->usb_packet, ohci->usb_buf, len);
@@ -1071,6 +1075,10 @@ static int ohci_service_td(OHCIState *ohci, struct ohci_ed *ed)
             return 1;
         }
         dev = ohci_find_device(ohci, OHCI_BM(ed->flags, ED_FA));
+        if (dev == NULL) {
+            trace_usb_ohci_td_dev_error();
+            return 1;
+        }
         ep = usb_ep_get(dev, pid, OHCI_BM(ed->flags, ED_EN));
         usb_packet_setup(&ohci->usb_packet, pid, ep, 0, addr, !flag_r,
                          OHCI_BM(td.flags, TD_DI) == 0);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 6/9] uhci: check device is not NULL before calling usb_ep_get()

[Liam Merwick]

In uhci_handle_td(), the call to ehci_find_device() can return NULL
if it doesn't find a device matching 'addr' so explicitly check
the return value before passing it to usb_ep_get().

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/hcd-uhci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index e694b62086d3..09df29ff9c16 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -858,13 +858,15 @@ static int uhci_handle_td(UHCIState *s, UHCIQueue *q, uint32_t qh_addr,
 
     /* Allocate new packet */
     if (q == NULL) {
-        USBDevice *dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
-        USBEndpoint *ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
+        USBDevice *dev;
+        USBEndpoint *ep;
 
-        if (ep == NULL) {
+        dev = uhci_find_device(s, (td->token >> 8) & 0x7f);
+        if (dev == NULL) {
             return uhci_handle_td_error(s, td, td_addr, USB_RET_NODEV,
                                         int_mask);
         }
+        ep = usb_ep_get(dev, pid, (td->token >> 15) & 0xf);
         q = uhci_queue_new(s, qh_addr, td, ep);
     }
     async = uhci_async_alloc(q, td_addr);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 7/9] usb: check device is not NULL before calling usb_ep_get()

[Liam Merwick]

In musb_packet(), the call to usb_find_device() can return NULL
if it doesn't find a device matching 'addr' so explicitly check
the return value before passing it to usb_ep_get().  This then
allows the subsequent calculation of 'id' to be streamlined.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/hcd-musb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index d70a91a58ce2..85d779655498 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -628,11 +628,11 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
 
     /* A wild guess on the FADDR semantics... */
     dev = usb_find_device(&s->port, ep->faddr[idx]);
-    uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
-    id = pid;
-    if (uep) {
-        id |= (dev->addr << 16) | (uep->nr << 8);
+    if (dev == NULL) {
+        return;
     }
+    uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
+    id = pid | (dev->addr << 16) | (uep->nr << 8);
     usb_packet_setup(&ep->packey[dir].p, pid, uep, 0, id, false, true);
     usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
     ep->packey[dir].ep = ep;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 8/9] usb: add device checks before redirector calls to usb_ep_get()

[Liam Merwick]

Add an assert and an explicit check before the two callers to
usb_ep_get() in the USB redirector code to ensure the device
passed in is not NULL.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/redirect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 18a42d19380e..7cb6b120d4f0 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -1728,6 +1728,7 @@ static void usbredir_ep_info(void *priv,
     USBRedirDevice *dev = priv;
     int i;
 
+    assert(dev != NULL);
     for (i = 0; i < MAX_ENDPOINTS; i++) {
         dev->endpoint[i].type = ep_info->type[i];
         dev->endpoint[i].interval = ep_info->interval[i];
@@ -2125,7 +2126,7 @@ static int usbredir_post_load(void *priv, int version_id)
 {
     USBRedirDevice *dev = priv;
 
-    if (dev->parser == NULL) {
+    if (dev == NULL || dev->parser == NULL) {
         return 0;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 9/9] usb: remove unnecessary NULL device check from usb_ep_get()

[Liam Merwick]

No caller of usb_ep_get() calls it with a NULL device (previous commits
have addressed the few remaining cases which didn't explicitly check).
Replace check for 'dev == NULL' with an assert instead.

Signed-off-by: Liam Merwick <liam.merwick@oracle.com>
---
 hw/usb/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index bfb7ae67bbf0..8fbd9c7d573b 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -717,9 +717,7 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep)
 {
     struct USBEndpoint *eps;
 
-    if (dev == NULL) {
-        return NULL;
-    }
+    assert(dev != NULL);
     if (ep == 0) {
         return &dev->ep_ctl;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 0/9] Dealing with Null devices in usb_ep_get()

[Gerd Hoffmann]

On Wed, Feb 06, 2019 at 01:36:47PM +0000, Liam Merwick wrote:
> This series is in response to feedback from Gerd Hoffman (below) on 
> <1548859022-3969-1-git-send-email-liam.merwick@oracle.com>
> 
> Instead of checking if usb_ep_get() returns NULL, ensure it never is passed
> a NULL device.

Added all to usb queue.

thanks,
  Gerd