[PATCH 1/3] net: phy: Check phy_driver ready before accessing

Since a phy_device is added to the global mdio_bus list during
phy_device_register(), but a phy_device's phy_driver doesn't get
attached until phy_probe(). It's possible of_phy_find_device() in
xgmiitorgmii will return a valid phy with a NULL phy_driver. Leading to
a NULL pointer access during the memcpy().

Signed-off-by: Brandon Maier <redacted>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 2e5150b0b8d5..04c8bec1c4c1 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c

@@ -81,6 +81,11 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
 		return -EPROBE_DEFER;
 	}
 
+	if (!priv->phy_dev->drv) {
+		dev_info(dev, "Attached phy not ready\n");
+		return -EPROBE_DEFER;
+	}
+
 	priv->addr = mdiodev->addr;
 	priv->phy_drv = priv->phy_dev->drv;
 	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,

-- 
2.14.3

We're ignoring the result of the attached phy device's read_status().
Return it so we can detect errors.

Signed-off-by: Brandon Maier <redacted>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index d6f8b64cddbe..74a8782313cf 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c

@@ -42,8 +42,11 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 	struct mii_bus *bus = priv->mdio->bus;
 	int addr = priv->mdio->addr;
 	u16 val = 0;
+	int err;
 
-	priv->phy_drv->read_status(phydev);
+	err = priv->phy_drv->read_status(phydev);
+	if (err < 0)
+		return err;
 
 	val = mdiobus_read(bus, addr, XILINX_GMII2RGMII_REG);
 	val &= ~XILINX_GMII2RGMII_SPEED_MASK;

-- 
2.14.3

The xgmiitorgmii is using the mii_bus of the device it's attached too,
instead of the bus it was given during probe.

Signed-off-by: Brandon Maier <redacted>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 04c8bec1c4c1..d6f8b64cddbe 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c

@@ -33,17 +33,19 @@ struct gmii2rgmii {
 	struct phy_device *phy_dev;
 	struct phy_driver *phy_drv;
 	struct phy_driver conv_phy_drv;
-	int addr;
+	struct mdio_device *mdio;
 };
 
 static int xgmiitorgmii_read_status(struct phy_device *phydev)
 {
 	struct gmii2rgmii *priv = phydev->priv;
+	struct mii_bus *bus = priv->mdio->bus;
+	int addr = priv->mdio->addr;
 	u16 val = 0;
 
 	priv->phy_drv->read_status(phydev);
 
-	val = mdiobus_read(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG);
+	val = mdiobus_read(bus, addr, XILINX_GMII2RGMII_REG);
 	val &= ~XILINX_GMII2RGMII_SPEED_MASK;
 
 	if (phydev->speed == SPEED_1000)

@@ -53,7 +55,7 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 	else
 		val |= BMCR_SPEED10;
 
-	mdiobus_write(phydev->mdio.bus, priv->addr, XILINX_GMII2RGMII_REG, val);
+	mdiobus_write(bus, addr, XILINX_GMII2RGMII_REG, val);
 
 	return 0;
 }

@@ -86,7 +88,7 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
 		return -EPROBE_DEFER;
 	}
 
-	priv->addr = mdiodev->addr;
+	priv->mdio = mdiodev;
 	priv->phy_drv = priv->phy_dev->drv;
 	memcpy(&priv->conv_phy_drv, priv->phy_dev->drv,
 	       sizeof(struct phy_driver));

-- 
2.14.3

On Thu, Jun 07, 2018 at 10:53:48AM -0500, Brandon Maier wrote:

We're ignoring the result of the attached phy device's read_status().
Return it so we can detect errors.

Signed-off-by: Brandon Maier <redacted>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Thu, Jun 07, 2018 at 10:53:47AM -0500, Brandon Maier wrote:

The xgmiitorgmii is using the mii_bus of the device it's attached too,
instead of the bus it was given during probe.

Hi Brandon

I think the assumption was they are both on the same bus.
However, they don't need to be.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Thu, Jun 07, 2018 at 10:53:46AM -0500, Brandon Maier wrote:

Since a phy_device is added to the global mdio_bus list during
phy_device_register(), but a phy_device's phy_driver doesn't get
attached until phy_probe(). It's possible of_phy_find_device() in
xgmiitorgmii will return a valid phy with a NULL phy_driver. Leading to
a NULL pointer access during the memcpy().

I'm sure there are more issues like this in the code.  e.g. there is
no attempt made to hold a reference to the child phy. So it could be
unbound. priv->phy_drv->read_status(phydev) is then going to do bad
things.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

On Thu, Jun 07, 2018 at 10:53:46AM -0500, Brandon Maier wrote:

Since a phy_device is added to the global mdio_bus list during
phy_device_register(), but a phy_device's phy_driver doesn't get
attached until phy_probe(). It's possible of_phy_find_device() in
xgmiitorgmii will return a valid phy with a NULL phy_driver. Leading to
a NULL pointer access during the memcpy().

Hi Brandon

FYI: net-next is closed at the moment. Please resubmit these in two
weeks time.

      Andrew

On Thu, Jun 7, 2018 at 11:52 AM, Andrew Lunn [off-list ref] wrote:

FYI: net-next is closed at the moment. Please resubmit these in two
weeks time.

Ah, I didn't see networking/netdev-FAQ.txt. I'll resubmit these then.

I'm sure there are more issues like this in the code.  e.g. there is
no attempt made to hold a reference to the child phy. So it could be
unbound. priv->phy_drv->read_status(phydev) is then going to do bad
things.

Agreed. Another thing that looks suspicious to me is the driver
overrides the private data of the device it's attaching too, in the
`priv->phy_dev->priv = priv` bit. Seems like that could cause all
sorts of driver corruption problems.

But fixing that is going to require more drastic changes to how this
driver works. So it'd be worth applying this patch in the mean time.

On Thu, Jun 7, 2018 at 11:49 AM, Andrew Lunn [off-list ref] wrote:

I think the assumption was they are both on the same bus.
However, they don't need to be.

That was my assumption as well. We ran into a scenario where they were
on separate buses.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

Thanks

Agreed. Another thing that looks suspicious to me is the driver
overrides the private data of the device it's attaching too, in the
`priv->phy_dev->priv = priv` bit. Seems like that could cause all
sorts of driver corruption problems.

Ah, yes. That is very broken. Many PHYs will just explode sometime
later, since they use phdev->priv.

But fixing that is going to require more drastic changes to how this
driver works. So it'd be worth applying this patch in the mean time.

Patches welcome.

	Andrew