[PATCH net v3 0/3] net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()

[PATCH net v3 0/3] net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()

[Florian Fainelli]

Hi all,

This patch series addresses the crash seen with the Generic PHY driver
in phy_attach_direct() introduced in the latest pull to Linus.

We also address how to properly bind and unbind to/from the PHY drivers which
would previously be crashing in flames since we did not stop the state machine.

Thanks!

Changes in v3:

- made more testing as module/built-in, with Generic and non-Generic PHY drivers
- exercised error paths on purpose by injecting errors
- properly incremenet Generic PHY module reference count as well
- fixed the error path to be correct

Changes in v2:

- fixed net: phy: Fix lack of reference count on PHY driver against
  the Generic PHY driver which is special

Florian Fainelli (3):
  net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()
  net: phy: Check phydev->drv
  net: phy: Fix PHY driver bind and unbind events

 drivers/net/phy/phy.c        | 26 +++++++++++++++++++++----
 drivers/net/phy/phy_device.c | 45 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/phy.h          |  3 +++
 3 files changed, 66 insertions(+), 8 deletions(-)

-- 
2.9.3

[PATCH net v3 1/3] net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()

[Florian Fainelli]

The Generic PHY drivers gets assigned after we checked that the current
PHY driver is NULL, so we need to check a few things before we can
safely dereference d->driver. This would be causing a NULL deference to
occur when a system binds to the Generic PHY driver. Update
phy_attach_direct() to do the following:

- grab the driver module reference after we have assigned the Generic
  PHY drivers accordingly

- update the error path to clean up the module reference in case the
  Generic PHY probe function fails

Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0d8f4d3847f6..d63d190a95ef 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -908,6 +908,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 	struct module *ndev_owner = dev->dev.parent->driver->owner;
 	struct mii_bus *bus = phydev->mdio.bus;
 	struct device *d = &phydev->mdio.dev;
+	bool using_genphy = false;
 	int err;
 
 	/* For Ethernet device drivers that register their own MDIO bus, we
@@ -938,12 +939,22 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			d->driver =
 				&genphy_driver[GENPHY_DRV_1G].mdiodrv.driver;
 
+		using_genphy = true;
+	}
+
+	if (!try_module_get(d->driver->owner)) {
+		dev_err(&dev->dev, "failed to get the device driver module\n");
+		err = -EIO;
+		goto error_put_device;
+	}
+
+	if (using_genphy) {
 		err = d->driver->probe(d);
 		if (err >= 0)
 			err = device_bind_driver(d);
 
 		if (err)
-			goto error;
+			goto error_module_put;
 	}
 
 	if (phydev->attached_dev) {
@@ -981,6 +992,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 error:
 	phy_detach(phydev);
+error_module_put:
+	module_put(d->driver->owner);
+error_put_device:
 	put_device(d);
 	module_put(d->driver->owner);
 	if (ndev_owner != bus->owner)
-- 
2.9.3

Re: [PATCH net v3 1/3] net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()

[Florian Fainelli]

On 02/08/2017 04:13 PM, Florian Fainelli wrote:
> The Generic PHY drivers gets assigned after we checked that the current
> PHY driver is NULL, so we need to check a few things before we can
> safely dereference d->driver. This would be causing a NULL deference to
> occur when a system binds to the Generic PHY driver. Update
> phy_attach_direct() to do the following:
> 
> - grab the driver module reference after we have assigned the Generic
>   PHY drivers accordingly
> 
> - update the error path to clean up the module reference in case the
>   Generic PHY probe function fails
> 
> Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0d8f4d3847f6..d63d190a95ef 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -908,6 +908,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  	struct module *ndev_owner = dev->dev.parent->driver->owner;
>  	struct mii_bus *bus = phydev->mdio.bus;
>  	struct device *d = &phydev->mdio.dev;
> +	bool using_genphy = false;
>  	int err;
>  
>  	/* For Ethernet device drivers that register their own MDIO bus, we
> @@ -938,12 +939,22 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>  			d->driver =
>  				&genphy_driver[GENPHY_DRV_1G].mdiodrv.driver;
>  
> +		using_genphy = true;
> +	}
> +
> +	if (!try_module_get(d->driver->owner)) {
> +		dev_err(&dev->dev, "failed to get the device driver module\n");
> +		err = -EIO;
> +		goto error_put_device;
> +	}

And still not correct, since we need to remove the other hunk, one day I
will learn how to properly rebase my work... will submit a v4 shortly.
-- 
Florian

Re: [net, v3, 1/3] net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()

[Heiko Schocher]

Hello Florian,

Am 09.02.2017 um 01:13 schrieb Florian Fainelli:
> The Generic PHY drivers gets assigned after we checked that the current
> PHY driver is NULL, so we need to check a few things before we can
> safely dereference d->driver. This would be causing a NULL deference to
> occur when a system binds to the Generic PHY driver. Update
> phy_attach_direct() to do the following:
>
> - grab the driver module reference after we have assigned the Generic
>    PHY drivers accordingly
>
> - update the error path to clean up the module reference in case the
>    Generic PHY probe function fails
>
> Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/net/phy/phy_device.c | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)

just stumbled over this bug on an am335x based board, with an
KSZ8081 attached, so there a "fixed-link" is used like:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-baltos-ir3220.dts#n105

With your patch it crashes also ...

If I remove this part:

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d63d190..9dd08a4 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -921,11 +921,6 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
                 return -EIO;
         }

-       if (!try_module_get(d->driver->owner)) {
-               dev_err(&dev->dev, "failed to get the device driver module\n");
-               return -EIO;
-       }
-
         get_device(d);

         /* Assume that if there is no driver, that it doesn't

it boots again .. I think, you forgot? simply this remove ?

bye,
Heiko
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0d8f4d3847f6..d63d190a95ef 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -908,6 +908,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>   	struct module *ndev_owner = dev->dev.parent->driver->owner;
>   	struct mii_bus *bus = phydev->mdio.bus;
>   	struct device *d = &phydev->mdio.dev;
> +	bool using_genphy = false;
>   	int err;
>
>   	/* For Ethernet device drivers that register their own MDIO bus, we
> @@ -938,12 +939,22 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>   			d->driver =
>   				&genphy_driver[GENPHY_DRV_1G].mdiodrv.driver;
>
> +		using_genphy = true;
> +	}
> +
> +	if (!try_module_get(d->driver->owner)) {
> +		dev_err(&dev->dev, "failed to get the device driver module\n");
> +		err = -EIO;
> +		goto error_put_device;
> +	}
> +
> +	if (using_genphy) {
>   		err = d->driver->probe(d);
>   		if (err >= 0)
>   			err = device_bind_driver(d);
>
>   		if (err)
> -			goto error;
> +			goto error_module_put;
>   	}
>
>   	if (phydev->attached_dev) {
> @@ -981,6 +992,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>
>   error:
>   	phy_detach(phydev);
> +error_module_put:
> +	module_put(d->driver->owner);
> +error_put_device:
>   	put_device(d);
>   	module_put(d->driver->owner);
>   	if (ndev_owner != bus->owner)
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [net, v3, 1/3] net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()

[Florian Fainelli]

On 02/08/2017 10:58 PM, Heiko Schocher wrote:
> Hello Florian,
> 
> Am 09.02.2017 um 01:13 schrieb Florian Fainelli:
>> The Generic PHY drivers gets assigned after we checked that the current
>> PHY driver is NULL, so we need to check a few things before we can
>> safely dereference d->driver. This would be causing a NULL deference to
>> occur when a system binds to the Generic PHY driver. Update
>> phy_attach_direct() to do the following:
>>
>> - grab the driver module reference after we have assigned the Generic
>>    PHY drivers accordingly
>>
>> - update the error path to clean up the module reference in case the
>>    Generic PHY probe function fails
>>
>> Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY
>> driver")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/net/phy/phy_device.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
> 
> just stumbled over this bug on an am335x based board, with an
> KSZ8081 attached, so there a "fixed-link" is used like:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-baltos-ir3220.dts#n105
> 
> 
> With your patch it crashes also ...

The final version of the patch is here:

http://patchwork.ozlabs.org/patch/725923/

Do you mind giving it a try?
-- 
Florian

Re: [net, v3, 1/3] net: phy: Fix PHY module checks and NULL deref in phy_attach_direct()

[Heiko Schocher]

Hello Florian,

Am 09.02.2017 um 08:13 schrieb Florian Fainelli:
>
>
> On 02/08/2017 10:58 PM, Heiko Schocher wrote:
>> Hello Florian,
>>
>> Am 09.02.2017 um 01:13 schrieb Florian Fainelli:
>>> The Generic PHY drivers gets assigned after we checked that the current
>>> PHY driver is NULL, so we need to check a few things before we can
>>> safely dereference d->driver. This would be causing a NULL deference to
>>> occur when a system binds to the Generic PHY driver. Update
>>> phy_attach_direct() to do the following:
>>>
>>> - grab the driver module reference after we have assigned the Generic
>>>     PHY drivers accordingly
>>>
>>> - update the error path to clean up the module reference in case the
>>>     Generic PHY probe function fails
>>>
>>> Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY
>>> driver")
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>    drivers/net/phy/phy_device.c | 16 +++++++++++++++-
>>>    1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> just stumbled over this bug on an am335x based board, with an
>> KSZ8081 attached, so there a "fixed-link" is used like:
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/am335x-baltos-ir3220.dts#n105
>>
>>
>> With your patch it crashes also ...
>
> The final version of the patch is here:
>
> http://patchwork.ozlabs.org/patch/725923/

Huh, sorry ...

> Do you mind giving it a try?

With this patch, ethernet works again fine on this board, thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[PATCH net v3 2/3] net: phy: Check phydev->drv

[Florian Fainelli]

In preparation for supporting driver bind/unbind properly, sprinkle checks on
phydev->drv where we may call into PHYLIB from user-space or other parts of the
kernel.

Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy.c        | 26 ++++++++++++++++++++++----
 drivers/net/phy/phy_device.c |  2 +-
 include/linux/phy.h          |  3 +++
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7cc1b7dcfe05..d6f7838455dd 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -580,7 +580,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		return 0;
 
 	case SIOCSHWTSTAMP:
-		if (phydev->drv->hwtstamp)
+		if (phydev->drv && phydev->drv->hwtstamp)
 			return phydev->drv->hwtstamp(phydev, ifr);
 		/* fall through */
 
