[Qemu-devel] [PATCH 0/2] USB static analysis patches

[Qemu-devel] [PATCH 0/2] USB static analysis patches

[Liam Merwick]

These two patches are a result of running the Parfait static analysis
tool on QEMU.  The first patch is an optimisation to move an assignment
after a test and the second patch handles a number of potential NULL
pointer dereferences if usb_ep_get() returns NULL.

Liam Merwick (2):
  usb: rearrange usb_ep_get()
  usb: deal with potential Null pointer returned by usb_ep_get()

 hw/usb/core.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] 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 and the resulting pointer also
validated before returning an entry indexed on 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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index 241ae66b1505..1aa0051b2b2d 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -720,12 +720,13 @@ 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;
+    assert(eps != NULL);
     return eps + ep - 1;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] usb: rearrange usb_ep_get()

[Gerd Hoffmann]

On Wed, Jan 30, 2019 at 02:37:01PM +0000, Liam Merwick wrote:
> 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 and the resulting pointer also
> validated before returning an entry indexed on 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 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index 241ae66b1505..1aa0051b2b2d 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -720,12 +720,13 @@ 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;
> +    assert(eps != NULL);

That assert is rather pointless.  It's impossible for eps to be NULL at
this point.

cheers,
  Gerd

[Qemu-devel] [PATCH 2/2] usb: deal with potential Null pointer returned by usb_ep_get()

[Liam Merwick]

From: Liam Merwick <Liam.Merwick@oracle.com>

usb_ep_get() can return a Null pointer in the (albeit unlikely) case
that a NULL USBDevice is passed in via the 'dev' parameter.  Before
dereferencing the return value from usb_ep_get(), check its validity
and use a default (invalid) value where needed.

