[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

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

[Linkui Xiao]

Hi Andrew,

Thanks for the review.

You are right - the ethtool patch was treating the symptom. I have split
this into two separate [PATCH net] threads: the EMAC driver fix, and a
reworked ethtool change that warns instead of hiding the bug.

 > 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.

Agreed, and EMAC is the broken one. emac_init_phy() handles a
"fixed-link" property by registering a fixed PHY for the MAC's own OF
node and then assigning the netdev pointer by hand:

		res = of_phy_register_fixed_link(np);
		ndev->phydev = of_phy_find_device(np);
		if (res || !ndev->phydev)
			return res ? res : -EINVAL;
		emac_adjust_link(dev->ndev);
		put_device(&ndev->phydev->mdio.dev);

Skipping phy_attach_direct() leaves phydev->attached_dev NULL, installs
no link change handler, keeps the PHY out of the netdev's
phy_link_topology, and the put_device() drops the reference the driver
just took. The user visible part is the missing topology entry:
ethnl_req_get_phydev() returns dev->phydev as-is for a request without a
phy-index, so phy_prepare_data() ends up doing xa_load() on a link_topo
that was never allocated.

The fix makes the fixed-link branch match what gianfar, fec, davinci_emac
and nixge already do: of_phy_register_fixed_link() followed by
of_phy_connect() on the MAC's own OF node (gemini uses
of_phy_get_and_connect() and stores the result, the same harmless shape
as the icssg users). The explicit put_device() goes away because
of_phy_connect() takes over the reference. The manual
emac_adjust_link() stays; the PHY state machine is still not started,
which is fine because a fixed link does not change and the driver
already treats dev->phy.address < 0 as "no PHY to poll". Full
rationale is in the commit message.

Cross compiled for powerpc with W=1 out of ppc44x_defconfig, no warnings.
No EMAC hardware here and no in-tree DTS pairs "ibm,emac" with
"fixed-link", so beyond the build it is inspection only. The hand
assignment goes back to 08e39982ef64, but it only started touching the
net_device with baab9de385a8, which is the Fixes: target.

Two adjacent issues I left out on purpose: emac_remove() never calls
phy_disconnect(), and the fixed-link branch never calls phy_read_status(),
so emac_adjust_link() copies SPEED_UNKNOWN/DUPLEX_UNKNOWN. Both predate
this patch and are worth separate fixes; happy to send them.

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

I went through every "->phydev =" in drivers/net/ and net/. EMAC is the
only driver that leaves net_device::phydev pointing at a PHY that was
never attached. The others are redundant but harmless:

   - icssg_prueth.c:425 and icssg_prueth_sr1.c:238 assign the result of
     of_phy_connect(), which already stored the same pointer via
     phy_attach_direct().
   - gemini.c:374 stores the result of of_phy_get_and_connect(), same
     shape.
   - net/dsa/user.c:2643 pre-fills user_dev->phydev from mdiobus_get_phy()
     before phylink_connect_phy() overwrites it with the same phy_device;
     mdiobus_get_phy() takes no reference, so nothing leaks.
   - Plain "phydev = NULL" right after a disconnect (gemini, rtsn, dpaa,
     icssg) clears what the disconnect already cleared.

Everything else called phydev in drivers/net/ is a private member of a
driver structure rather than net_device::phydev (tehuti, wangxun, r8169,
xgbe, qualcomm emac, tsnep, hns3, hibmcge, bgx, smsc95xx, oa_tc6,
dm9051, plus phylib internals and phylink's pl->phydev).  tsnep does
have a separate of_phy_find_device() reference leak, but that is
unrelated to net_device::phydev and not touched here.  So EMAC is the
only offender.

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

Fair, and that is why v2 no longer returns -EOPNOTSUPP silently. It
warns once and then returns -EOPNOTSUPP:

	if (WARN_ON_ONCE(!topo))
		return -EOPNOTSUPP;

phy_prepare_data() already returns -EOPNOTSUPP when the phydev is not in
the topology, so callers do not learn a new error code. The driver bug
still lands in the log with a stack trace, but a plain ethtool query no
longer takes the machine down. The dump side returns early when
dev->link_topo is NULL, so this can only fire from the doit path, at most
once per boot.

This is also the only spot in net/ethtool/ that touches ->link_topo
without coping with a missing one - phy_link_topo_empty() is "return
!dev->link_topo", phy_link_topo_get_phy()/get_port() return NULL, and
common.c, port.c, tsinfo.c all test it first. Before 9dd2ad5e92b9,
ethnl_phy_parse_request() had "if (!topo) return 0;" ahead of the xarray;
that check did not make it into phy_prepare_data().

If you would rather keep the oops as the canary, say the word and I will
drop the ethtool patch - the EMAC one is the part that actually fixes
something.

Both v2 patches drop Cc: stable: the EMAC one is latent in mainline, and
the ethtool one only warns. Say the word if you disagree.

Thanks,
Linkui