netdev - Re: [net-next,v3,06/11] net: phy: Represent PHY-less SFP modules with phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20260203195710.2176606-1-horms@kernel.org>
Date: Tue,  3 Feb 2026 19:57:10 +0000
From: Simon Horman <horms@...nel.org>
To: maxime.chevallier@...tlin.com
Cc: Simon Horman <horms@...nel.org>,
	netdev@...r.kernel.org,
	andrew@...n.ch,
	pabeni@...hat.com,
	linux-kernel@...r.kernel.org,
	nicveronese@...il.com,
	kory.maincent@...tlin.com,
	f.fainelli@...il.com,
	o.rempel@...gutronix.de,
	kuba@...nel.org,
	mwojtas@...omium.org,
	daniel@...rotopia.org,
	davem@...emloft.net,
	dimitri.fedrau@...bherr.com,
	edumazet@...gle.com,
	romain.gantois@...tlin.com,
	vladimir.oltean@....com,
	kabel@...nel.org,
	christophe.leroy@...roup.eu,
	thomas.petazzoni@...tlin.com,
	herve.codina@...tlin.com,
	linux@...linux.org.uk,
	hkallweit1@...il.com
Subject: Re: [net-next,v3,06/11] net: phy: Represent PHY-less SFP modules with phy_port

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: phy: Represent PHY-less SFP modules with phy_port

Now that the SFP bus infrastructure notifies when PHY-less modules are
connected, we can create a phy_port to represent it. Instead of letting
the SFP subsystem handle that, the Bus' upstream is in charge of
maintaining that phy_port and register it to the topology, as the
upstream (in this case a phy device) is directly interacting with the
underlying net_device.

Add a phy_caps helper to get the achievable modes on this module based
on what the phy_port representing the bus supports.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -1609,6 +1613,74 @@ static void phy_sfp_link_down(void *upstream)
> +static int phy_add_sfp_mod_port(struct phy_device *phydev)
> +{
> +	const struct sfp_module_caps *caps;
> +	struct phy_port *port;
> +	int ret = 0;
> +
> +	/* Create mod port */
> +	port = phy_port_alloc();
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->active = true;
> +
> +	caps = sfp_get_module_caps(phydev->sfp_bus);
> +
> +	phy_caps_linkmode_filter_ifaces(port->supported, caps->link_modes,
> +					phydev->sfp_bus_port->interfaces);

At line 1638, phy_add_sfp_mod_port() dereferences phydev->sfp_bus_port
without checking for NULL. Can this be NULL here?

Looking at the initialization in phy_sfp_probe(), when sfp_bus_add_upstream()
fails, the error path frees the port but then unconditionally stores it in
phydev->sfp_bus_port:

> @@ -1700,7 +1774,7 @@ static struct phy_port *phy_setup_sfp_port(struct phy_device *phydev)
>  static int phy_sfp_probe(struct phy_device *phydev)
>  {
> -	struct phy_port *port;
> +	struct phy_port *port = NULL;
>  	struct sfp_bus *bus;
>  	int ret;
>
> @@ -1729,6 +1803,8 @@ static int phy_sfp_probe(struct phy_device *phydev)
>  		phy_port_destroy(port);
>  	}
>
> +	phydev->sfp_bus_port = port;
> +
>  	return ret;
>  }

If sfp_bus_add_upstream() returns an error at line 1800, the cleanup path at
line 1802 checks 'ret && port', then calls phy_del_port() and
phy_port_destroy() to free the port. However, line 1806 still executes
'phydev->sfp_bus_port = port', which assigns the freed pointer to the
phydev structure.

When phy_sfp_probe() returns an error, probe fails and the PHY device is not
properly initialized. Later, if a PHY-less SFP module is inserted, the SFP
subsystem calls phy_sfp_module_start(), which calls phy_add_sfp_mod_port(),
which dereferences phydev->sfp_bus_port->interfaces at line 1638.

Should the assignment at line 1806 be conditional, or should port be set to
NULL after freeing?