[PATCH net] net: ethtool: phy: fix NULL link_topo dereference in phy_prepare_data()

[PATCH net] net: ethtool: phy: fix NULL link_topo dereference in phy_prepare_data()

[Linkui Xiao]

phy_prepare_data() picks up the link topology of the target netdev and
dereferences it without any NULL check:

	struct phy_link_topology *topo = reply_data->dev->link_topo;
	...
	pdn = xa_load(&topo->phys, phydev->phyindex);

The phydev it looks up comes from ethnl_req_get_phydev(), which returns
dev->phydev as-is when the request carries no phy-index, without ever
consulting the topology. A driver that assigns netdev->phydev directly
instead of going through phy_attach_direct() therefore ends up with a
non-NULL phydev and a NULL link_topo, and ETHTOOL_MSG_PHY_GET on such a
device dereferences NULL. The fixed-link path of the IBM EMAC driver does
exactly that.

This is a regression from the conversion of PHY_GET to the generic phy
dump infrastructure: the code it replaced bailed out of
ethnl_phy_parse_request() with "if (!topo) return 0;" before touching
the xarray, and that guard was dropped by the rewrite.

Restore the check and answer -EOPNOTSUPP, which is what
phy_prepare_data() already returns when the phydev cannot be found in
the topology.

Reaching the crash needs an IBM EMAC device with a fixed-link node, which
was not available here, so the fix is verified by compilation and code
inspection only.

This was found during an audit of net/ethtool/ performed with the help
of an AI coding assistant (Codex), which also proposed the analysis and
the fix. I verified the Fixes commit and both the doit and the dump call
paths.

Fixes: 9dd2ad5e92b9 ("net: ethtool: phy: Convert the PHY_GET command to generic phy dump")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@126.com>
---
 net/ethtool/phy.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index 018b0412be86..60a617d9a332 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -83,6 +83,9 @@ static int phy_prepare_data(const struct ethnl_req_info *req_info,
 	if (IS_ERR_OR_NULL(phydev))
 		return -EOPNOTSUPP;
 
+	if (!topo)
+		return -EOPNOTSUPP;
+
 	pdn = xa_load(&topo->phys, phydev->phyindex);
 	if (!pdn)
 		return -EOPNOTSUPP;
-- 
2.25.1

Re: [PATCH net] net: ethtool: phy: fix NULL link_topo dereference in phy_prepare_data()

[Andrew Lunn]

> Reaching the crash needs an IBM EMAC device with a fixed-link node, which
> was not available here, so the fix is verified by compilation and code
> inspection only.

There is another way to look at this. The MAC driver might be broken,
it is assigning ndev->phydev, which it should not. It needs to call
one of the helpers that get to phy_attach_direct(), which sets
ndev->phydev.

Are there other MAC drivers which assign ndev->phydev?

This NULL pointer dereference can be considered good, because it might
be pointing out a bug. So we might want to keep it.

	Andrew