Skip to contentSkip to conversation

USB: chipidea: msm: fix ulpi-node lookup

PATCH In review Johan Hovold 2 participants 4 messages Nov 13, 2017 – Dec 13, 2017

Subsystems (3)
Stable branch USB Chipidea USB high speed dual role controller
lore
Share & tools
lore

Johan Hovold

author
#0 lore raw

PATCH 1/1 USB: chipidea: msm: fix ulpi-node lookup

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 <[email protected]> # 4.10: b74c43156c0c
Cc Stephen Boyd <[email protected]>
Cc Frank Rowand <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/chipidea/ci_hdrc_msm.c +1 -1
index 3593ce0ec641..880009987460 100644
@@ -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");
Signature · 1 line
2.15.0
#1 lore raw
Quoted 23 lines · 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 <[email protected]> # 4.10: b74c43156c0c
Cc Stephen Boyd <[email protected]>
Cc Frank Rowand <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/chipidea/ci_hdrc_msm.c +0 -0
index 3593ce0ec641..880009987460 100644

drivers/usb/chipidea/ci_hdrc_msm.c static int ci_hdrc_msm_probe(struct platform_device *pdev) · line 250

Quoted 5 lines
drivers/usb/chipidea/ci_hdrc_msm.c +1 -1
@@ -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.

Signature · 2 lines
Best Regards,
Peter Chen
author
#2 lore raw
Quoted 27 lines · Peter Chen
Quoted 26 lines · 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 <[email protected]> # 4.10: b74c43156c0c
Cc Stephen Boyd <[email protected]>
Cc Frank Rowand <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/chipidea/ci_hdrc_msm.c +0 -0
index 3593ce0ec641..880009987460 100644
@@ -247,7 +247,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_mux;
Quoted 5 lines

drivers/usb/chipidea/ci_hdrc_msm.c static int ci_hdrc_msm_probe(struct platform_device *pdev) · line 250

Quoted 2 lines
drivers/usb/chipidea/ci_hdrc_msm.c +1 -1
-	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

#3 lore raw
Quoted 50 lines · Johan Hovold
Quoted 27 lines · Peter Chen
Quoted 26 lines · 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 <[email protected]> # 4.10: b74c43156c0c
Cc Stephen Boyd <[email protected]>
Cc Frank Rowand <[email protected]>
Signed-off-by Johan Hovold <[email protected]>
---
 drivers/usb/chipidea/ci_hdrc_msm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/chipidea/ci_hdrc_msm.c +0 -0
index 3593ce0ec641..880009987460 100644
@@ -247,7 +247,7 @@ static int ci_hdrc_msm_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_mux;
Quoted 5 lines

drivers/usb/chipidea/ci_hdrc_msm.c static int ci_hdrc_msm_probe(struct platform_device *pdev) · line 250

Quoted 2 lines
drivers/usb/chipidea/ci_hdrc_msm.c +1 -1
-	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.

Quoted 5 lines

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.

Signature · 2 lines
Best Regards,
Peter Chen