net: ti: icssg: Fix device_node handling in prueth_probe()

Hello,

This series fixes the device_node handling in prueth_probe() of both ICSSG drivers.

Patches 1 and 2, carried over from v1 unchanged, fix a device_node refcount leak on the "ti,mii-g-rt" and "ti,mii-rt" syscon lookup failure paths. Patches 3 and 4 fix the port scan loop that is shared by both drivers: a port whose reg property cannot be read is skipped instead of being routed based on the uninitialized value, the node reference is taken only for a port that is actually accepted so an out-of-range reg value no longer leaks it, duplicate reg values are rejected, which is what the existing "port0 and port1 can't have same reg" check intended but never caught, and that unreachable check is dropped.

Thanks to Simon Horman for the review of v1 [1][2] and for suggesting to collect the fixes into one series.

Changes in v2:

- Collect the two independent fixes into one series (suggested by Simon Horman).

- Add the port scan fixes for both drivers.

[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/

Xiangwu Lai (4):
net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()
net: ti: icssg-prueth-sr1: Fix device_node refcount leak in
prueth_probe()
net: ti: icssg-prueth: Fix port scan issues in prueth_probe()
net: ti: icssg-prueth-sr1: Fix port scan issues in prueth_probe()

 drivers/net/ethernet/ti/icssg/icssg_prueth.c  | 32 +++++++++++--------
 .../net/ethernet/ti/icssg/icssg_prueth_sr1.c  | 32 +++++++++++--------
 2 files changed, 38 insertions(+), 26 deletions(-)

2.50.1.windows.1

prueth_probe() takes references on the eth0_node/eth1_node device nodes with of_node_get() and stores them in prueth->eth_node[]. If either of the "ti,mii-g-rt" or "ti,mii-rt" syscon regmap lookups fails, probe() returns -ENODEV without dropping those references, leaking the device nodes.

Jump to the existing put_cores label so the error path releases the eth0_node/eth1_node references like all the other failure paths in the function do.

---
Changes in v2:

- Collect the two independent fixes into one series (suggested by Simon Horman).

- No code change since v1.

 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index 591be5c80..9b971d1df 100644
@@ -2258,13 +2258,15 @@ static int prueth_probe(struct platform_device *pdev)
 	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
 	if (IS_ERR(prueth->miig_rt)) {
 		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto put_cores;
 	}
 
 	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
 	if (IS_ERR(prueth->mii_rt)) {
 		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto put_cores;
 	}
 
 	prueth->pa_stats = syscon_regmap_lookup_by_phandle(np, "ti,pa-stats");

2.50.1.windows.1

prueth_probe() takes references on the eth0_node/eth1_node device nodes with of_node_get() and stores them in prueth->eth_node[]. If either of the "ti,mii-g-rt" or "ti,mii-rt" syscon regmap lookups fails, probe() returns -ENODEV without dropping those references, leaking the device nodes.

Jump to the existing put_cores label so the error path releases the eth0_node/eth1_node references like all the other failure paths in the function do.

---
Changes in v2:

- Collect the two independent fixes into one series (suggested by Simon Horman).

- No code change since v1.

 drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

index b8115ca47..ef1dc32ff 100644
@@ -963,13 +963,15 @@ static int prueth_probe(struct platform_device *pdev)
 	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
 	if (IS_ERR(prueth->miig_rt)) {
 		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto put_cores;
 	}
 
 	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
 	if (IS_ERR(prueth->mii_rt)) {
 		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto put_cores;
 	}
 
 	if (eth0_node) {

2.50.1.windows.1

The port scan loop in prueth_probe() has several issues:

- When reading the reg property of a port fails, the error is only logged and the uninitialized value of reg is then used to decide which slot the port node is stored in.

- of_node_get() is called on every port child before the reg value is validated. If reg is neither 0 nor 1, the reference is never released. If two ports use the same reg value, the second assignment overwrites the first pointer and its reference is lost.

- The "port0 and port1 can't have same reg" check after the loop is dead code: both variables are initialized to NULL, each iteration assigns at most one of them, for_each_child_of_node() visits each child exactly once, and the both-NULL case is handled by the preceding check.

Fix the first issue by skipping a port when its reg property cannot be read, and the second by taking the reference only when a port is actually accepted. Reject duplicate reg values, which is what the existing check intended but never caught, and remove the unreachable check.

---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 26 +++++++++++---------
 1 file changed, 15 insertions(+), 11 deletions(-)

index 9b971d1df..c19a51174 100644
@@ -2217,24 +2217,34 @@ static int prueth_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			dev_err(dev, "%pOF error reading port_id %d\n",
 				eth_node, ret);
+			continue;
 		}
 
-		of_node_get(eth_node);
+		if (reg != 0 && reg != 1) {
+			dev_err(dev, "port reg should be 0 or 1\n");
+			continue;
+		}
+
+		if ((reg == 0 && eth0_node) || (reg == 1 && eth1_node)) {
+			dev_err(dev, "port reg %d already in use\n", reg);
+			ret = -ENODEV;
+			of_node_put(eth_node);
+			of_node_put(eth_ports_node);
+			goto put_cores;
+		}
 
 		if (reg == 0) {
-			eth0_node = eth_node;
+			eth0_node = of_node_get(eth_node);
 			if (!of_device_is_available(eth0_node)) {
 				of_node_put(eth0_node);
 				eth0_node = NULL;
 			}
 		} else if (reg == 1) {
-			eth1_node = eth_node;
+			eth1_node = of_node_get(eth_node);
 			if (!of_device_is_available(eth1_node)) {
 				of_node_put(eth1_node);
 				eth1_node = NULL;
 			}
-		} else {
-			dev_err(dev, "port reg should be 0 or 1\n");
 		}
 	}
 