@@ -603,6 +603,9 @@ int phy_start_aneg(struct phy_device *phydev)
 {
 	int err;
 
+	if (!phydev->drv)
+		return -EIO;
+
 	mutex_lock(&phydev->lock);
 
 	if (AUTONEG_DISABLE == phydev->autoneg)
@@ -975,7 +978,7 @@ void phy_state_machine(struct work_struct *work)
 
 	old_state = phydev->state;
 
-	if (phydev->drv->link_change_notify)
+	if (phydev->drv && phydev->drv->link_change_notify)
 		phydev->drv->link_change_notify(phydev);
 
 	switch (phydev->state) {
@@ -1286,6 +1289,9 @@ EXPORT_SYMBOL(phy_write_mmd_indirect);
  */
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
 {
+	if (!phydev->drv)
+		return -EIO;
+
 	/* According to 802.3az,the EEE is supported only in full duplex-mode.
 	 * Also EEE feature is active when core is operating with MII, GMII
 	 * or RGMII (all kinds). Internal PHYs are also allowed to proceed and
@@ -1363,6 +1369,9 @@ EXPORT_SYMBOL(phy_init_eee);
  */
 int phy_get_eee_err(struct phy_device *phydev)
 {
+	if (!phydev->drv)
+		return -EIO;
+
 	return phy_read_mmd_indirect(phydev, MDIO_PCS_EEE_WK_ERR, MDIO_MMD_PCS);
 }
 EXPORT_SYMBOL(phy_get_eee_err);
@@ -1379,6 +1388,9 @@ int phy_ethtool_get_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
 	int val;
 
+	if (!phydev->drv)
+		return -EIO;
+
 	/* Get Supported EEE */
 	val = phy_read_mmd_indirect(phydev, MDIO_PCS_EEE_ABLE, MDIO_MMD_PCS);
 	if (val < 0)
@@ -1412,6 +1424,9 @@ int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_eee *data)
 {
 	int val = ethtool_adv_to_mmd_eee_adv_t(data->advertised);
 
+	if (!phydev->drv)
+		return -EIO;
+
 	/* Mask prohibited EEE modes */
 	val &= ~phydev->eee_broken_modes;
 
@@ -1423,7 +1438,7 @@ EXPORT_SYMBOL(phy_ethtool_set_eee);
 
 int phy_ethtool_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
-	if (phydev->drv->set_wol)
+	if (phydev->drv && phydev->drv->set_wol)
 		return phydev->drv->set_wol(phydev, wol);
 
 	return -EOPNOTSUPP;
@@ -1432,7 +1447,7 @@ EXPORT_SYMBOL(phy_ethtool_set_wol);
 
 void phy_ethtool_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 {
-	if (phydev->drv->get_wol)
+	if (phydev->drv && phydev->drv->get_wol)
 		phydev->drv->get_wol(phydev, wol);
 }
 EXPORT_SYMBOL(phy_ethtool_get_wol);
@@ -1468,6 +1483,9 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
 	if (!phydev)
 		return -ENODEV;
 
+	if (!phydev->drv)
+		return -EIO;
+
 	return genphy_restart_aneg(phydev);
 }
 EXPORT_SYMBOL(phy_ethtool_nway_reset);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d63d190a95ef..40675b9706ae 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1790,7 +1790,7 @@ static int phy_remove(struct device *dev)
 	phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
 
-	if (phydev->drv->remove)
+	if (phydev->drv && phydev->drv->remove)
 		phydev->drv->remove(phydev);
 	phydev->drv = NULL;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7fc1105605bf..231e07bb0d76 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -802,6 +802,9 @@ int phy_stop_interrupts(struct phy_device *phydev);
 
 static inline int phy_read_status(struct phy_device *phydev)
 {
+	if (!phydev->drv)
+		return -EIO;
+
 	return phydev->drv->read_status(phydev);
 }
 
-- 
2.9.3

[PATCH net v3 3/3] net: phy: Fix PHY driver bind and unbind events

[Florian Fainelli]

The PHY library does not deal very well with bind and unbind events. The first
thing we would see is that we were not properly canceling the PHY state machine
workqueue, so we would be crashing while dereferencing phydev->drv since there
is no driver attached anymore.

Once we fix that, there are several things that did not quite work as expected:

- if the PHY state machine was running, we were not stopping it properly, and
  the state machine state would not be marked as such
- when we rebind the driver, nothing would happen, since we would not know which
  state we were before the unbind

This patch takes the following approach:

- if the PHY was attached, and the state machine was running we would stop it,
  remember where we left, and schedule the state machine for restart upong
  driver bind
- if the PHY was attached, but HALTED, we would let it in that state, and do not
  alter the state upon driver bind
- in all other cases (detached) we would keep the PHY in DOWN state waiting for
  a network driver to show up, and set PHY_READY on driver bind

Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 40675b9706ae..6e46f6807bb7 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1723,6 +1723,7 @@ static int phy_probe(struct device *dev)
 	struct phy_device *phydev = to_phy_device(dev);
 	struct device_driver *drv = phydev->mdio.dev.driver;
 	struct phy_driver *phydrv = to_phy_driver(drv);
+	bool should_start = false;
 	int err = 0;
 
 	phydev->drv = phydrv;
@@ -1772,24 +1773,46 @@ static int phy_probe(struct device *dev)
 	}
 
 	/* Set the state to READY by default */
-	phydev->state = PHY_READY;
+	if (phydev->state > PHY_UP && phydev->state != PHY_HALTED)
+		should_start = true;
+	else
+		phydev->state = PHY_READY;
 
 	if (phydev->drv->probe)
 		err = phydev->drv->probe(phydev);
 
 	mutex_unlock(&phydev->lock);
 
+	if (should_start)
+		phy_start(phydev);
+
 	return err;
 }
 
 static int phy_remove(struct device *dev)
 {
 	struct phy_device *phydev = to_phy_device(dev);
+	bool should_stop = false;
+	enum phy_state state;
+
+	cancel_delayed_work_sync(&phydev->state_queue);
 
 	mutex_lock(&phydev->lock);
-	phydev->state = PHY_DOWN;
+	state = phydev->state;
+	if (state > PHY_UP && state != PHY_HALTED)
+		should_stop = true;
+	else
+		phydev->state = PHY_DOWN;
 	mutex_unlock(&phydev->lock);
 
+	/* phy_stop() sets the state to HALTED, undo that for the ->probe() function
+	 * to have a chance to resume where we left
+	 */
+	if (should_stop) {
+		phy_stop(phydev);
+		phydev->state = state;
+	}
+
 	if (phydev->drv && phydev->drv->remove)
 		phydev->drv->remove(phydev);
 	phydev->drv = NULL;
-- 
2.9.3