[PATCH] USB: core: of: Check device_node before parsing in usb_of_get_child_node()

[PATCH] USB: core: of: Check device_node before parsing in usb_of_get_child_node()

[Vaibhav Hiremath]

In case of HUB devices connected to USB ports, we may not have DT
node representing it inside USB, and when devices connected to hub
gets enumerated, call to usb_of_get_child_node() leads to NULL pointer
dereference.

In the usecase we have, where EHCI port is connected to USB HUB
device, and downward ports of HUB are connected to further USB
devices. When those devices gets enumerated, in order,
 1. USB HUB ->
	-> Call to usb_of_get_child_node() is OK, as
	parent->dev.of_node is pointing to host node.
 2. Devices connected to downward port of USB HUB
 	-> Call to usb_of_get_child_node() leads to NULL
	pointer dereference as parent->dev.of_node = NULL,
	as USB HUB DTS node may be empty.

Fix this NULL pointer dereference by adding check for pointer
device_node inside usb_of_get_child_node() fn.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
Testing: I have build tested it against mainline.

 drivers/usb/core/of.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
index 2289700..dc667a3 100644
--- a/drivers/usb/core/of.c
+++ b/drivers/usb/core/of.c
@@ -34,6 +34,9 @@ struct device_node *usb_of_get_child_node(struct device_node *parent,
 	struct device_node *node;
 	u32 port;
 
+	if (!parent)
+		return NULL;
+
 	for_each_child_of_node(parent, node) {
 		if (!of_property_read_u32(node, "reg", &port)) {
 			if (port == portnum)
-- 
2.7.4

Re: [PATCH] USB: core: of: Check device_node before parsing in usb_of_get_child_node()

[Greg KH]

On Mon, Aug 15, 2016 at 11:31:10AM -0700, Vaibhav Hiremath wrote:
> In case of HUB devices connected to USB ports, we may not have DT
> node representing it inside USB, and when devices connected to hub
> gets enumerated, call to usb_of_get_child_node() leads to NULL pointer
> dereference.

Really?  That seems messed up.

> In the usecase we have, where EHCI port is connected to USB HUB
> device, and downward ports of HUB are connected to further USB
> devices. When those devices gets enumerated, in order,
>  1. USB HUB ->
> 	-> Call to usb_of_get_child_node() is OK, as
> 	parent->dev.of_node is pointing to host node.
>  2. Devices connected to downward port of USB HUB
>  	-> Call to usb_of_get_child_node() leads to NULL
> 	pointer dereference as parent->dev.of_node = NULL,
> 	as USB HUB DTS node may be empty.

Why is the hub DTS empty?  Shouldn't that be the fix here?

thanks,

greg k-h

Re: [PATCH] USB: core: of: Check device_node before parsing in usb_of_get_child_node()

[Vaibhav Hiremath]

On Monday 15 August 2016 11:41 AM, Greg KH wrote:
> On Mon, Aug 15, 2016 at 11:31:10AM -0700, Vaibhav Hiremath wrote:
>> In case of HUB devices connected to USB ports, we may not have DT
>> node representing it inside USB, and when devices connected to hub
>> gets enumerated, call to usb_of_get_child_node() leads to NULL pointer
>> dereference.
> Really?  That seems messed up.

unfortunately yes :)

>> In the usecase we have, where EHCI port is connected to USB HUB
>> device, and downward ports of HUB are connected to further USB
>> devices. When those devices gets enumerated, in order,
>>   1. USB HUB ->
>> 	-> Call to usb_of_get_child_node() is OK, as
>> 	parent->dev.of_node is pointing to host node.
>>   2. Devices connected to downward port of USB HUB
>>   	-> Call to usb_of_get_child_node() leads to NULL
>> 	pointer dereference as parent->dev.of_node = NULL,
>> 	as USB HUB DTS node may be empty.
> Why is the hub DTS empty?  Shouldn't that be the fix here?

Because HUB can be enumerated dynamically and one possible
reason could be you don't need to do anything to bring up HUB.
May be one of following could be the reason -

  1. HUB automatically comes up on power ON, and USB host enumerates it.
    There is no control path for HUB

  2. HUB has different control path, in our case it is over I2C.
    So HUB configuration and bringup happens as part of I2C client driver.



So you may not need DTS for HUB as a child node inside USB host.
What I am trying to say here is,


&usb_ehci {
     ...

     status = "ok";
};

This would enumerate HUB first, and then devices connected to HUB, right?
So this will lead to kernel crash.



Reference DTS with HUB and downward devices -

&usb_ehci {
     status = "ok";

     usb_hub: usb_hub {
         compatible = "usbxxxx";
         reg = <1>;

         usb_dev: usb_dev {
             compatible = "usbxxxx";
             reg = <1>;

             ...
         };
    };
};


Thanks,
Vaibhav

Re: [PATCH] USB: core: of: Check device_node before parsing in usb_of_get_child_node()

[Alan Stern]

On Mon, 15 Aug 2016, Greg KH wrote:

> On Mon, Aug 15, 2016 at 11:31:10AM -0700, Vaibhav Hiremath wrote:
> > In case of HUB devices connected to USB ports, we may not have DT
> > node representing it inside USB, and when devices connected to hub
> > gets enumerated, call to usb_of_get_child_node() leads to NULL pointer
> > dereference.
> 
> Really?  That seems messed up.
> 
> > In the usecase we have, where EHCI port is connected to USB HUB
> > device, and downward ports of HUB are connected to further USB
> > devices. When those devices gets enumerated, in order,
> >  1. USB HUB ->
> > 	-> Call to usb_of_get_child_node() is OK, as
> > 	parent->dev.of_node is pointing to host node.
> >  2. Devices connected to downward port of USB HUB
> >  	-> Call to usb_of_get_child_node() leads to NULL
> > 	pointer dereference as parent->dev.of_node = NULL,
> > 	as USB HUB DTS node may be empty.
> 
> Why is the hub DTS empty?  Shouldn't that be the fix here?

It's empty because there's no DT entry for the hub.  That's normal; 
most USB devices aren't represented in DT because they aren't part of 
the original system -- they are added plugged in later.

Or, it's possible that the hub _is_ part of the original system and it 
was left out of the DT database.

Alan Stern

Re: [PATCH] USB: core: of: Check device_node before parsing in usb_of_get_child_node()

[Peter Chen]

On Mon, Aug 15, 2016 at 11:31:10AM -0700, Vaibhav Hiremath wrote:
> In case of HUB devices connected to USB ports, we may not have DT
> node representing it inside USB, and when devices connected to hub
> gets enumerated, call to usb_of_get_child_node() leads to NULL pointer
> dereference.
> 
> In the usecase we have, where EHCI port is connected to USB HUB
> device, and downward ports of HUB are connected to further USB
> devices. When those devices gets enumerated, in order,
>  1. USB HUB ->
> 	-> Call to usb_of_get_child_node() is OK, as
> 	parent->dev.of_node is pointing to host node.
>  2. Devices connected to downward port of USB HUB
>  	-> Call to usb_of_get_child_node() leads to NULL
> 	pointer dereference as parent->dev.of_node = NULL,
> 	as USB HUB DTS node may be empty.
> 
> Fix this NULL pointer dereference by adding check for pointer
> device_node inside usb_of_get_child_node() fn.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
> Testing: I have build tested it against mainline.
> 
>  drivers/usb/core/of.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> index 2289700..dc667a3 100644
> --- a/drivers/usb/core/of.c
> +++ b/drivers/usb/core/of.c
> @@ -34,6 +34,9 @@ struct device_node *usb_of_get_child_node(struct device_node *parent,
>  	struct device_node *node;
>  	u32 port;
>  
> +	if (!parent)
> +		return NULL;
> +
>  	for_each_child_of_node(parent, node) {
>  		if (!of_property_read_u32(node, "reg", &port)) {
>  			if (port == portnum)

I am afraid I can't reproduce it, would you please show me your dump
when null pointer dereference occurs? From what I find the
__of_get_next_child checks null pointer for parent node.

-- 

Best Regards,
Peter Chen

Re: [PATCH] USB: core: of: Check device_node before parsing in usb_of_get_child_node()

[Vaibhav Hiremath]

On Monday 15 August 2016 06:33 PM, Peter Chen wrote:
> On Mon, Aug 15, 2016 at 11:31:10AM -0700, Vaibhav Hiremath wrote:
>> In case of HUB devices connected to USB ports, we may not have DT
>> node representing it inside USB, and when devices connected to hub
>> gets enumerated, call to usb_of_get_child_node() leads to NULL pointer
>> dereference.
>>
>> In the usecase we have, where EHCI port is connected to USB HUB
>> device, and downward ports of HUB are connected to further USB
>> devices. When those devices gets enumerated, in order,
>>   1. USB HUB ->
>> 	-> Call to usb_of_get_child_node() is OK, as
>> 	parent->dev.of_node is pointing to host node.
>>   2. Devices connected to downward port of USB HUB
>>   	-> Call to usb_of_get_child_node() leads to NULL
>> 	pointer dereference as parent->dev.of_node = NULL,
>> 	as USB HUB DTS node may be empty.
>>
>> Fix this NULL pointer dereference by adding check for pointer
>> device_node inside usb_of_get_child_node() fn.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>> Testing: I have build tested it against mainline.
>>
>>   drivers/usb/core/of.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
>> index 2289700..dc667a3 100644
>> --- a/drivers/usb/core/of.c
>> +++ b/drivers/usb/core/of.c
>> @@ -34,6 +34,9 @@ struct device_node *usb_of_get_child_node(struct device_node *parent,
>>   	struct device_node *node;
>>   	u32 port;
>>   
>> +	if (!parent)
>> +		return NULL;
>> +
>>   	for_each_child_of_node(parent, node) {
>>   		if (!of_property_read_u32(node, "reg", &port)) {
>>   			if (port == portnum)
> I am afraid I can't reproduce it, would you please show me your dump
> when null pointer dereference occurs? From what I find the
> __of_get_next_child checks null pointer for parent node.
>

Peter,
You are right, __of_get_next_child is taking care of this.

When I observed this issue with my setup [1], I only looked at changes in
the mainline for of.c and core/usb.c, did not see the anything.....

Anyways, for the record, we do not need this patch. Instead I need to 
backport
below commit from mainline to my kernel base.

commit 43cb43678705e39b175b325f17938295996aefc7
Author: Florian Fainelli <f.fainelli@gmail.com>
Date:   Wed May 28 10:39:02 2014 -0700

     of: handle NULL node in next_child iterators

     Add an early check for the node argument in __of_get_next_child and
     of_get_next_available_child() to avoid dereferencing a NULL node 
pointer
     a few lines after.


[1] Also I missed to mention about my kernel version, I am based on very
ancient kernel version (3.10). Do not ask me why, it is something out of my
control :)

-- 
Thanks,
Vaibhav