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
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
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
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
© 2016 - 2025 Red Hat, Inc.