Re: [PATCH v17 1/5] phy: core: Fix use-after-free in phy_get paths

[Manivannan Sadhasivam]

On Sun, Sep 06, 2026 at 03:17:05PM +0100, Bryan O'Donoghue wrote:
> Sashiko asked during a patch review if the existing usage pattern had a
> race condition; specifically in of_phy_get() if it was possible between
> returning from _of_phy_get() and running try_module_get() that a module
> might be unbound leading to use-after-free.
> 
> Looking at the code this appears to be so, there is no linkage between the
> phy and module under a synchronisation primitive.
> 
> Using the phy_provider_mutex in phy_get() will ensure there is a link between
> the returned phy pointer and the module_get() bumping the module reference
> count.
> 
> Amend phy_get(), of_phy_get() and devm_of_phy_get_by_index() to fix the
> same usage pattern.
> 
> phy_provider_unregister() must take the phy_provider_mutex so amending
> phy_get()/of_phy_get() to take that same mutex guarantees there is no
> use-after-free.
> 
> Fixes: ff764963479a1 ("drivers: phy: add generic PHY framework")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> ---
>  drivers/phy/phy-core.c | 45 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 21aaf2f76e53e..89addd732bff3 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -124,13 +124,13 @@ static struct phy *phy_find(struct device *dev, const char *con_id)
>  	const char *dev_id = dev_name(dev);
>  	struct phy_lookup *p, *pl = NULL;
>  
> -	mutex_lock(&phy_provider_mutex);
> +	lockdep_assert_held(&phy_provider_mutex);
> +
>  	list_for_each_entry(p, &phys, node)
>  		if (!strcmp(p->dev_id, dev_id) && !strcmp(p->con_id, con_id)) {
>  			pl = p;
>  			break;
>  		}
> -	mutex_unlock(&phy_provider_mutex);
>  
>  	return pl ? pl->phy : ERR_PTR(-ENODEV);
>  }
> @@ -624,6 +624,8 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
>  	struct phy *phy = NULL;
>  	struct of_phandle_args args;
>  
> +	lockdep_assert_held(&phy_provider_mutex);
> +
>  	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells",
>  		index, &args);
>  	if (ret)
> @@ -635,11 +637,10 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
>  		goto out_put_node;
>  	}
>  
> -	mutex_lock(&phy_provider_mutex);
>  	phy_provider = of_phy_provider_lookup(args.np);
>  	if (IS_ERR(phy_provider) || !try_module_get(phy_provider->owner)) {
>  		phy = ERR_PTR(-EPROBE_DEFER);
> -		goto out_unlock;
> +		goto out_put_node;
>  	}
>  
>  	if (!of_device_is_available(args.np)) {
> @@ -653,8 +654,6 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
>  out_put_module:
>  	module_put(phy_provider->owner);
>  
> -out_unlock:
> -	mutex_unlock(&phy_provider_mutex);
>  out_put_node:
>  	of_node_put(args.np);
>  
> @@ -678,15 +677,21 @@ struct phy *of_phy_get(struct device_node *np, const char *con_id)
>  	if (con_id)
>  		index = of_property_match_string(np, "phy-names", con_id);
>  
> +	mutex_lock(&phy_provider_mutex);
> +

I was tempted to suggest using "guard(mutex)(&phy_provider_mutex)" in all 3
functions, but then realised that this patch is pointing the Fixes commit dating
back to the origin of the PHY framework. So cleanup.h won't be available and
backporting to older kernels won't be possible. So manual lock/unlock is OK
here.

>  	phy = _of_phy_get(np, index);
>  	if (IS_ERR(phy))
> -		return phy;
> +		goto out_unlock;
>  
> -	if (!try_module_get(phy->ops->owner))
> -		return ERR_PTR(-EPROBE_DEFER);
> +	if (!try_module_get(phy->ops->owner)) {
> +		phy = ERR_PTR(-EPROBE_DEFER);
> +		goto out_unlock;
> +	}
>  
>  	get_device(&phy->dev);
>  
> +out_unlock:
> +	mutex_unlock(&phy_provider_mutex);
>  	return phy;
>  }
>  EXPORT_SYMBOL_GPL(of_phy_get);
> @@ -786,6 +791,7 @@ struct phy *phy_get(struct device *dev, const char *string)
>  	struct phy *phy;
>  	struct device_link *link;
>  
> +	mutex_lock(&phy_provider_mutex);
>  	if (dev->of_node) {
>  		if (string)
>  			index = of_property_match_string(dev->of_node, "phy-names",
> @@ -796,15 +802,18 @@ struct phy *phy_get(struct device *dev, const char *string)
>  	} else {
>  		if (string == NULL) {
>  			dev_WARN(dev, "missing string\n");
> -			return ERR_PTR(-EINVAL);
> +			phy = ERR_PTR(-EINVAL);
> +			goto out_unlock;
>  		}
>  		phy = phy_find(dev, string);
>  	}
>  	if (IS_ERR(phy))
> -		return phy;
> +		goto out_unlock;
>  
> -	if (!try_module_get(phy->ops->owner))
> -		return ERR_PTR(-EPROBE_DEFER);
> +	if (!try_module_get(phy->ops->owner)) {
> +		phy = ERR_PTR(-EPROBE_DEFER);
> +		goto out_unlock;
> +	}
>  
>  	get_device(&phy->dev);

You could've dropped the mutex here itself instead of covering the whole
function. But that's not a deal breaker here. So,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>

- Mani

-- 
மணிவண்ணன் சதாசிவம்