Re: [PATCH net v2] net: ethtool: fix NULL pointer dereference in phy_reply_size

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 07/05/2026 15:17, Quan Sun wrote:
> In phy_prepare_data(), several strings such as 'name', 'drvname',
> 'upstream_sfp_name', and 'downstream_sfp_name' are allocated using
> kstrdup(). However, these allocations were not checked  for failure.
> 
> If kstrdup() fails for 'name', it returns NULL while the function
> continues. This leads to a kernel NULL pointer dereference and panic
> later in phy_reply_size() when it unconditionally calls strlen() on
> the NULL pointer.
> 
> While other strings like 'upstream_sfp_name' might be checked before
> access in certain code paths, failing to handle these allocations
> consistently can lead to incomplete data reporting or hidden bugs.
> 
> Fix this by adding proper NULL checks for all kstrdup() calls in
> phy_prepare_data() and implement a centralized error handling path
> using goto labels to ensure all previously allocated resources are
> freed on failure.
> 
> Fixes: 9dd2ad5e92b9 ("net: ethtool: phy: Convert the PHY_GET command to generic phy dump")
> Signed-off-by: Quan Sun <2022090917019@std.uestc.edu.cn>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

> ---
> Changes in v2:
> - Add Fixes: tag.
> - Expand the fix to cover all kstrdup() allocations in the function.
> - Use goto labels for a cleaner and more robust error handling path.
> ---
>  net/ethtool/phy.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
> index d4e6887055ab1..f76d94d848d6d 100644
> --- a/net/ethtool/phy.c
> +++ b/net/ethtool/phy.c
> @@ -76,6 +76,7 @@ static int phy_prepare_data(const struct ethnl_req_info *req_info,
>  	struct nlattr **tb = info->attrs;
>  	struct phy_device_node *pdn;
>  	struct phy_device *phydev;
> +	int ret;
>  
>  	/* RTNL is held by the caller */
>  	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PHY_HEADER,
> @@ -88,8 +89,17 @@ static int phy_prepare_data(const struct ethnl_req_info *req_info,
>  		return -EOPNOTSUPP;
>  
>  	rep_data->phyindex = phydev->phyindex;
> +
>  	rep_data->name = kstrdup(dev_name(&phydev->mdio.dev), GFP_KERNEL);
> +	if (!rep_data->name)
> +		return -ENOMEM;
> +
>  	rep_data->drvname = kstrdup(phydev->drv->name, GFP_KERNEL);
> +	if (!rep_data->drvname) {
> +		ret = -ENOMEM;
> +		goto err_free_name;
> +	}
> +
>  	rep_data->upstream_type = pdn->upstream_type;
>  
>  	if (pdn->upstream_type == PHY_UPSTREAM_PHY) {
> @@ -97,15 +107,33 @@ static int phy_prepare_data(const struct ethnl_req_info *req_info,
>  		rep_data->upstream_index = upstream->phyindex;
>  	}
>  
> -	if (pdn->parent_sfp_bus)
> +	if (pdn->parent_sfp_bus) {
>  		rep_data->upstream_sfp_name = kstrdup(sfp_get_name(pdn->parent_sfp_bus),
>  						      GFP_KERNEL);
> +		if (!rep_data->upstream_sfp_name) {
> +			ret = -ENOMEM;
> +			goto err_free_drvname;
> +		}
> +	}
>  
> -	if (phydev->sfp_bus)
> +	if (phydev->sfp_bus) {
>  		rep_data->downstream_sfp_name = kstrdup(sfp_get_name(phydev->sfp_bus),
>  							GFP_KERNEL);
> +		if (!rep_data->downstream_sfp_name) {
> +			ret = -ENOMEM;
> +			goto err_free_upstream_sfp;
> +		}
> +	}
>  
>  	return 0;
> +
> +err_free_upstream_sfp:
> +	kfree(rep_data->upstream_sfp_name);
> +err_free_drvname:
> +	kfree(rep_data->drvname);
> +err_free_name:
> +	kfree(rep_data->name);
> +	return ret;
>  }
>  
>  static int phy_fill_reply(struct sk_buff *skb,