@@ -2246,12 +2256,6 @@ static int prueth_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	if (eth0_node == eth1_node) {
-		dev_err(dev, "port0 and port1 can't have same reg\n");
-		of_node_put(eth0_node);
-		return -ENODEV;
-	}
-
 	prueth->eth_node[PRUETH_MAC0] = eth0_node;
 	prueth->eth_node[PRUETH_MAC1] = eth1_node;
 

2.50.1.windows.1

The port scan loop in prueth_probe() has several issues:

- When reading the reg property of a port fails, the error is only logged and the uninitialized value of reg is then used to decide which slot the port node is stored in.

- of_node_get() is called on every port child before the reg value is validated. If reg is neither 0 nor 1, the reference is never released. If two ports use the same reg value, the second assignment overwrites the first pointer and its reference is lost.

- The "port0 and port1 can't have same reg" check after the loop is dead code: both variables are initialized to NULL, each iteration assigns at most one of them, for_each_child_of_node() visits each child exactly once, and the both-NULL case is handled by the preceding check.

Fix the first issue by skipping a port when its reg property cannot be read, and the second by taking the reference only when a port is actually accepted. Reject duplicate reg values, which is what the existing check intended but never caught, and remove the unreachable check.

---
 .../net/ethernet/ti/icssg/icssg_prueth_sr1.c  | 26 +++++++++++--------
 1 file changed, 15 insertions(+), 11 deletions(-)

index ef1dc32ff..1a9dd70b2 100644
@@ -922,24 +922,34 @@ static int prueth_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			dev_err(dev, "%pOF error reading port_id %d\n",
 				eth_node, ret);
+			continue;
 		}
 
-		of_node_get(eth_node);
+		if (reg != 0 && reg != 1) {
+			dev_err(dev, "port reg should be 0 or 1\n");
+			continue;
+		}
+
+		if ((reg == 0 && eth0_node) || (reg == 1 && eth1_node)) {
+			dev_err(dev, "port reg %d already in use\n", reg);
+			ret = -ENODEV;
+			of_node_put(eth_node);
+			of_node_put(eth_ports_node);
+			goto put_cores;
+		}
 
 		if (reg == 0) {
-			eth0_node = eth_node;
+			eth0_node = of_node_get(eth_node);
 			if (!of_device_is_available(eth0_node)) {
 				of_node_put(eth0_node);
 				eth0_node = NULL;
 			}
 		} else if (reg == 1) {
-			eth1_node = eth_node;
+			eth1_node = of_node_get(eth_node);
 			if (!of_device_is_available(eth1_node)) {
 				of_node_put(eth1_node);
 				eth1_node = NULL;
 			}
-		} else {
-			dev_err(dev, "port reg should be 0 or 1\n");
 		}
 	}
 

@@ -951,12 +961,6 @@ static int prueth_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	if (eth0_node == eth1_node) {
-		dev_err(dev, "port0 and port1 can't have same reg\n");
-		of_node_put(eth0_node);
-		return -ENODEV;
-	}
-
 	prueth->eth_node[PRUETH_MAC0] = eth0_node;
 	prueth->eth_node[PRUETH_MAC1] = eth1_node;
 

2.50.1.windows.1

Hi,

Please find below the additional information requested for this series. The same information was provided for the v1 postings of patches 1 and 2 here:

https://lore.kernel.org/netdev/[email protected]/
https://lore.kernel.org/netdev/[email protected]/

How the issues were discovered: manual code inspection of the error paths in prueth_probe() while auditing both ICSSG drivers for probe-time resource leaks. The port scan issues addressed by patches 3 and 4 were found in the same audit; the use of the uninitialized reg value was also raised during the review of v1.

Whether the issues were actually triggered: no, they are purely theoretical, all of them require a malformed device tree:

- Patches 1 and 2: the leak triggers only when the "ti,mii-g-rt" or "ti,mii-rt" syscon regmap lookup fails, which does not happen on a valid DT. probe() then fails with -ENODEV and the references taken on eth0_node/eth1_node are leaked silently; there is no stack trace or error message pointing at the leak itself.

- Patches 3 and 4: the uninitialized reg value is only used when a port node has no reg property, and a reference is only leaked for port nodes with a reg value outside 0/1 or with a duplicate reg value. On a valid DT (one port with reg 0 and one with reg 1) the loop behaves as before, with identical reference accounting for the accepted ports.

Hardware the change was tested on: none, the series was not tested on real hardware. Patches 1 and 2 only re-route two error branches to the existing put_cores label, which is already exercised by the other failure paths in the same function (e.g. prueth_get_cores() failures). Patches 3 and 4 move of_node_get() from an unconditional call on every port child to the assignment points, so for ports that are accepted the reference accounting is unchanged, and the new error path only releases references it has just taken.

Best regards,
Xiangwu Lai

Hi,

The netdev patchwork cc_maintainers check reports that some people listed by get_maintainer.pl for the files touched by this series were not on Cc:

- Vignesh Raghavendra <[email protected]> (original contributor of the ICSSG driver code this series fixes)
- Meghana Malladi <[email protected]>
- Jacob Keller <[email protected]>

Adding them here for visibility. They will be included directly in the Cc list of any future revision of the series. No action is required from them.

Best regards,
Xiangwu Lai