[PATCH] USB: chipidea: msm: fix ulpi-node lookup

[PATCH] USB: chipidea: msm: fix ulpi-node lookup

[Johan Hovold]

Fix child-node lookup during probe, which ended up searching the whole
device tree depth-first starting at the parent rather than just matching
on its children.

Note that the original premature free of the parent node has already
been fixed separately, but that fix was apparently never backported to
stable.

Fixes: 47654a162081 ("usb: chipidea: msm: Restore wrapper settings after reset")
Fixes: b74c43156c0c ("usb: chipidea: msm: ci_hdrc_msm_probe() missing of_node_get()")
Cc: stable <stable@vger.kernel.org>     # 4.10: b74c43156c0c
Cc: Stephen Boyd <stephen.boyd@linaro.org>
Cc: Frank Rowand <frank.rowand@sony.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c
index 3593ce0ec641..880009987460 100644
--- a/drivers/usb/chipidea/ci_hdrc_msm.c
+++ b/drivers/usb/chipidea/ci_hdrc_msm.c
@@ -247,7 +247,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_mux;
 
-	ulpi_node = of_find_node_by_name(of_node_get(pdev->dev.of_node), "ulpi");
+	ulpi_node = of_get_child_by_name(pdev->dev.of_node, "ulpi");
 	if (ulpi_node) {
 		phy_node = of_get_next_available_child(ulpi_node, NULL);
 		ci->hsic = of_device_is_compatible(phy_node, "qcom,usb-hsic-phy");
-- 
2.15.0

Re: [PATCH] USB: chipidea: msm: fix ulpi-node lookup

[Peter Chen]

On Mon, Nov 13, 2017 at 11:12:58AM +0100, Johan Hovold wrote:
> Fix child-node lookup during probe, which ended up searching the whole
> device tree depth-first starting at the parent rather than just matching
> on its children.
> 
> Note that the original premature free of the parent node has already
> been fixed separately, but that fix was apparently never backported to
> stable.
> 
> Fixes: 47654a162081 ("usb: chipidea: msm: Restore wrapper settings after reset")
> Fixes: b74c43156c0c ("usb: chipidea: msm: ci_hdrc_msm_probe() missing of_node_get()")
> Cc: stable <stable@vger.kernel.org>     # 4.10: b74c43156c0c
> Cc: Stephen Boyd <stephen.boyd@linaro.org>
> Cc: Frank Rowand <frank.rowand@sony.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c
> index 3593ce0ec641..880009987460 100644
> --- a/drivers/usb/chipidea/ci_hdrc_msm.c
> +++ b/drivers/usb/chipidea/ci_hdrc_msm.c
> @@ -247,7 +247,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_mux;
>  
> -	ulpi_node = of_find_node_by_name(of_node_get(pdev->dev.of_node), "ulpi");
> +	ulpi_node = of_get_child_by_name(pdev->dev.of_node, "ulpi");

Stephen, would you comment on it? I am afraid I can't find the benefit
for this change.

-- 

Best Regards,
Peter Chen

Re: [PATCH] USB: chipidea: msm: fix ulpi-node lookup

[Johan Hovold]

On Tue, Dec 12, 2017 at 11:08:17AM +0800, Peter Chen wrote:
> On Mon, Nov 13, 2017 at 11:12:58AM +0100, Johan Hovold wrote:
> > Fix child-node lookup during probe, which ended up searching the whole
> > device tree depth-first starting at the parent rather than just matching
> > on its children.
> > 
> > Note that the original premature free of the parent node has already
> > been fixed separately, but that fix was apparently never backported to
> > stable.
> > 
> > Fixes: 47654a162081 ("usb: chipidea: msm: Restore wrapper settings after reset")
> > Fixes: b74c43156c0c ("usb: chipidea: msm: ci_hdrc_msm_probe() missing of_node_get()")
> > Cc: stable <stable@vger.kernel.org>     # 4.10: b74c43156c0c
> > Cc: Stephen Boyd <stephen.boyd@linaro.org>
> > Cc: Frank Rowand <frank.rowand@sony.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c
> > index 3593ce0ec641..880009987460 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_msm.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_msm.c
> > @@ -247,7 +247,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto err_mux;
> >  
> > -	ulpi_node = of_find_node_by_name(of_node_get(pdev->dev.of_node), "ulpi");
> > +	ulpi_node = of_get_child_by_name(pdev->dev.of_node, "ulpi");
> 
> Stephen, would you comment on it? I am afraid I can't find the benefit
> for this change.

The general problem is that several drivers were using the wrong
OF-helper when looking up child nodes. This meant that instead of just
matching on child nodes, a tree-wide, depth-first search was done,
something which could end up matching an unrelated node elsewhere in the
device tree.

To make things worse, most erroneous users of of_find_node_by_name(),
also failed to notice that that function drops a reference to its first
argument, something which can lead to use-after-free and crashes, for
example, after probe deferrals.

In this case, it looks like the child node is looked-up to determine
whether to enable a hardware quirk. The potential use-after-free has
already been fixed up (by adding the missing of_node_get()), but that
fix was never backported to stable.

This patch addresses both issues (tree-wide search + unbalanced put in
stable), while removing buggy code which could otherwise end up being
reproduced in yet another driver.

Johan

Re: [PATCH] USB: chipidea: msm: fix ulpi-node lookup

[Peter Chen]

On Tue, Dec 12, 2017 at 01:59:29PM +0100, Johan Hovold wrote:
> On Tue, Dec 12, 2017 at 11:08:17AM +0800, Peter Chen wrote:
> > On Mon, Nov 13, 2017 at 11:12:58AM +0100, Johan Hovold wrote:
> > > Fix child-node lookup during probe, which ended up searching the whole
> > > device tree depth-first starting at the parent rather than just matching
> > > on its children.
> > > 
> > > Note that the original premature free of the parent node has already
> > > been fixed separately, but that fix was apparently never backported to
> > > stable.
> > > 
> > > Fixes: 47654a162081 ("usb: chipidea: msm: Restore wrapper settings after reset")
> > > Fixes: b74c43156c0c ("usb: chipidea: msm: ci_hdrc_msm_probe() missing of_node_get()")
> > > Cc: stable <stable@vger.kernel.org>     # 4.10: b74c43156c0c
> > > Cc: Stephen Boyd <stephen.boyd@linaro.org>
> > > Cc: Frank Rowand <frank.rowand@sony.com>
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > >  drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_msm.c b/drivers/usb/chipidea/ci_hdrc_msm.c
> > > index 3593ce0ec641..880009987460 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_msm.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_msm.c
> > > @@ -247,7 +247,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
> > >  	if (ret)
> > >  		goto err_mux;
> > >  
> > > -	ulpi_node = of_find_node_by_name(of_node_get(pdev->dev.of_node), "ulpi");
> > > +	ulpi_node = of_get_child_by_name(pdev->dev.of_node, "ulpi");
> > 
> > Stephen, would you comment on it? I am afraid I can't find the benefit
> > for this change.
> 
> The general problem is that several drivers were using the wrong
> OF-helper when looking up child nodes. This meant that instead of just
> matching on child nodes, a tree-wide, depth-first search was done,
> something which could end up matching an unrelated node elsewhere in the
> device tree.
> 
> To make things worse, most erroneous users of of_find_node_by_name(),
> also failed to notice that that function drops a reference to its first
> argument, something which can lead to use-after-free and crashes, for
> example, after probe deferrals.
> 
> In this case, it looks like the child node is looked-up to determine
> whether to enable a hardware quirk. The potential use-after-free has
> already been fixed up (by adding the missing of_node_get()), but that
> fix was never backported to stable.
> 
> This patch addresses both issues (tree-wide search + unbalanced put in
> stable), while removing buggy code which could otherwise end up being
> reproduced in yet another driver.
> 
> Johan

Thanks, if Stephen does not have comments, I will send Greg it next
Monday.

-- 

Best Regards,
Peter Chen