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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
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