Reported by the Parfait static code analysis tool

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 | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/hw/usb/core.c b/hw/usb/core.c
index 1aa0051b2b2d..9d46a4b41e99 100644
--- a/hw/usb/core.c
+++ b/hw/usb/core.c
@@ -733,19 +733,23 @@ struct USBEndpoint *usb_ep_get(USBDevice *dev, int pid, int ep)
 uint8_t usb_ep_get_type(USBDevice *dev, int pid, int ep)
 {
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
-    return uep->type;
+    return uep ? uep->type : USB_ENDPOINT_XFER_INVALID;
 }
 
 void usb_ep_set_type(USBDevice *dev, int pid, int ep, uint8_t type)
 {
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
-    uep->type = type;
+    if (uep) {
+        uep->type = type;
+    }
 }
 
 void usb_ep_set_ifnum(USBDevice *dev, int pid, int ep, uint8_t ifnum)
 {
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
-    uep->ifnum = ifnum;
+    if (uep) {
+        uep->ifnum = ifnum;
+    }
 }
 
 void usb_ep_set_max_packet_size(USBDevice *dev, int pid, int ep,
@@ -754,6 +758,10 @@ void usb_ep_set_max_packet_size(USBDevice *dev, int pid, int ep,
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
     int size, microframes;
 
+    if (!uep) {
+        return;
+    }
+
     size = raw & 0x7ff;
     switch ((raw >> 11) & 3) {
     case 1:
@@ -774,6 +782,10 @@ void usb_ep_set_max_streams(USBDevice *dev, int pid, int ep, uint8_t raw)
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
     int MaxStreams;
 
+    if (!uep) {
+        return;
+    }
+
     MaxStreams = raw & 0x1f;
     if (MaxStreams) {
         uep->max_streams = 1 << MaxStreams;
@@ -785,7 +797,9 @@ void usb_ep_set_max_streams(USBDevice *dev, int pid, int ep, uint8_t raw)
 void usb_ep_set_halted(USBDevice *dev, int pid, int ep, bool halted)
 {
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
-    uep->halted = halted;
+    if (uep) {
+        uep->halted = halted;
+    }
 }
 
 USBPacket *usb_ep_find_packet_by_id(USBDevice *dev, int pid, int ep,
@@ -794,6 +808,10 @@ USBPacket *usb_ep_find_packet_by_id(USBDevice *dev, int pid, int ep,
     struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
     USBPacket *p;
 
+    if (!uep) {
+        return NULL;
+    }
+
     QTAILQ_FOREACH(p, &uep->queue, queue) {
         if (p->id == id) {
             return p;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] usb: deal with potential Null pointer returned by usb_ep_get()

[Gerd Hoffmann]

On Wed, Jan 30, 2019 at 02:37:02PM +0000, Liam Merwick wrote:
> From: Liam Merwick <Liam.Merwick@oracle.com>
> 
> usb_ep_get() can return a Null pointer in the (albeit unlikely) case
> that a NULL USBDevice is passed in via the 'dev' parameter.

That should never ever happen.

> Reported by the Parfait static code analysis tool

Try add "assert(dev != NULL)" to usb_ep_get() instead of sprinkling
pointless checks all over the place.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] usb: deal with potential Null pointer returned by usb_ep_get()

[Liam Merwick]

On 31/01/2019 08:03, Gerd Hoffmann wrote:
> On Wed, Jan 30, 2019 at 02:37:02PM +0000, Liam Merwick wrote:
>> From: Liam Merwick <Liam.Merwick@oracle.com>
>>
>> usb_ep_get() can return a Null pointer in the (albeit unlikely) case
>> that a NULL USBDevice is passed in via the 'dev' parameter.
> That should never ever happen.
>
>> Reported by the Parfait static code analysis tool
> Try add "assert(dev != NULL)" to usb_ep_get() instead of sprinkling
> pointless checks all over the place.
>
Adding "assert(dev != NULL)" to usb_ep_get() isn't sufficient for that 
tool unless the 'if (dev== NULL)' check is removed which seems a 
backwards step even if that NULL USBDevice case is impossible.

Adding an assert like below in 7 places in hw/usb/core.c that call 
usb_ep_get() would resolve it but would that  pass coding conventions 
(checkpatch.pl seems OK with it)?

  uint8_t usb_ep_get_type(USBDevice *dev, int pid, int ep)
  {
+    assert(dev);
      struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
      return uep->type;
  }

(the other option below it seems like too much code churn).


  uint8_t usb_ep_get_type(USBDevice *dev, int pid, int ep)
  {
-    struct USBEndpoint *uep = usb_ep_get(dev, pid, ep);
+    struct USBEndpoint *uep;
+    assert(dev);
+    uep = usb_ep_get(dev, pid, ep);
      return uep->type;
  }


So that's kinda a long way of saying I'll drop this patch unless someone 
thinks it still adds a benefit and will send a v2 with a modified Patch1 
and a patch that adds two asserts to hw/usb/hcd-xhci.c

Regards,

Liam

Re: [Qemu-devel] [PATCH 2/2] usb: deal with potential Null pointer returned by usb_ep_get()

[Gerd Hoffmann]

On Mon, Feb 04, 2019 at 11:50:33AM +0000, Liam Merwick wrote:
> On 31/01/2019 08:03, Gerd Hoffmann wrote:
> > On Wed, Jan 30, 2019 at 02:37:02PM +0000, Liam Merwick wrote:
> > > From: Liam Merwick <Liam.Merwick@oracle.com>
> > > 
> > > usb_ep_get() can return a Null pointer in the (albeit unlikely) case
> > > that a NULL USBDevice is passed in via the 'dev' parameter.
> > That should never ever happen.
> > 
> > > Reported by the Parfait static code analysis tool
> > Try add "assert(dev != NULL)" to usb_ep_get() instead of sprinkling
> > pointless checks all over the place.
> > 
> Adding "assert(dev != NULL)" to usb_ep_get() isn't sufficient for that tool
> unless the 'if (dev== NULL)' check is removed which seems a backwards step
> even if that NULL USBDevice case is impossible.

Looked at the code again.

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.

cheers,
  Gerd