Re: [PATCH net-next v19 02/13] phylib: Add support for PHYs with broken forced mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: C45 MDIO writes falsely trigger C22 register handlers corrupting state
- [High] phylib: NULL pointer deref due to TOCTOU race on phydev->drv
--

commit f3ec63e34d550025dac6b66482995851d8d99969
Author: Birger Koblitz <mail@birger-koblitz.de>

phylib: Add support for PHYs with broken forced mode

This commit adds support for PHYs which only support auto-negotiated speed
modes. This prevents a kernel warning and stack-trace within the
phy_state_machine when PHYs return -EOPNOTSUPP from config_aneg() because
they do not support forced modes.

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

[ ... ]

> @@ -354,6 +354,9 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>  			switch (devad) {
>  			case MII_BMCR:
>  				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
> +					if (phydev->drv &&
> +					    (phydev->drv->flags & PHY_BROKEN_FORCED))
> +						return -EOPNOTSUPP;

[Severity: High]
Can this unlocked check on phydev->drv lead to a NULL pointer dereference?

If a concurrent sysfs unbind calls phy_remove() and sets phydev->drv = NULL
without holding rtnl_lock or phydev->lock, the compiler might fetch
phydev->drv to verify it is non-NULL, and then fetch it again to read flags.
This double-fetch could result in a NULL pointer dereference if the pointer
is cleared between the two operations.

Also, for C45 accesses in phy_mii_ioctl(), devad holds the MMD address but
is matched against C22 register definitions like MII_BMCR (which is 0). 

Does adding this early return mean a C45 write to MMD 0 (with zeroed reset
and anenable bits) will now incorrectly hit this C22 handler and fail with
-EOPNOTSUPP?

[ ... ]

> @@ -1190,6 +1193,10 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  	      duplex != DUPLEX_FULL)))
>  		return -EINVAL;
>  
> +	if (autoneg == AUTONEG_DISABLE && phydev->drv &&
> +	    (phydev->drv->flags & PHY_BROKEN_FORCED))
> +		return -EOPNOTSUPP;

[Severity: High]
Does this present the same time-of-check to time-of-use race as above?

A sysfs unbind could set phydev->drv to NULL between the NULL check and the
flags dereference, since ethtool operations and unbind operations do not
share a common lock here.

> +
>  	mutex_lock(&phydev->lock);
>  	phydev->autoneg = autoneg;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ax88179a-v19-0-7475173cfb56@birger-koblitz.de?part=2