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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver saves a device node pointer (`np`) in `channel->of_node` without acquiring a reference, leading to a potential dangling pointer.
- [High] Potential out-of-bounds array access when populating the `drv->channels` array due to a time-of-check to time-of-use (TOCTOU) race condition.
- [High] The probe function incorrectly returns success (0) when a child node has an out-of-bounds 'reg' property.
--

--- Patch [1]: [PATCH] phy: renesas: rcar-gen2: Fix double of_node_put on phy creation failure ---
commit 007ce311e8760f22502bbc2b270c02df1d8b9ee0
Author: Felix Gu <ustc.gu@gmail.com>

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

This commit removes a redundant of_node_put() in the error path of
devm_phy_create(). The loop uses for_each_child_of_node_scoped(), which
automatically handles releasing the device node reference on scope exit.

> 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)

[Severity: High]
This is a pre-existing issue, but does this loop in rcar_gen2_phy_probe()
have a potential out-of-bounds array access?

The driver allocates the channels array based on of_get_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 child nodes:

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

If a child node is added concurrently to the device tree (e.g., via an
overlay) between the count and the loop, could i exceed the allocated size
of the array?

[Severity: High]
This is a pre-existing issue, but does this code save a device node pointer
without taking a reference?

Inside the same for_each_child_of_node_scoped() loop in
rcar_gen2_phy_probe():

    channel->of_node = np;

The iterator automatically drops the reference to np when advancing to the
next iteration or when the loop terminates. If the device tree overlay is
removed and the node is freed, could this result in a dangling pointer when
accessed later?

Should it use of_node_get() to increment the reference count?

[Severity: High]
This is a pre-existing issue, but can this error handling block return 0
instead of a proper error code?

When validating the "reg" property in rcar_gen2_phy_probe():

    error = of_property_read_u32(np, "reg", &channel_num);
    if (error || channel_num >= data->num_channels) {
        dev_err(dev, "Invalid \"reg\" property\n");
        return error;
    }

If of_property_read_u32() succeeds, error is set to 0. If the channel_num is
subsequently found to be out of bounds, the condition evaluates to true.

Would returning error here falsely signal to the driver core that the probe
succeeded?

>  						   data->gen2_phy_ops);
>  			if (IS_ERR(phy->phy)) {
>  				dev_err(dev, "Failed to create PHY\n");
> -				of_node_put(np);
>  				return PTR_ERR(phy->phy);
>  			}
>  			phy_set_drvdata(phy->phy, phy);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-rcar-gen2-v1-1-9aa35c36d7d7@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy