lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Nov 2020 16:22:18 +0100 From: Nicolas Ferre <nicolas.ferre@...rochip.com> To: Parshuram Thombare <pthombar@...ence.com>, <kuba@...nel.org>, <linux-arm-kernel@...ts.infradead.org>, <netdev@...r.kernel.org>, <linux@...linux.org.uk> CC: <Claudiu.Beznea@...rochip.com>, <Santiago.Esteban@...rochip.com>, <andrew@...n.ch>, <davem@...emloft.net>, <linux-kernel@...r.kernel.org>, <harini.katakam@...inx.com>, <michal.simek@...inx.com> Subject: Re: [PATCH] net: macb: fix NULL dereference due to no pcs_config method On 05/11/2020 at 15:37, Parshuram Thombare wrote: > This patch fixes NULL pointer dereference due to NULL pcs_config > in pcs_ops. > > Fixes: e4e143e26ce8 ("net: macb: add support for high speed interface") What is this tag? In linux-next? As patch is not yet in Linus' tree, you cannot refer to it like this. > Reported-by: Nicolas Ferre <Nicolas.Ferre@...rochip.com> > Link: https://lkml.org/lkml/2020/11/4/482 You might need to change this to a "lore" link: https://lore.kernel.org/netdev/2db854c7-9ffb-328a-f346-f68982723d29@microchip.com/ > Signed-off-by: Parshuram Thombare <pthombar@...ence.com> This fix looks a bit weird to me. What about proposing a patch to Russell like the chunk that you already identified in function phylink_major_config()? > --- > drivers/net/ethernet/cadence/macb_main.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index b7bc160..130a5af 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -633,6 +633,15 @@ static void macb_pcs_an_restart(struct phylink_pcs *pcs) > /* Not supported */ > } > > +static int macb_pcs_config(struct phylink_pcs *pcs, > + unsigned int mode, > + phy_interface_t interface, > + const unsigned long *advertising, > + bool permit_pause_to_mac) > +{ > + return 0; > +} Russell, is the requirement for this void function intended? > + > static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { > .pcs_get_state = macb_usx_pcs_get_state, > .pcs_config = macb_usx_pcs_config, > @@ -642,6 +651,7 @@ static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { > static const struct phylink_pcs_ops macb_phylink_pcs_ops = { > .pcs_get_state = macb_pcs_get_state, > .pcs_an_restart = macb_pcs_an_restart, > + .pcs_config = macb_pcs_config, > }; > > static void macb_mac_config(struct phylink_config *config, unsigned int mode, > @@ -776,10 +786,13 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode, > > if (interface == PHY_INTERFACE_MODE_10GBASER) > bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops; > - else > + else if (interface == PHY_INTERFACE_MODE_SGMII) > bp->phylink_pcs.ops = &macb_phylink_pcs_ops; Do you confirm that all SGMII type interfaces need phylink_pcs.ops? > + else > + bp->phylink_pcs.ops = NULL; > > - phylink_set_pcs(bp->phylink, &bp->phylink_pcs); > + if (bp->phylink_pcs.ops) > + phylink_set_pcs(bp->phylink, &bp->phylink_pcs); > > return 0; > } Regards, Nicolas -- Nicolas Ferre
Powered by blists - more mailing lists