netdev - Re: [PATCH net 2/2] sh_eth: Fix ethtool operation crash when net device is down

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGVrzcbj2JANTECtxgDUrb7c_20nyoW4n5ARndHGKYuQu-0kQg@mail.gmail.com>
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