Re: [PATCH net-next v17 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] phylib: NULL pointer dereference in phy_ethtool_ksettings_set()
- [Medium] phylib: Userspace can bypass PHY_BROKEN_FORCED via SIOCSMIIREG
--

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

    phylib: Add support for PHYs with broken forced mode
    
    Add support for PHYs which only supported 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
> index fce9bc7be3304..b83f938fe7c3b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1190,6 +1190,10 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
>  	      duplex != DUPLEX_FULL)))
>  		return -EINVAL;
>  
> +	if (autoneg == AUTONEG_DISABLE &&
> +	    (phydev->drv->flags & PHY_BROKEN_FORCED))
> +		return -EOPNOTSUPP;
> +

[Severity: High]
Can this unconditionally dereference phydev->drv when it might be NULL?

If a PHY driver is unbound (e.g., via sysfs), phy_remove() sets
phydev->drv = NULL but does not detach the PHY from the net_device. A
subsequent call to ethtool -s ethX autoneg off reaches
phy_ethtool_ksettings_set(). The newly added check accesses phydev->drv
without verifying it is non-NULL, which can cause a kernel oops, allowing
a privileged user to crash the system instead of receiving a graceful error.

Later in the same function, _phy_start_aneg() correctly checks for
!phydev->drv.

[Severity: Medium]
Does this fix fully prevent userspace from bypassing the check?

While this patch aims to prevent a WARN(1) and stack trace in
phy_state_machine() by blocking AUTONEG_DISABLE via ethtool for PHYs with
PHY_BROKEN_FORCED, userspace can still bypass this via the SIOCSMIIREG
ioctl.

By writing to MII_BMCR and clearing the BMCR_ANENABLE bit, phy_mii_ioctl()
intercepts it to unconditionally set phydev->autoneg = AUTONEG_DISABLE
without checking the new PHY_BROKEN_FORCED flag:

drivers/net/phy/phy.c:phy_mii_ioctl() {
    ...
					phydev->autoneg = AUTONEG_DISABLE;
					if (val & BMCR_FULLDPLX)
    ...
}

When phy_state_machine() subsequently runs and calls _phy_start_aneg(), the
driver will return -EOPNOTSUPP, and phy_error_precise() will trigger the
exact same WARN(1) the patch intended to fix.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006-ax88179a-v17-0-252814fe35ea@birger-koblitz.de?part=2