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: Fri, 16 Jan 2015 10:45:19 -0800 From: Florian Fainelli <f.fainelli@...il.com> To: Ben Hutchings <ben.hutchings@...ethink.co.uk> Cc: ct178-internal@...ts.codethink.co.uk, "David S. Miller" <davem@...emloft.net>, netdev <netdev@...r.kernel.org>, linux-kernel <linux-kernel@...ts.codethink.co.uk>, Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.com>, Mitsuhiro Kimura <mitsuhiro.kimura.kc@...esas.com>, Hisashi Nakamura <hisashi.nakamura.ak@...esas.com>, Yoshihiro Kaneko <ykaneko0929@...il.com> Subject: Re: [PATCH net 2/2] sh_eth: Fix ethtool operation crash when net device is down 2015-01-16 9:51 GMT-08:00 Ben Hutchings <ben.hutchings@...ethink.co.uk>: > The driver connects and disconnects the PHY device whenever the > net device is brought up and down. The ethtool get_settings, > set_settings and nway_reset operations will dereference a null > or dangling pointer if called while it is down. > > I think it would be preferable to keep the PHY connected, but there > may be good reasons not to. phy_disconnect() is the canonical way to stop the PHY library state machine, and avoid deferred work to be done and call the driver's adjust_link function. This also boils down to calling phy_detach() which can put the PHY in a low-power mode when implemented. > > As an immediate fix for this bug: > - Set the phydev pointer to NULL after disconnecting the PHY > - Change those three operations to return -ENODEV while the PHY is > not connected > > Signed-off-by: Ben Hutchings <ben.hutchings@...ethink.co.uk> > --- > drivers/net/ethernet/renesas/sh_eth.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index 0c4d5b5..28e3822 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -1827,6 +1827,9 @@ static int sh_eth_get_settings(struct net_device *ndev, > unsigned long flags; > int ret; > > + if (!mdp->phydev) > + return -ENODEV; Since the PHY is disconnected, would not checking for netif_running() make sense here, unless there is a good reason to still allow phy_ethtool_gset() to be called? > + > spin_lock_irqsave(&mdp->lock, flags); > ret = phy_ethtool_gset(mdp->phydev, ecmd); > spin_unlock_irqrestore(&mdp->lock, flags); > @@ -1841,6 +1844,9 @@ static int sh_eth_set_settings(struct net_device *ndev, > unsigned long flags; > int ret; > > + if (!mdp->phydev) > + return -ENODEV; > + > spin_lock_irqsave(&mdp->lock, flags); > > /* disable tx and rx */ > @@ -1875,6 +1881,9 @@ static int sh_eth_nway_reset(struct net_device *ndev) > unsigned long flags; > int ret; > > + if (!mdp->phydev) > + return -ENODEV; > + > spin_lock_irqsave(&mdp->lock, flags); > ret = phy_start_aneg(mdp->phydev); > spin_unlock_irqrestore(&mdp->lock, flags); > @@ -2184,6 +2193,7 @@ static int sh_eth_close(struct net_device *ndev) > if (mdp->phydev) { > phy_stop(mdp->phydev); > phy_disconnect(mdp->phydev); > + mdp->phydev = NULL; > } > > free_irq(ndev->irq, ndev); > -- > 1.7.10.4 > > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@...r.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Florian -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists