Re: [PATCH v2 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Error paths in `stm32_usb2phy_enable()` and `stm32_usb2phy_disable()` fail to unwind the `en_refcnt` atomic counter, leading to state corruption and leaked resources.
- [High] Incorrect handling of `devm_regulator_get_optional()` return value unconditionally fails probe on systems without the regulator.
- [High] Lockless `en_refcnt` check introduces a TOCTOU race condition allowing concurrent callers to bypass hardware initialization.
- [Medium] Unchecked dereferences of `dev->of_node` and `phy_dev->hw_data` during probe allow a local Denial of Service (Oops) if bound without a Device Tree node.
--

--- Patch [2]: [PATCH v2 2/9] phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY ---
commit e1cc42ded5599cd15a5c991b72a87e2e909f244d
Author: Pankaj Dev <[email protected]>

phy: stm32: Add support for ST STM32MP25 USB2-FEMTO PHY

Add USB2 PHY driver for STM32MP25 USB2 controllers, which includes the
USB2.0 host-only controller and USB 2.0 part of the DWC3 controller.
Two such PHYs in total are present in STM32MP25 SoC.

> diff --git a/drivers/phy/st/phy-stm32-usb2phy.c b/drivers/phy/st/phy-stm32-usb2phy.c
> new file mode 100644
> index 0000000000000..a5cc7b855c61f
> --- /dev/null
> +++ b/drivers/phy/st/phy-stm32-usb2phy.c
> @@ -0,0 +1,361 @@
[ ... ]
> +static int stm32_usb2phy_enable(struct stm32_usb2phy *phy_dev)
> +{
> +	const struct stm32mp2_usb2phy_hw_data *phy_data = phy_dev->hw_data;
> +	unsigned long rate;
> +	int refsel, ret;
> +
> +	/* Check if a phy is already init or clk48 in use */
> +	if (atomic_inc_return(&phy_dev->en_refcnt) > 1)
> +		return 0;

[Severity: High]
Is it safe to rely on this lockless check?

If two threads call stm32_usb2phy_enable() concurrently (for instance, via
phy_init and clk_prepare), the second thread will increment en_refcnt to 2
and return success immediately.

Will the second thread then proceed to use the hardware before the first
thread has actually finished enabling the regulators and clocks?

> +
> +	rate = clk_get_rate(phy_dev->phyref);
> +	if (rate == 19200000)
> +		refsel = 0;
> +	else if (rate == 20000000)
> +		refsel = 1;
> +	else if (rate == 24000000)
> +		refsel = 2;
> +	else
> +		return -EINVAL;

[Severity: High]
Does this error path leak the en_refcnt?

If we return here, the counter remains incremented. Subsequent calls to
stm32_usb2phy_enable() will then see a value > 1 and instantly return
success without initializing the hardware.

[ ... ]
> +static int stm32_usb2phy_disable(struct stm32_usb2phy *phy_dev)
> +{
> +	int ret;
> +
> +	/* Check if a phy is still init or clk48 in use */
> +	if (atomic_dec_return(&phy_dev->en_refcnt) > 0)
> +		return 0;
> +
> +	ret = reset_control_assert(phy_dev->rstc);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this error path need to restore the en_refcnt?

If reset_control_assert() fails, we return without disabling the clock
or regulator, and the counter remains decremented.

[ ... ]
> +static int stm32_usb2phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	phy_dev->vdd33 = devm_regulator_get_optional(dev, "vdd33");
> +	if (IS_ERR(phy_dev->vdd33))
> +		return dev_err_probe(dev, PTR_ERR(phy_dev->vdd33), "Failed to get vdd3v3 supply\n");

[Severity: High]
If this regulator is truly optional, shouldn't we handle the -ENODEV return
value gracefully instead of failing the probe unconditionally?

If it's missing from the Device Tree, devm_regulator_get_optional() will
return -ENODEV and cause the probe to fail on valid hardware configurations.

> +
> +	phy_dev->regmap = syscon_node_to_regmap(dev->of_node->parent);

[Severity: Medium]
Can dev->of_node be NULL here?

If a user manually instantiates the device via sysfs or software nodes
without an associated Device Tree node, this will result in a NULL pointer
dereference when accessing parent.

> +	if (IS_ERR(phy_dev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(phy_dev->regmap), "Failed to get regmap\n");
> +
> +	offset = of_get_address(dev->of_node, 0, NULL, NULL);
> +	if (!offset)
> +		return dev_err_probe(dev, -EINVAL, "Failed to get regmap offset\n");
> +
> +	phy_dev->cr_offset = be32_to_cpu(*offset);
> +
> +	phy_dev->hw_data = device_get_match_data(dev);

[Severity: Medium]
Should we check if phy_dev->hw_data is NULL before proceeding?

In a scenario without a Device Tree node, device_get_match_data() will
return NULL, which is later dereferenced in the enable and disable paths.

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

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