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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] phy: TOCTOU race allowing NULL pointer dereference in phy_mii_ioctl and phy_ethtool_ksettings_set
- [Medium] net: phy: data race modifying bitfields in phy_mii_ioctl
- [Medium] net: phy: SIOCSMIIREG C45 writes misinterpret MMDs as C22 registers
--

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

phylib: Add support for PHYs with broken forced mode

This adds support for PHYs which only support auto-negotiated speed modes,
preventing a kernel warning and stack-trace when PHYs return -EOPNOTSUPP
from config_aneg() for forced modes.

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index fce9bc7be3304..079c53516f424 100644
> --- 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:

[Severity: Medium]
Does this switch statement evaluate Clause 45 MMDs against Clause 22
register macros?

In phy_mii_ioctl(), devad contains the MMD for C45 writes:

	case SIOCSMIIREG:
		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
			prtad = mdio_phy_id_prtad(mii_data->phy_id);
			devad = mdio_phy_id_devad(mii_data->phy_id);

If a user issues an SIOCSMIIREG ioctl targeting a Clause 45 PHY and
writes to MMD 4 (PHY XS), devad will be 4. Doesn't this hit the
MII_ADVERTISE case below, incorrectly corrupting phydev->advertising?

Additionally, will the newly added logic here improperly intercept C45 writes
to MMD 0?

>  				if ((val & (BMCR_RESET | BMCR_ANENABLE)) == 0) {
> +					if (phydev->drv &&
> +					    (phydev->drv->flags & PHY_BROKEN_FORCED))

[Severity: High]
Can the compiler double-fetch phydev->drv here without locks, risking a NULL
pointer dereference?

If an asynchronous driver unbind occurs via sysfs:

echo <phy_id> > /sys/bus/mdio_bus/drivers/<drv>/unbind

This calls phy_remove(), which clears phydev->drv to NULL without acquiring
phydev->lock. Could the second fetch of phydev->drv in the condition above
yield NULL, leading to a kernel panic when dereferencing flags?

> +						return -EOPNOTSUPP;
>  					if (phydev->autoneg == AUTONEG_ENABLE)
>  						change_autoneg = true;
>  					phydev->autoneg = AUTONEG_DISABLE;

[Severity: Medium]
Is it safe to modify phydev->autoneg and phydev->duplex locklessly here?

These fields are part of a bitfield block in struct phy_device:

	unsigned autoneg:1;
	unsigned link:1;
	unsigned autoneg_complete:1;

phy_state_machine() modifies adjacent bitfields (like link and
autoneg_complete) asynchronously while holding phydev->lock. Because they
share the same memory word, could this lockless assignment in phy_mii_ioctl()
cause a read-modify-write race that silently overwrites the state machine's
updates?

[ ... ]
> @@ -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))

[Severity: High]
Similar to the check in phy_mii_ioctl(), can the compiler double-fetch
phydev->drv here before phydev->lock is taken?

If phy_remove() clears phydev->drv concurrently, could the second load yield
NULL and cause a dereference crash?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-ax88179a-v18-0-6fa1d4c40422@birger-koblitz.de?part=2