Skip to contentSkip to conversation

phy: cadence: Sierra: fix error handling bugs in probe()

PATCH Applied Dan Carpenter 2 participants 2 messages Jan 15, 2022 – Jan 23, 2022

Subsystems (2)
Kernel janitors Generic PHY framework
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 phy: cadence: Sierra: fix error handling bugs in probe()

There are two bugs in the error handling:
1: If devm_of_phy_provider_register() fails then there was no cleanup. 2: The error handling called of_node_put(child) improperly leading to
a use after free. We are only holding the reference inside the loop so the last two gotos after the loop lead to a use after free bug. Fix this by cleaning up the partial allocations (or partial iterations) in the loop before doing the goto.

Fixes a43f72ae136a ("phy: cadence: Sierra: Change MAX_LANES of Sierra to 16")
Fixes 44d30d622821 ("phy: cadence: Add driver for Sierra PHY")
Signed-off-by Dan Carpenter <[email protected]>

---
From static analysis. Not tested.

 drivers/phy/cadence/phy-cadence-sierra.c | 35 ++++++++++++++----------
 1 file changed, 21 insertions(+), 14 deletions(-)
drivers/phy/cadence/phy-cadence-sierra.c +21 -14
index da24acd26666..e265647e29a2 100644
@@ -1338,7 +1338,7 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	const struct cdns_sierra_data *data;
 	unsigned int id_value;
-	int i, ret, node = 0;
+	int ret, node = 0;
 	void __iomem *base;
 	struct device_node *dn = dev->of_node, *child;
 

@@ -1416,7 +1416,8 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 			dev_err(dev, "failed to get reset %s\n",
 				child->full_name);
 			ret = PTR_ERR(sp->phys[node].lnk_rst);
-			goto put_child2;
+			of_node_put(child);
+			goto put_control;
 		}
 
 		if (!sp->autoconf) {

@@ -1424,7 +1425,9 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 			if (ret) {
 				dev_err(dev, "missing property in node %s\n",
 					child->name);
-				goto put_child;
+				of_node_put(child);
+				reset_control_put(sp->phys[node].lnk_rst);
+				goto put_control;
 			}
 		}
 

@@ -1434,7 +1437,9 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 
 		if (IS_ERR(gphy)) {
 			ret = PTR_ERR(gphy);
-			goto put_child;
+			of_node_put(child);
+			reset_control_put(sp->phys[node].lnk_rst);
+			goto put_control;
 		}
 		sp->phys[node].phy = gphy;
 		phy_set_drvdata(gphy, &sp->phys[node]);

@@ -1446,26 +1451,28 @@ static int cdns_sierra_phy_probe(struct platform_device *pdev)
 	if (sp->num_lanes > SIERRA_MAX_LANES) {
 		ret = -EINVAL;
 		dev_err(dev, "Invalid lane configuration\n");
-		goto put_child2;
+		goto put_control;
 	}
 
 	/* If more than one subnode, configure the PHY as multilink */
 	if (!sp->autoconf && sp->nsubnodes > 1) {
 		ret = cdns_sierra_phy_configure_multilink(sp);
 		if (ret)
-			goto put_child2;
+			goto put_control;
 	}
 
 	pm_runtime_enable(dev);
 	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
-	return PTR_ERR_OR_ZERO(phy_provider);
-
-put_child:
-	node++;
-put_child2:
-	for (i = 0; i < node; i++)
-		reset_control_put(sp->phys[i].lnk_rst);
-	of_node_put(child);
+	if (IS_ERR(phy_provider)) {
+		ret = PTR_ERR(phy_provider);
+		goto put_control;
+	}
+
+	return 0;
+
+put_control:
+	while (--node >= 0)
+		reset_control_put(sp->phys[node].lnk_rst);
 clk_disable:
 	cdns_sierra_phy_disable_clocks(sp);
 	reset_control_assert(sp->apb_rst);
Signature · 7 lines
2.20.1


-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy
maintainer Applied
#1 lore raw
Quoted 7 lines · Dan Carpenter

There are two bugs in the error handling:
1: If devm_of_phy_provider_register() fails then there was no cleanup. 2: The error handling called of_node_put(child) improperly leading to
a use after free. We are only holding the reference inside the loop so the last two gotos after the loop lead to a use after free bug. Fix this by cleaning up the partial allocations (or partial iterations) in the loop before doing the goto.

Applied, thanks

Signature · 6 lines
~Vinod

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy