Re: [PATCH v13 1/5] phy: core: Add phy_get_by_of_node()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly introduced phy_get_by_of_node() API bypasses the usb-nop-xceiv compatibility check, leading to infinite probe deferral.

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition during PHY lookup returns an unpinned pointer, allowing concurrent PHY destruction to cause a Use-After-Free.
--

--- Patch [1]: [PATCH v13 1/5] phy: core: Add phy_get_by_of_node() ---
commit 2964f1957e7138a13d72185f47a72cbb1e4e193c
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

phy: core: Add phy_get_by_of_node()

Add new function phy_get_by_of_node() allowing lookup of a phy by
device_node. Separates existing logic in _of_phy_get() into an internal
helper method _of_phy_get_with_args() to allow for reuse in new method.

> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 21aaf2f76e53e..ebfad5325072e 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
[ ... ]
> @@ -607,54 +607,69 @@ int phy_validate(struct phy *phy, enum phy_mode mode, int submode,
[ ... ]
> +static struct phy *_of_phy_get(struct device_node *np, int index)
> +{
> +	int ret;
> +	struct phy *phy;
> +	struct of_phandle_args args;
> +
> +	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells",
> +		index, &args);
> +	if (ret)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* This phy type handled by the usb-phy subsystem for now */
> +	if (of_device_is_compatible(args.np, "usb-nop-xceiv")) {
> +		phy = ERR_PTR(-ENODEV);
> +		goto out_put_node;
> +	}
> +
> +	phy = _of_phy_get_with_args(&args);

[Severity: High]
If a caller uses the new phy_get_by_of_node() API on a device node compatible
with "usb-nop-xceiv", will it bypass this check? 

Because the compatibility check was left here in _of_phy_get() instead of
being moved into the shared _of_phy_get_with_args() helper, it appears the
new API will skip this fallback and could enter an infinite probe deferral
instead of correctly returning -ENODEV.

[ ... ]
> @@ -986,6 +1001,34 @@ struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
>  }
>  EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
>  
> +/**
> + * phy_get_by_of_node() - lookup and obtain a reference to a phy by device_node
> + * @np: node containing the phy
> + *
> + * Returns the phy associated with the device node or ERR_PTR.
> + */
> +struct phy *phy_get_by_of_node(struct device_node *np)
> +{
> +	struct of_phandle_args args = { .np = np, .args_count = 0 };
> +	struct phy *phy;
> +
> +	if (!np)
> +		return ERR_PTR(-EINVAL);
> +
> +	phy = _of_phy_get_with_args(&args);
> +
> +	if (IS_ERR(phy))
> +		return phy;
> +
> +	if (!try_module_get(phy->ops->owner))

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
here between retrieving the phy and pinning its module?

The _of_phy_get_with_args() helper retrieves the phy pointer via the
provider's of_xlate callback. Typical callbacks (like of_phy_simple_xlate())
drop the PHY device reference before returning. _of_phy_get_with_args() then
unlocks phy_provider_mutex and drops the provider's module reference.

Could a concurrent thread unbinding the PHY provider destroy the PHY object
before this caller evaluates try_module_get(phy->ops->owner), resulting in a
use-after-free?

> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	get_device(&phy->dev);
> +
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(phy_get_by_of_node);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v13-0-ae811e2f0799@linaro.org?part=1

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