Re: [PATCH v2 3/3] phy: renesas: rcar-gen2: Use dev_err_probe() in probe

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

Pre-existing issues:
- [High] The PHY provider is registered before the driver data is set, creating a race window where PHY lookup can fail.
- [High] The driver saves a scoped device node pointer into a persistent structure without incrementing its reference count, leading to a dangling pointer.
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race condition with device tree modifications can lead to an array out-of-bounds access.
--

--- Patch [3]: [PATCH v2 3/3] phy: renesas: rcar-gen2: Use dev_err_probe() in probe ---
commit 709d9b8218087abc6526b29b382856eb53ccb3ba
Author: Felix Gu <[email protected]>

    phy: renesas: rcar-gen2: Use dev_err_probe() in probe
    
    Convert the error paths in rcar_gen2_phy_probe() to dev_err_probe().

> diff --git a/drivers/phy/renesas/phy-rcar-gen2.c b/drivers/phy/renesas/phy-rcar-gen2.c
> index b18727ed41a10..581f6768e2bb2 100644
> --- a/drivers/phy/renesas/phy-rcar-gen2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen2.c
[ ... ]
> @@ -388,10 +384,9 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
>  		channel->selected_phy = -1;

[Severity: High]
This isn't a bug introduced by this patch, but could the assignment to
channel->of_node just above this line result in a dangling pointer?

Looking at rcar_gen2_phy_probe(), the loop uses
for_each_child_of_node_scoped(dev->of_node, np) which automatically
decrements the reference count of np at the end of each iteration.

Since channel->of_node = np stores this pointer into the persistent
channel structure without calling of_node_get(np), wouldn't this leave
channel->of_node pointing to freed memory once the scope ends?

[Severity: High]
This isn't a bug introduced by this patch, but is there a risk of an
out-of-bounds array access in this loop?

Earlier in rcar_gen2_phy_probe(), drv->channels is allocated based on
of_get_child_count():

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

If a dynamic device tree overlay adds a child node concurrently before or
during the for_each_child_of_node_scoped() loop, the loop will iterate
more times than drv->num_channels. Would the assignment
channel = drv->channels + i then write past the end of the allocation?

>  		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 ?: -EINVAL;
> -		}
> +		if (error || channel_num >= data->num_channels)
> +			return dev_err_probe(dev, error ?: -EINVAL,
> +					     "Invalid \"reg\" property\n");
>  		channel->select_mask = select_mask[channel_num];
[ ... ]
> @@ -414,10 +408,9 @@ static int rcar_gen2_phy_probe(struct platform_device *pdev)
>  	}
>  
>  	provider = devm_of_phy_provider_register(dev, rcar_gen2_phy_xlate);
> -	if (IS_ERR(provider)) {
> -		dev_err(dev, "Failed to register PHY provider\n");
> -		return PTR_ERR(provider);
> -	}
> +	if (IS_ERR(provider))
> +		return dev_err_probe(dev, PTR_ERR(provider),
> +				     "Failed to register PHY provider\n");
>  
>  	dev_set_drvdata(dev, drv);

[Severity: High]
This isn't a bug introduced by this patch, but does the registration order
create a race condition?

The PHY provider is registered via devm_of_phy_provider_register() before
dev_set_drvdata(dev, drv) is called.

If a concurrent deferred probe or module load calls rcar_gen2_phy_xlate()
during this window, it will try to access driver data that hasn't been set
yet. Should dev_set_drvdata() be called before exposing the PHY provider
to the system?

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

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