[PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.

[PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.

[Dr. H. Nikolaus Schaller]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

fixed a potential NULL pointer dereference.
    
    Rationale:
    this is the only location in the musb driver where the
    otg->gadget pointer is dereferenced. Assuming that it
    is never NULL is not only potentially unsafe but was
    observed in the wild on a GTA04 (OMAP3/TPS65950 based
    board) when trying to boot a device tree based 3.14-rc2
    kernel with USB cable plugged in.
    
    DT boot appears to modify the order in which components
    (gadget driver) are loaded and linked and therefore
    an early musb interrupt triggers with a NULL gadget
    pointer ending in a kernel panic.
    
    Since a non-existing gadget can never be "active" we
    simply use a 0 value for musb->is_active.
    
    Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>



[-- Attachment #2: 0001-fixed-a-potential-NULL-pointer-dereference.patch --]
[-- Type: application/octet-stream, Size: 1635 bytes --]

From fecccbfe82a170c81dfb1820810d587def811f54 Mon Sep 17 00:00:00 2001
From: "H. Nikolaus Schaller" <hns@goldelico.com>
Date: Wed, 12 Feb 2014 10:35:21 +0100
Subject: [PATCH] fixed a potential NULL pointer dereference.

Rationale:
this is the only location in the musb driver where the
otg->gadget pointer is dereferenced. Assuming that it
is never NULL is not only potentially unsafe but was
observed in the wild on a GTA04 (OMAP3/TPS65950 based
board) when trying to boot a device tree based 3.14-rc2
kernel with USB cable plugged in.

DT boot appears to modify the order in which components
(gadget driver) are loaded and linked and therefore
an early musb interrupt triggers with a NULL gadget
pointer ending in a kernel panic.

Since a non-existing gadget can never be "active" we
simply use a 0 value for musb->is_active.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/usb/musb/musb_core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index df3f65d..f68afef 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -653,7 +653,8 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
 				break;
 		case OTG_STATE_B_PERIPHERAL:
 			musb_g_suspend(musb);
-			musb->is_active = otg->gadget->b_hnp_enable;
+			musb->is_active =
+				otg->gadget ? otg->gadget->b_hnp_enable : 0;
 			if (musb->is_active) {
 				musb->xceiv->state = OTG_STATE_B_WAIT_ACON;
 				dev_dbg(musb->controller, "HNP: Setting timer for b_ase0_brst\n");
-- 
1.7.7.4

Re: [PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.

[Greg Kroah-Hartman]

On Wed, Feb 12, 2014 at 11:08:22AM +0100, Dr. H. Nikolaus Schaller wrote:
> fixed a potential NULL pointer dereference.
>     
>     Rationale:
>     this is the only location in the musb driver where the
>     otg->gadget pointer is dereferenced. Assuming that it
>     is never NULL is not only potentially unsafe but was
>     observed in the wild on a GTA04 (OMAP3/TPS65950 based
>     board) when trying to boot a device tree based 3.14-rc2
>     kernel with USB cable plugged in.
>     
>     DT boot appears to modify the order in which components
>     (gadget driver) are loaded and linked and therefore
>     an early musb interrupt triggers with a NULL gadget
>     pointer ending in a kernel panic.
>     
>     Since a non-existing gadget can never be "active" we
>     simply use a 0 value for musb->is_active.
>     
>     Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Please don't attach patches, our tools don't like them at all.

And shouldn't we fix the root problem here, not just gloss over the fact
that this pointer is NULL at this point in time?  Fixing the real issue
should be the correct solution, right?

thanks,

greg k-h

Re: [PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.

[Dr. H. Nikolaus Schaller]

Am 12.02.2014 um 15:29 schrieb Greg Kroah-Hartman:

> On Wed, Feb 12, 2014 at 11:08:22AM +0100, Dr. H. Nikolaus Schaller wrote:
>> fixed a potential NULL pointer dereference.
>> 
>>    Rationale:
>>    this is the only location in the musb driver where the
>>    otg->gadget pointer is dereferenced. Assuming that it
>>    is never NULL is not only potentially unsafe but was
>>    observed in the wild on a GTA04 (OMAP3/TPS65950 based
>>    board) when trying to boot a device tree based 3.14-rc2
>>    kernel with USB cable plugged in.
>> 
>>    DT boot appears to modify the order in which components
>>    (gadget driver) are loaded and linked and therefore
>>    an early musb interrupt triggers with a NULL gadget
>>    pointer ending in a kernel panic.
>> 
>>    Since a non-existing gadget can never be "active" we
>>    simply use a 0 value for musb->is_active.
>> 
>>    Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> 
> Please don't attach patches, our tools don't like them at all.

Sorry. That was by accident. Here is the patch appended.

> 
> And shouldn't we fix the root problem here, not just gloss over the fact
> that this pointer is NULL at this point in time?  Fixing the real issue
> should be the correct solution, right?

Well,
I must admit that I have no idea what the root cause could be 
(because I neither know the musb nor the gadget subsystems)
or if it is intended and just something in the initialization
sequence has changed.

I have done one more test and it appears to be in a non-device-tree
3.14-rc1 as well. We did not see it in a 3.12.7 kernel on the same hardware.

> 
> thanks,
> 
> greg k-h


BR,
Nikolaus Schaller

---
 drivers/usb/musb/musb_core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index df3f65d..f68afef 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -653,7 +653,8 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
 				break;
 		case OTG_STATE_B_PERIPHERAL:
 			musb_g_suspend(musb);
-			musb->is_active = otg->gadget->b_hnp_enable;
+			musb->is_active =
+				otg->gadget ? otg->gadget->b_hnp_enable : 0;
 			if (musb->is_active) {
 				musb->xceiv->state = OTG_STATE_B_WAIT_ACON;
 				dev_dbg(musb->controller, "HNP: Setting timer for b_ase0_brst\n");
-- 
1.7.7.4

Re: [PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.

[Greg Kroah-Hartman]

On Wed, Feb 12, 2014 at 04:35:12PM +0100, Dr. H. Nikolaus Schaller wrote:
> 
> Am 12.02.2014 um 15:29 schrieb Greg Kroah-Hartman:
> 
> > On Wed, Feb 12, 2014 at 11:08:22AM +0100, Dr. H. Nikolaus Schaller wrote:
> >> fixed a potential NULL pointer dereference.
> >> 
> >>    Rationale:
> >>    this is the only location in the musb driver where the
> >>    otg->gadget pointer is dereferenced. Assuming that it
> >>    is never NULL is not only potentially unsafe but was
> >>    observed in the wild on a GTA04 (OMAP3/TPS65950 based
> >>    board) when trying to boot a device tree based 3.14-rc2
> >>    kernel with USB cable plugged in.
> >> 
> >>    DT boot appears to modify the order in which components
> >>    (gadget driver) are loaded and linked and therefore
> >>    an early musb interrupt triggers with a NULL gadget
> >>    pointer ending in a kernel panic.
> >> 
> >>    Since a non-existing gadget can never be "active" we
> >>    simply use a 0 value for musb->is_active.
> >> 
> >>    Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> > 
> > Please don't attach patches, our tools don't like them at all.
> 
> Sorry. That was by accident. Here is the patch appended.

That's better, but we need a "clean" patch to be able to apply it for
real :)

> > And shouldn't we fix the root problem here, not just gloss over the fact
> > that this pointer is NULL at this point in time?  Fixing the real issue
> > should be the correct solution, right?
> 
> Well,
> I must admit that I have no idea what the root cause could be 
> (because I neither know the musb nor the gadget subsystems)
> or if it is intended and just something in the initialization
> sequence has changed.
> 
> I have done one more test and it appears to be in a non-device-tree
> 3.14-rc1 as well. We did not see it in a 3.12.7 kernel on the same hardware.

Ah, can you run 'git bisect' to track down the real problem here?  We
don't like papering over problems like this, that doesn't help anyone
out.

thanks,

greg k-h

Re: [PATCH 1 of 1]: musb: fixed a potential NULL pointer dereference.

[Dr. H. Nikolaus Schaller]

Am 12.02.2014 um 17:20 schrieb Greg Kroah-Hartman:

> On Wed, Feb 12, 2014 at 04:35:12PM +0100, Dr. H. Nikolaus Schaller wrote:
>> 
>> Am 12.02.2014 um 15:29 schrieb Greg Kroah-Hartman:
>> 
>>> On Wed, Feb 12, 2014 at 11:08:22AM +0100, Dr. H. Nikolaus Schaller wrote:
>>>> fixed a potential NULL pointer dereference.
>>>> 
>>>>   Rationale:
>>>>   this is the only location in the musb driver where the
>>>>   otg->gadget pointer is dereferenced. Assuming that it
>>>>   is never NULL is not only potentially unsafe but was
>>>>   observed in the wild on a GTA04 (OMAP3/TPS65950 based
>>>>   board) when trying to boot a device tree based 3.14-rc2
>>>>   kernel with USB cable plugged in.
>>>> 
>>>>   DT boot appears to modify the order in which components
>>>>   (gadget driver) are loaded and linked and therefore
>>>>   an early musb interrupt triggers with a NULL gadget
>>>>   pointer ending in a kernel panic.
>>>> 
>>>>   Since a non-existing gadget can never be "active" we
>>>>   simply use a 0 value for musb->is_active.
>>>> 
>>>>   Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> 
>>> Please don't attach patches, our tools don't like them at all.
>> 
>> Sorry. That was by accident. Here is the patch appended.
> 
> That's better, but we need a "clean" patch to be able to apply it for
> real :)

Ok, please take it as a "bug report" and documentation how a
workaround could look like - and not as a fix to be applied.

> 
>>> And shouldn't we fix the root problem here, not just gloss over the fact
>>> that this pointer is NULL at this point in time?  Fixing the real issue
>>> should be the correct solution, right?
>> 
>> Well,
>> I must admit that I have no idea what the root cause could be 
>> (because I neither know the musb nor the gadget subsystems)
>> or if it is intended and just something in the initialization
>> sequence has changed.
>> 
>> I have done one more test and it appears to be in a non-device-tree
>> 3.14-rc1 as well. We did not see it in a 3.12.7 kernel on the same hardware.
> 
> Ah, can you run 'git bisect' to track down the real problem here?  We
> don't like papering over problems like this, that doesn't help anyone
> out.

Well,I can't promise anything. Bisecting kernel panic problems means
compiling, flashing a SD cards, booting, looking at the result, telling git
the outcome and compiling a different commit.

I tried and failed immediately because the proposed bisects did
not even compile or boot on our hardware.

The problem appears to be that if we want to bisect e.g.

	(3.12.7 + private extensions) ... (3.14-rc1 + adapted private extensions)

that git bisect finds the common ancestor and presents some commit
in the middle. And that one does not "interpolate" between the required
private extensions but is some mainline version that we can't even
compile (first problem is with our defconfig not being available).

But we need these extensions to make the kernel run on our board and
are in the (uncompleted) middle of upstreaming them. So I think bisecting
works only on boards that are already completely supported by mainline.
Or at least at the "bisect good" commit.

So to me it appears that the faster approach would be if someone who
understands how musb interacts with the gadget setup/loading mechanism
helps to look into the code.

Hope we can find someone with more insights on the code than me,
Nikolaus Schaller