| Message ID | E1sN8tn-00GDCZ-Jj@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: fix potential use of NULL pointer in phy_suspend() | expand |
On Fri, Jun 28, 2024 at 11:32:11AM +0100, Russell King (Oracle) wrote: > phy_suspend() checks the WoL status, and then dereferences > phydrv->flags if (and only if) we decided that WoL has been enabled > on either the PHY or the netdev. > > We then check whether phydrv was NULL, but we've potentially already > dereferenced the pointer. > > If phydrv is NULL, then phy_ethtool_get_wol() will return an error > and leave wol.wolopts set to zero. However, if netdev->wol_enabled > is true, then we would dereference a NULL pointer. > > Checking the PHY drivers, the only place that phydev->wol_enabled is > checked by them is in their suspend/resume callbacks and nowhere else > (which is correct, because phylib only updates this in phy_suspend()). > > So, move the NULL pointer check earlier to avoid a NULL pointer > dereference. Leave the check for phydrv->suspend in place as a driver > may populate the .resume method but not the .suspend method. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- Note - I don't believe anyone has hit this condition, so maybe we want the patch for net-next rather than net? For now I have the patch against both trees so let me know which you'd prefer. > drivers/net/phy/phy_device.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 6c6ec9475709..19f8ae113dd3 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1980,7 +1980,7 @@ int phy_suspend(struct phy_device *phydev) > const struct phy_driver *phydrv = phydev->drv; > int ret; > > - if (phydev->suspended) > + if (phydev->suspended || !phydrv) > return 0; > > phy_ethtool_get_wol(phydev, &wol); > @@ -1989,7 +1989,7 @@ int phy_suspend(struct phy_device *phydev) > if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND)) > return -EBUSY; > > - if (!phydrv || !phydrv->suspend) > + if (!phydrv->suspend) > return 0; > > ret = phydrv->suspend(phydev); > -- > 2.30.2 > >
On Fri, Jun 28, 2024 at 11:32:11AM +0100, Russell King (Oracle) wrote: > phy_suspend() checks the WoL status, and then dereferences > phydrv->flags if (and only if) we decided that WoL has been enabled > on either the PHY or the netdev. > > We then check whether phydrv was NULL, but we've potentially already > dereferenced the pointer. > > If phydrv is NULL, then phy_ethtool_get_wol() will return an error > and leave wol.wolopts set to zero. However, if netdev->wol_enabled > is true, then we would dereference a NULL pointer. > > Checking the PHY drivers, the only place that phydev->wol_enabled is > checked by them is in their suspend/resume callbacks and nowhere else > (which is correct, because phylib only updates this in phy_suspend()). > > So, move the NULL pointer check earlier to avoid a NULL pointer > dereference. Leave the check for phydrv->suspend in place as a driver > may populate the .resume method but not the .suspend method. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> As you say, nobody has reported this. So i think net-next is sufficient. Andrew
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 6c6ec9475709..19f8ae113dd3 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1980,7 +1980,7 @@ int phy_suspend(struct phy_device *phydev) const struct phy_driver *phydrv = phydev->drv; int ret; - if (phydev->suspended) + if (phydev->suspended || !phydrv) return 0; phy_ethtool_get_wol(phydev, &wol); @@ -1989,7 +1989,7 @@ int phy_suspend(struct phy_device *phydev) if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND)) return -EBUSY; - if (!phydrv || !phydrv->suspend) + if (!phydrv->suspend) return 0; ret = phydrv->suspend(phydev);
phy_suspend() checks the WoL status, and then dereferences phydrv->flags if (and only if) we decided that WoL has been enabled on either the PHY or the netdev. We then check whether phydrv was NULL, but we've potentially already dereferenced the pointer. If phydrv is NULL, then phy_ethtool_get_wol() will return an error and leave wol.wolopts set to zero. However, if netdev->wol_enabled is true, then we would dereference a NULL pointer. Checking the PHY drivers, the only place that phydev->wol_enabled is checked by them is in their suspend/resume callbacks and nowhere else (which is correct, because phylib only updates this in phy_suspend()). So, move the NULL pointer check earlier to avoid a NULL pointer dereference. Leave the check for phydrv->suspend in place as a driver may populate the .resume method but not the .suspend method. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phy_device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)