diff mbox series

[net] net: phy: fix potential use of NULL pointer in phy_suspend()

Message ID E1sN8tn-00GDCZ-Jj@rmk-PC.armlinux.org.uk (mailing list archive)
State Accepted
Commit 19e6ad2c75782bd15b3df24df7982da457d702c5
Delegated to: Netdev Maintainers
Headers show
Series [net] net: phy: fix potential use of NULL pointer in phy_suspend() | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 856 this patch: 856
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 860 this patch: 860
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 860 this patch: 860
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 40 this patch: 40
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-28--21-00 (tests: 665)

Commit Message

Russell King (Oracle) June 28, 2024, 10:32 a.m. UTC
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(-)

Comments

Russell King (Oracle) June 28, 2024, 10:33 a.m. UTC | #1
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
> 
>
Andrew Lunn June 28, 2024, 2:12 p.m. UTC | #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
patchwork-bot+netdevbpf@kernel.org July 2, 2024, 3:20 a.m. UTC | #3
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 28 Jun 2024 11:32:11 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net: phy: fix potential use of NULL pointer in phy_suspend()
    https://git.kernel.org/netdev/net-next/c/19e6ad2c7578

You are awesome, thank you!
diff mbox series

Patch

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);