| Message ID | 60o.ZbUd.3E5eHrOkFLD.1csh{G@seznam.cz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: phy: dp83822: Fix NULL pointer dereference on DP83825 devices | expand |
Hello Tomas, On Fri, 06 Sep 2024 10:38:40 +0200 (CEST) "Tomas Paukrt" <tomaspaukrt@email.cz> wrote: > The probe() function is only used for DP83822 and DP83826 models, > leaving the private data pointer uninitialized for the DP83825 models > which causes a NULL pointer dereference in the recently changed functions > dp8382x_config_init() and dp83822_set_wol(). > > Add the dp8382x_probe() function, so all PHY models will have a valid > private data pointer to prevent similar issues in the future. > > Fixes: 9ef9ecfa9e9f ("net: phy: dp8382x: keep WOL settings across suspends") > Signed-off-by: Tomas Paukrt <tomaspaukrt@email.cz> The dp83825 does seem to support WoL, so allocating the private data is indeed the way to go here. As this is a fix, you should send the patch targetting the "net" tree, so you need to include that in the patch subject (use the --subject-prefix="PATCH net" option for git format-patch). > --- > drivers/net/phy/dp83822.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c > index efeb643..58877c0 100644 > --- a/drivers/net/phy/dp83822.c > +++ b/drivers/net/phy/dp83822.c > @@ -271,8 +271,7 @@ static int dp83822_config_intr(struct phy_device *phydev) > DP83822_ENERGY_DET_INT_EN | > DP83822_LINK_QUAL_INT_EN); > > - /* Private data pointer is NULL on DP83825 */ > - if (!dp83822 || !dp83822->fx_enabled) > + if (!dp83822->fx_enabled) > misr_status |= DP83822_ANEG_COMPLETE_INT_EN | > DP83822_DUP_MODE_CHANGE_INT_EN | > DP83822_SPEED_CHANGED_INT_EN; > @@ -292,8 +291,7 @@ static int dp83822_config_intr(struct phy_device *phydev) > DP83822_PAGE_RX_INT_EN | > DP83822_EEE_ERROR_CHANGE_INT_EN); > > - /* Private data pointer is NULL on DP83825 */ > - if (!dp83822 || !dp83822->fx_enabled) > + if (!dp83822->fx_enabled) > misr_status |= DP83822_ANEG_ERR_INT_EN | > DP83822_WOL_PKT_INT_EN; > > @@ -731,6 +729,20 @@ static int dp83826_probe(struct phy_device *phydev) > return 0; > } > > +static int dp8382x_probe(struct phy_device *phydev) > +{ > + struct dp83822_private *dp83822; > + > + dp83822 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83822), > + GFP_KERNEL); > + if (!dp83822) > + return -ENOMEM; > + > + phydev->priv = dp83822; > + > + return 0; > +} Now all the PHYs from that driver use a probe() function that does that same allocation and sets the phydev->priv, maybe we could have dp83826_probe() and dp83826_probe() call this newly introduced function, to avoid some duplication ? The rest looks good to me, Thanks, Maxime
diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c index efeb643..58877c0 100644 --- a/drivers/net/phy/dp83822.c +++ b/drivers/net/phy/dp83822.c @@ -271,8 +271,7 @@ static int dp83822_config_intr(struct phy_device *phydev) DP83822_ENERGY_DET_INT_EN | DP83822_LINK_QUAL_INT_EN); - /* Private data pointer is NULL on DP83825 */ - if (!dp83822 || !dp83822->fx_enabled) + if (!dp83822->fx_enabled) misr_status |= DP83822_ANEG_COMPLETE_INT_EN | DP83822_DUP_MODE_CHANGE_INT_EN | DP83822_SPEED_CHANGED_INT_EN; @@ -292,8 +291,7 @@ static int dp83822_config_intr(struct phy_device *phydev) DP83822_PAGE_RX_INT_EN | DP83822_EEE_ERROR_CHANGE_INT_EN); - /* Private data pointer is NULL on DP83825 */ - if (!dp83822 || !dp83822->fx_enabled) + if (!dp83822->fx_enabled) misr_status |= DP83822_ANEG_ERR_INT_EN | DP83822_WOL_PKT_INT_EN; @@ -731,6 +729,20 @@ static int dp83826_probe(struct phy_device *phydev) return 0; } +static int dp8382x_probe(struct phy_device *phydev) +{ + struct dp83822_private *dp83822; + + dp83822 = devm_kzalloc(&phydev->mdio.dev, sizeof(*dp83822), + GFP_KERNEL); + if (!dp83822) + return -ENOMEM; + + phydev->priv = dp83822; + + return 0; +} + static int dp83822_suspend(struct phy_device *phydev) { int value; @@ -795,6 +807,7 @@ static int dp83822_resume(struct phy_device *phydev) PHY_ID_MATCH_MODEL(_id), \ .name = (_name), \ /* PHY_BASIC_FEATURES */ \ + .probe = dp8382x_probe, \ .soft_reset = dp83822_phy_reset, \ .config_init = dp8382x_config_init, \ .get_wol = dp83822_get_wol, \
The probe() function is only used for DP83822 and DP83826 models, leaving the private data pointer uninitialized for the DP83825 models which causes a NULL pointer dereference in the recently changed functions dp8382x_config_init() and dp83822_set_wol(). Add the dp8382x_probe() function, so all PHY models will have a valid private data pointer to prevent similar issues in the future. Fixes: 9ef9ecfa9e9f ("net: phy: dp8382x: keep WOL settings across suspends") Signed-off-by: Tomas Paukrt <tomaspaukrt@email.cz> --- drivers/net/phy/dp83822.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)