* [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect()
@ 2011-07-11 9:21 Jesper Juhl
2011-07-11 9:36 ` wangyanqing
2011-07-11 15:01 ` Alan Stern
0 siblings, 2 replies; 6+ messages in thread
From: Jesper Juhl @ 2011-07-11 9:21 UTC (permalink / raw)
To: linux-kernel
Cc: linux-usb, Greg Kroah-Hartman, Sarah Sharp, Alan Stern,
Andiry Xu, Luben Tuikov
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@chaosbits.net>
---
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.
--
1.7.6
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect()
2011-07-11 9:21 [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect() Jesper Juhl
@ 2011-07-11 9:36 ` wangyanqing
2011-07-11 9:47 ` Jesper Juhl
2011-07-11 15:01 ` Alan Stern
1 sibling, 1 reply; 6+ messages in thread
From: wangyanqing @ 2011-07-11 9:36 UTC (permalink / raw)
To: Jesper Juhl; +Cc: linux-kernel
On Mon, Jul 11, 2011 at 11:21:18AM +0200, 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@chaosbits.net>
> ---
> 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.
> --
> 1.7.6
>
>
> --
> Jesper Juhl <jj@chaosbits.net> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Hello,All.Just a question,should we add the NULL checker code at everywhere that
dereference pointer in kernel? If so,it will scare me.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect()
2011-07-11 9:36 ` wangyanqing
@ 2011-07-11 9:47 ` Jesper Juhl
0 siblings, 0 replies; 6+ messages in thread
From: Jesper Juhl @ 2011-07-11 9:47 UTC (permalink / raw)
To: wangyanqing; +Cc: linux-kernel
On Mon, 11 Jul 2011, wangyanqing wrote:
> On Mon, Jul 11, 2011 at 11:21:18AM +0200, 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@chaosbits.net>
> > ---
> > 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.
> > --
> > 1.7.6
> >
> >
> Hello,All.Just a question,should we add the NULL checker code at everywhere that
> dereference pointer in kernel? If so,it will scare me.
>
Of course not.
But here there is already an explicit test for NULL which implies that
this pointer can in fact be NULL, which is why it seems unsafe to
dereference it before that check. Thus I moved the dereference.
If someone can prove that the pointer will never be NULL, then I'd be fine
with just removing the NULL test and debug print instead.
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect()
2011-07-11 9:21 [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect() Jesper Juhl
2011-07-11 9:36 ` wangyanqing
@ 2011-07-11 15:01 ` Alan Stern
2011-07-11 20:59 ` Jesper Juhl
1 sibling, 1 reply; 6+ messages in thread
From: Alan Stern @ 2011-07-11 15:01 UTC (permalink / raw)
To: Jesper Juhl
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Sarah Sharp,
Andiry Xu, Luben Tuikov
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@chaosbits.net>
> ---
> 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.
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect()
2011-07-11 15:01 ` Alan Stern
@ 2011-07-11 20:59 ` Jesper Juhl
2011-07-12 0:50 ` Alan Stern
0 siblings, 1 reply; 6+ messages in thread
From: Jesper Juhl @ 2011-07-11 20:59 UTC (permalink / raw)
To: Alan Stern
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Sarah Sharp,
Andiry Xu, Luben Tuikov
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@chaosbits.net>
> > ---
> > 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@chaosbits.net>
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@chaosbits.net>
---
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@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect()
2011-07-11 20:59 ` Jesper Juhl
@ 2011-07-12 0:50 ` Alan Stern
0 siblings, 0 replies; 6+ messages in thread
From: Alan Stern @ 2011-07-12 0:50 UTC (permalink / raw)
To: Jesper Juhl
Cc: linux-kernel, linux-usb, Greg Kroah-Hartman, Sarah Sharp,
Andiry Xu, Luben Tuikov
On Mon, 11 Jul 2011, Jesper Juhl wrote:
> > 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 :-)
Given a choice, I prefer this one. :-)
> From: Jesper Juhl <jj@chaosbits.net>
> 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@chaosbits.net>
> ---
> 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.
Acked-by: Alan Stern <stern@rowland.harvard.edu>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-12 0:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-11 9:21 [PATCH] USB: Avoid NULL pointer dereference in usb_disconnect() Jesper Juhl
2011-07-11 9:36 ` wangyanqing
2011-07-11 9:47 ` Jesper Juhl
2011-07-11 15:01 ` Alan Stern
2011-07-11 20:59 ` Jesper Juhl
2011-07-12 0:50 ` Alan Stern
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®