| Message ID | 20220629193358.4007923-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b7d78b46d5e8dc77c656c13885d31e931923b915 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phylink: fix NULL pl->pcs dereference during phylink_pcs_poll_start | expand |
On Wed, Jun 29, 2022 at 10:33:58PM +0300, Vladimir Oltean wrote: > The current link mode of the phylink instance may not require an > attached PCS. However, phylink_major_config() unconditionally > dereferences this potentially NULL pointer when restarting the link poll > timer, which will panic the kernel. > > Fix the problem by checking whether a PCS exists in phylink_pcs_poll_start(), > otherwise do nothing. The code prior to the blamed patch also only > looked at pcs->poll within an "if (pcs)" block. > > Fixes: bfac8c490d60 ("net: phylink: disable PCS polling over major configuration") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Thanks. Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
On 29.06.22 21:42, Russell King (Oracle) wrote: > On Wed, Jun 29, 2022 at 10:33:58PM +0300, Vladimir Oltean wrote: >> The current link mode of the phylink instance may not require an >> attached PCS. However, phylink_major_config() unconditionally >> dereferences this potentially NULL pointer when restarting the link poll >> timer, which will panic the kernel. >> >> Fix the problem by checking whether a PCS exists in phylink_pcs_poll_start(), >> otherwise do nothing. The code prior to the blamed patch also only >> looked at pcs->poll within an "if (pcs)" block. >> >> Fixes: bfac8c490d60 ("net: phylink: disable PCS polling over major configuration") >> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Thanks. > > Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > Fixes the problem on my side. Tested-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> The current link mode of the phylink instance may not require an > attached PCS. However, phylink_major_config() unconditionally > dereferences this potentially NULL pointer when restarting the link poll > timer, which will panic the kernel. > > Fix the problem by checking whether a PCS exists in phylink_pcs_poll_start(), > otherwise do nothing. The code prior to the blamed patch also only > looked at pcs->poll within an "if (pcs)" block. > > Fixes: bfac8c490d60 ("net: phylink: disable PCS polling over major configuration") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Tested-by: Michael Walle <michael@walle.cc> # on kontron-kbox-a-230-ls -michael
Vladimir, Russell, On 29/06/2022 at 21:33, Vladimir Oltean wrote: > The current link mode of the phylink instance may not require an > attached PCS. However, phylink_major_config() unconditionally > dereferences this potentially NULL pointer when restarting the link poll > timer, which will panic the kernel. > > Fix the problem by checking whether a PCS exists in phylink_pcs_poll_start(), > otherwise do nothing. The code prior to the blamed patch also only > looked at pcs->poll within an "if (pcs)" block. > > Fixes: bfac8c490d60 ("net: phylink: disable PCS polling over major configuration") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/phy/phylink.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > index 1a7550f5fdf5..48f0b9b39491 100644 > --- a/drivers/net/phy/phylink.c > +++ b/drivers/net/phy/phylink.c > @@ -766,7 +766,7 @@ static void phylink_pcs_poll_stop(struct phylink *pl) > > static void phylink_pcs_poll_start(struct phylink *pl) > { > - if (pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND) > + if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND) > mod_timer(&pl->link_poll, jiffies + HZ); > } > Fixes the NULL pointer on my boards: Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> # on sam9x60ek Best regards, Nicolas
On Thu, Jun 30, 2022 at 03:46:54PM +0200, Nicolas Ferre wrote: > Vladimir, Russell, > > On 29/06/2022 at 21:33, Vladimir Oltean wrote: > > The current link mode of the phylink instance may not require an > > attached PCS. However, phylink_major_config() unconditionally > > dereferences this potentially NULL pointer when restarting the link poll > > timer, which will panic the kernel. > > > > Fix the problem by checking whether a PCS exists in phylink_pcs_poll_start(), > > otherwise do nothing. The code prior to the blamed patch also only > > looked at pcs->poll within an "if (pcs)" block. > > > > Fixes: bfac8c490d60 ("net: phylink: disable PCS polling over major configuration") > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > --- > > drivers/net/phy/phylink.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c > > index 1a7550f5fdf5..48f0b9b39491 100644 > > --- a/drivers/net/phy/phylink.c > > +++ b/drivers/net/phy/phylink.c > > @@ -766,7 +766,7 @@ static void phylink_pcs_poll_stop(struct phylink *pl) > > static void phylink_pcs_poll_start(struct phylink *pl) > > { > > - if (pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND) > > + if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND) > > mod_timer(&pl->link_poll, jiffies + HZ); > > } > > Fixes the NULL pointer on my boards: > Tested-by: Nicolas Ferre <nicolas.ferre@microchip.com> # on sam9x60ek Thanks all, hopefully it'll get applied to net-next soon. Sadly, this slipped through my testing, as the only platform I have access to at the moment always supplies a PCS (mvneta based) so there's no way my testing would ever have caught this. Sorry for the problems.
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Wed, 29 Jun 2022 22:33:58 +0300 you wrote: > The current link mode of the phylink instance may not require an > attached PCS. However, phylink_major_config() unconditionally > dereferences this potentially NULL pointer when restarting the link poll > timer, which will panic the kernel. > > Fix the problem by checking whether a PCS exists in phylink_pcs_poll_start(), > otherwise do nothing. The code prior to the blamed patch also only > looked at pcs->poll within an "if (pcs)" block. > > [...] Here is the summary with links: - [net-next] net: phylink: fix NULL pl->pcs dereference during phylink_pcs_poll_start https://git.kernel.org/netdev/net-next/c/b7d78b46d5e8 You are awesome, thank you!
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 1a7550f5fdf5..48f0b9b39491 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -766,7 +766,7 @@ static void phylink_pcs_poll_stop(struct phylink *pl) static void phylink_pcs_poll_start(struct phylink *pl) { - if (pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND) + if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND) mod_timer(&pl->link_poll, jiffies + HZ); }
The current link mode of the phylink instance may not require an attached PCS. However, phylink_major_config() unconditionally dereferences this potentially NULL pointer when restarting the link poll timer, which will panic the kernel. Fix the problem by checking whether a PCS exists in phylink_pcs_poll_start(), otherwise do nothing. The code prior to the blamed patch also only looked at pcs->poll within an "if (pcs)" block. Fixes: bfac8c490d60 ("net: phylink: disable PCS polling over major configuration") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/phy/phylink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)