Re: [PATCH] USB: Avoid NULL pointer dereference inusb_disconnect()

[Jesper Juhl]

On Mon, 11 Jul 2011, Alan Stern wrote:

> On Mon, 11 Jul 2011, Jesper Juhl wrote:
> 
> > If, in drivers/usb/core/hub.c::usb_disconnect(), 'udev' is NULL, then
> > we'll never get to the printing of a debug message and returning from
> > the function since we'll have already dereferenced the NULL pointer in
> > the bus_to_hcd(udev->bus) call.
> > This patch moves the dereference to after the test for NULL, thus
> > avoiding that potential crash.
> > 
> > Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>
> > ---
> >  drivers/usb/core/hub.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> >  disclaimer: compile tested only.
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index a428aa0..0a98c76 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -1634,13 +1634,15 @@ void usb_disconnect(struct usb_device **pdev)
> >  {
> >  	struct usb_device	*udev = *pdev;
> >  	int			i;
> > -	struct usb_hcd		*hcd = bus_to_hcd(udev->bus);
> > +	struct usb_hcd		*hcd;
> >  
> >  	if (!udev) {
> >  		pr_debug ("%s nodev\n", __func__);
> >  		return;
> >  	}
> >  
> > +	hcd = bus_to_hcd(udev->bus);
> > +
> >  	/* mark the device as inactive, so any further urb submissions for
> >  	 * this device (and any of its children) will fail immediately.
> >  	 * this quiesces everything except pending urbs.
> 
> This is okay, but it might be better just to remove the NULL test 
> entirely.  Code inspection easily shows that it can never be triggered.
> 
I must admit I don't care much one way or the other. Moving the 
dereference seemed like the best thing to do to me - defensive in the 
face of future changes + anything that I possibly overlooked that could 
cause the pointer to actually be NULL. 
But I'm just as fine with just removing the NULL test and associated debug 
print if that seems the best solution to others.

Here's a patch to remove the test. Now you have both, pick whichever you 
prefer :-)


From: Jesper Juhl <jj@xxxxxxxxxxxxx>
Subject: [PATCH] USB: Remove test for NULL that'll never happen in usb_disconnect()

In drivers/usb/core/hub.c::usb_disconnect(), 'udev' will never be
NULL, so remove the test and printing of debug message.

Signed-off-by: Jesper Juhl <jj@xxxxxxxxxxxxx>
---
 drivers/usb/core/hub.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index a428aa0..99fff6b 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1636,11 +1636,6 @@ void usb_disconnect(struct usb_device **pdev)
 	int			i;
 	struct usb_hcd		*hcd = bus_to_hcd(udev->bus);
 
-	if (!udev) {
-		pr_debug ("%s nodev\n", __func__);
-		return;
-	}
-
 	/* mark the device as inactive, so any further urb submissions for
 	 * this device (and any of its children) will fail immediately.
 	 * this quiesces everything except pending urbs.
-- 
1.7.6


-- 
Jesper Juhl <jj@xxxxxxxxxxxxx>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/