Re: [PATCH v2 1/3] phy: renesas: rcar-gen2: Fix double of_node_put on phy creation failure

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing reference count increment for device node leads to a dangling pointer.
- [High] TOCTOU race condition between child count and iteration leads to out-of-bounds array write.
--

--- Patch [1]: [PATCH v2 1/3] phy: renesas: rcar-gen2: Fix double of_node_put on phy creation failure ---
commit 047745265c7b61da44949752ecc277b2156237a3
Author: Felix Gu <[email protected]>

phy: renesas: rcar-gen2: Fix double of_node_put on phy creation failure

This commit fixes a double of_node_put() issue in the error path of
devm_phy_create(). The scoped iterator for_each_child_of_node_scoped()
already handles dropping the node reference, so the explicit of_node_put()
was redundant.

> diff --git a/drivers/phy/renesas/phy-rcar-gen2.c b/drivers/phy/renesas/phy-rcar-gen2.c
> index 6c671254c6258..5a272e25e051c 100644
> --- a/drivers/phy/renesas/phy-rcar-gen2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen2.c
> @@ -405,7 +405,6 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
>  						   data->gen2_phy_ops);
>  			if (IS_ERR(phy->phy)) {
>  				dev_err(dev, "Failed to create PHY\n");

[Severity: High]
This is a pre-existing issue, but does this code leave a dangling pointer
when saving the node?

Looking at rcar_gen2_phy_probe(), we save np to channel->of_node:

	for_each_child_of_node_scoped(dev->of_node, np) {
		struct rcar_gen2_channel *channel = drv->channels + i;
		...
		channel->of_node = np;

Because for_each_child_of_node_scoped() automatically drops the reference on
the node when advancing or exiting, channel->of_node becomes a dangling pointer.

Later in rcar_gen2_phy_xlate(), this potentially dangling pointer is
used for comparison:

	if (np == drv->channels[i].of_node)

Could this falsely match a newly allocated node due to pointer aliasing if a
DT overlay is applied and removed? Should we call of_node_get() before
saving it to channel->of_node?


[Severity: High]
This is also a pre-existing issue, but is there a potential out-of-bounds write
if the number of child nodes changes?

Earlier in rcar_gen2_phy_probe(), the driver allocates the channels array based
on the child count:

	drv->num_channels = of_get_child_count(dev->of_node);
	drv->channels = devm_kcalloc(dev, drv->num_channels, ...);

Then it iterates over the children and increments i:

	for_each_child_of_node_scoped(dev->of_node, np) {
		struct rcar_gen2_channel *channel = drv->channels + i;
		...
		i++;
	}

If child nodes are added dynamically (e.g., via a DT overlay) between the call
to of_get_child_count() and the for_each_child_of_node_scoped() loop, could i
exceed drv->num_channels and cause an out-of-bounds write when accessing
drv->channels + i?

> -				of_node_put(np);
>  				return PTR_ERR(phy->phy);
>  			}
>  			phy_set_drvdata(phy->phy, phy);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

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