Re: [PATCH net-next v19 06/10] net: phy: phy_port: Store information about a port's upstream

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: phy: UAF in upstream_port when connect_phy fails
- [Low] Concern: phy_set_upstream_port()
  (drivers/net/phy/phy_device.c:3712-3722) writes phydev->upstream_port…

--

net: phy: phy_port: Store information about a port's upstream

This commit adds an upstream_port pointer to struct phy_port and struct
phy_device. For an MDI port that is fed by an MII port, such as an SFP
module port fed by the SFP cage port, the pointer records that MII port.
A new helper, phy_set_upstream_port(), is called from the SFP
connect_phy/disconnect_phy paths in both phy_device.c and phylink.c.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 13eb1d40badb5..e9da6767faaf4 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -3698,6 +3703,25 @@ struct phy_port *phy_get_sfp_port(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL_GPL(phy_get_sfp_port);
>  
> +/**
> + * phy_set_upstream_port() - Sets the phy_port controlling the MII this PHY is
> + *			     attached to.
> + * @phydev: pointer to the PHY device we set the upstream of.
> + * @port: The phy_port upstream of this PHY, can be NULL.
> + */
> +void phy_set_upstream_port(struct phy_device *phydev, struct phy_port *port)
> +{
> +	struct phy_port *local_port;
> +
> +	ASSERT_RTNL();
> +
> +	phydev->upstream_port = port;
> +
> +	phy_for_each_port(phydev, local_port)
> +		local_port->upstream_port = port;
> +}

[Severity: Low]
Can phydev->upstream_port and port->upstream_port get out of sync here?
This helper only updates the ports that are on phydev->ports when it
runs.

Ports added later through phy_probe()->phy_setup_ports()->phy_add_port()
do not copy phydev->upstream_port:

drivers/net/phy/phy_device.c:phy_add_port() {
    ...
	phy_port_update_supported(port);

	list_add(&port->head, &phydev->ports);

	phydev->n_ports++;
    ...
}

This can happen if the module PHY driver binds after connect. The
phy_sfp_connect_phy() path has no phy->drv check. It can also happen if
the driver is unbound and rebound while the PHY is connected. In both
cases phydev->upstream_port still points at the SFP cage port, but the
new ports have upstream_port == NULL.

Today the mismatch is not visible. Nothing in the series reads
phydev->upstream_port. The only reader of port->upstream_port is
port_prepare_data() in net/ethtool/port.c, and it only sees ports in
the link topology. Ports created later by phy_add_port() are never
added to the topology.

Would it make sense for phy_add_port() to set port->upstream_port from
phydev->upstream_port, so that both fields stay consistent for future
users?

> +EXPORT_SYMBOL_GPL(phy_set_upstream_port);
> +
>  /**
>   * fwnode_mdio_find_device - Given a fwnode, find the mdio_device
>   * @fwnode: pointer to the mdio_device's fwnode

[ ... ]

> @@ -1403,6 +1403,7 @@ static int phy_sfp_connect_phy(void *upstream, struct phy_device *phy)
>  	int ret;
>  
>  	phydev->has_sfp_mod_phy = true;
> +	phy_set_upstream_port(phy, phydev->sfp_cage_port);
>  
>  	/* If we aren't attached to a netdev, we can't add the SFP PHY to its
>  	 * topology.

[Severity: High]
If a later step in the connect path fails, do phy->upstream_port and
the module PHY's port->upstream_port pointers stay pointing at the cage
port?

phy_sfp_connect_phy() calls phy_set_upstream_port() before
phy_link_topo_add_phy(). When phy_link_topo_add_phy() fails, it only
resets phydev->has_sfp_mod_phy before returning the error.

phylink_sfp_connect_phy() follows the same pattern. It sets the
upstream port and then returns whatever phylink_sfp_config_phy()
returns. That call can fail in phylink_validate(),
phylink_sfp_select_interface(), phylink_attach_phy() or
phylink_bringup_phy().

The sfp-bus core does not call ->disconnect_phy() when ->connect_phy()
fails, so the NULL reset added to phy_sfp_disconnect_phy() and
phylink_sfp_disconnect_phy() does not cover this case.

When the module PHY is first probed through sfp_sm_probe_phy(), a
failure destroys the PHY, so the stale pointer goes away with it.
However, if I read sfp_register_bus() correctly, ->connect_phy() is
also reached when the upstream registers on a bus that already has a
module PHY in bus->phydev. The upstream here is a PHY driver via
phy_sfp_probe(), or phylink.

In that case the error goes back to the upstream, which then tears
down and frees its sfp_cage_port. Meanwhile bus->phydev stays alive on
the SFP bus. Its upstream_port, and the upstream_port of each of its
ports, still point at the freed phy_port. Anything that later follows
port->upstream_port, such as the ethtool port code reading
upstream_port->id later in this series, would then hit a
use-after-free.

Could phy_set_upstream_port(phy, NULL) be added to the error paths of
both connect callbacks? Or, if the pointer does not need to be valid
while phy_link_topo_add_phy() and phylink_sfp_config_phy() run, could
the assignment simply move after those calls succeed?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016133107.54054-1-maxime.chevallier%40bootlin.com