* [PATCH] phy: cadence: Sierra: fix error handling bugs in probe()
@ 2022-01-15 11:51 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2022-01-15 11:51 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Alan Douglas
Cc: Vinod Koul, Philipp Zabel, Swapnil Jakhade, Aswath Govindraju,
linux-phy, kernel-janitors
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 <dan.carpenter@oracle.com>
---
From static analysis. Not tested.
drivers/phy/cadence/phy-cadence-sierra.c | 35 ++++++++++++++----------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index da24acd26666..e265647e29a2 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -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);
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH] phy: cadence: Sierra: fix error handling bugs in probe()
@ 2022-01-15 11:51 ` Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2022-01-15 11:51 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Alan Douglas
Cc: Vinod Koul, Philipp Zabel, Swapnil Jakhade, Aswath Govindraju,
linux-phy, kernel-janitors
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 <dan.carpenter@oracle.com>
---
From static analysis. Not tested.
drivers/phy/cadence/phy-cadence-sierra.c | 35 ++++++++++++++----------
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index da24acd26666..e265647e29a2 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -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);
--
2.20.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] phy: cadence: Sierra: fix error handling bugs in probe()
2022-01-15 11:51 ` Dan Carpenter
@ 2022-01-23 14:04 ` Vinod Koul
-1 siblings, 0 replies; 4+ messages in thread
From: Vinod Koul @ 2022-01-23 14:04 UTC (permalink / raw)
To: Dan Carpenter
Cc: Kishon Vijay Abraham I, Alan Douglas, Philipp Zabel,
Swapnil Jakhade, Aswath Govindraju, linux-phy, kernel-janitors
On 15-01-22, 14:51, Dan Carpenter wrote:
> 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
--
~Vinod
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] phy: cadence: Sierra: fix error handling bugs in probe()
@ 2022-01-23 14:04 ` Vinod Koul
0 siblings, 0 replies; 4+ messages in thread
From: Vinod Koul @ 2022-01-23 14:04 UTC (permalink / raw)
To: Dan Carpenter
Cc: Kishon Vijay Abraham I, Alan Douglas, Philipp Zabel,
Swapnil Jakhade, Aswath Govindraju, linux-phy, kernel-janitors
On 15-01-22, 14:51, Dan Carpenter wrote:
> 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
--
~Vinod
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-23 14:04 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-15 11:51 [PATCH] phy: cadence: Sierra: fix error handling bugs in probe() Dan Carpenter
2022-01-15 11:51 ` Dan Carpenter
2022-01-23 14:04 ` Vinod Koul
2022-01-23 14:04 ` Vinod Koul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.