[PATCH 0/3] net: dsa: lantiq_gswip: probe fixes and remove cleanup

[PATCH 0/3] net: dsa: lantiq_gswip: probe fixes and remove cleanup

[Johan Hovold]

This series fix a few issues found through inspection when fixing up new
bad uses of of_find_compatible_node() that have crept in since 4.19.

Note that these have only been compile tested.

Johan


Johan Hovold (3):
  net: dsa: lantiq_gswip: fix use-after-free on failed probe
  net: dsa: lantiq_gswip: fix OF child-node lookups
  net: dsa: lantiq_gswip: drop bogus drvdata check

 drivers/net/dsa/lantiq_gswip.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH 1/3] net: dsa: lantiq_gswip: fix use-after-free on failed probe

[Johan Hovold]

Make sure to disable and deregister the switch on late probe errors to
avoid use-after-free when the device-resource-managed switch is freed.

Fixes: 14fceff4771e ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
Cc: stable <stable@vger.kernel.org>	# 4.20
Cc: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/dsa/lantiq_gswip.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 693a67f45bef..b06c41c98de9 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1099,7 +1099,7 @@ static int gswip_probe(struct platform_device *pdev)
 		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
 			priv->hw_info->cpu_port);
 		err = -EINVAL;
-		goto mdio_bus;
+		goto disable_switch;
 	}
 
 	platform_set_drvdata(pdev, priv);
@@ -1109,6 +1109,9 @@ static int gswip_probe(struct platform_device *pdev)
 		 (version & GSWIP_VERSION_MOD_MASK) >> GSWIP_VERSION_MOD_SHIFT);
 	return 0;
 
+disable_switch:
+	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
+	dsa_unregister_switch(priv->ds);
 mdio_bus:
 	if (mdio_np)
 		mdiobus_unregister(priv->ds->slave_mii_bus);
-- 
2.20.1

[PATCH 2/3] net: dsa: lantiq_gswip: fix OF child-node lookups

[Johan Hovold]

Use the new of_get_compatible_child() helper to look up child nodes to
avoid ever matching non-child nodes elsewhere in the tree.

Also fix up the related struct device_node leaks.

Fixes: 14fceff4771e ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
Cc: stable <stable@vger.kernel.org>	# 4.20
Cc: Hauke Mehrtens <hauke@hauke-m.de>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/dsa/lantiq_gswip.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index b06c41c98de9..5792674dd4e4 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1069,10 +1069,10 @@ static int gswip_probe(struct platform_device *pdev)
 	version = gswip_switch_r(priv, GSWIP_VERSION);
 
 	/* bring up the mdio bus */
-	gphy_fw_np = of_find_compatible_node(pdev->dev.of_node, NULL,
-					     "lantiq,gphy-fw");
+	gphy_fw_np = of_get_compatible_child(dev->of_node, "lantiq,gphy-fw");
 	if (gphy_fw_np) {
 		err = gswip_gphy_fw_list(priv, gphy_fw_np, version);
+		of_node_put(gphy_fw_np);
 		if (err) {
 			dev_err(dev, "gphy fw probe failed\n");
 			return err;
@@ -1080,13 +1080,12 @@ static int gswip_probe(struct platform_device *pdev)
 	}
 
 	/* bring up the mdio bus */
-	mdio_np = of_find_compatible_node(pdev->dev.of_node, NULL,
-					  "lantiq,xrx200-mdio");
+	mdio_np = of_get_compatible_child(dev->of_node, "lantiq,xrx200-mdio");
 	if (mdio_np) {
 		err = gswip_mdio(priv, mdio_np);
 		if (err) {
 			dev_err(dev, "mdio probe failed\n");
-			goto gphy_fw;
+			goto put_mdio_node;
 		}
 	}
 
@@ -1115,7 +1114,8 @@ static int gswip_probe(struct platform_device *pdev)
 mdio_bus:
 	if (mdio_np)
 		mdiobus_unregister(priv->ds->slave_mii_bus);
-gphy_fw:
+put_mdio_node:
+	of_node_put(mdio_np);
 	for (i = 0; i < priv->num_gphy_fw; i++)
 		gswip_gphy_fw_remove(priv, &priv->gphy_fw[i]);
 	return err;
@@ -1134,8 +1134,10 @@ static int gswip_remove(struct platform_device *pdev)
 
 	dsa_unregister_switch(priv->ds);
 
-	if (priv->ds->slave_mii_bus)
+	if (priv->ds->slave_mii_bus) {
 		mdiobus_unregister(priv->ds->slave_mii_bus);
+		of_node_put(priv->ds->slave_mii_bus->dev.of_node);
+	}
 
 	for (i = 0; i < priv->num_gphy_fw; i++)
 		gswip_gphy_fw_remove(priv, &priv->gphy_fw[i]);
-- 
2.20.1

[PATCH 3/3] net: dsa: lantiq_gswip: drop bogus drvdata check

[Johan Hovold]

The platform-device driver data is set on successful probe and will
never be NULL on remove (or we have much bigger problems).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/dsa/lantiq_gswip.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
index 5792674dd4e4..27d092cab40e 100644
--- a/drivers/net/dsa/lantiq_gswip.c
+++ b/drivers/net/dsa/lantiq_gswip.c
@@ -1126,9 +1126,6 @@ static int gswip_remove(struct platform_device *pdev)
 	struct gswip_priv *priv = platform_get_drvdata(pdev);
 	int i;
 
-	if (!priv)
-		return 0;
-
 	/* disable the switch */
 	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
 
-- 
2.20.1

Re: [PATCH 1/3] net: dsa: lantiq_gswip: fix use-after-free on failed probe

[Andrew Lunn]

On Wed, Jan 16, 2019 at 11:23:33AM +0100, Johan Hovold wrote:
> Make sure to disable and deregister the switch on late probe errors to
> avoid use-after-free when the device-resource-managed switch is freed.
> 
> Fixes: 14fceff4771e ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable <stable@vger.kernel.org>	# 4.20
> Cc: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/net/dsa/lantiq_gswip.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 693a67f45bef..b06c41c98de9 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -1099,7 +1099,7 @@ static int gswip_probe(struct platform_device *pdev)
>  		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
>  			priv->hw_info->cpu_port);
>  		err = -EINVAL;
> -		goto mdio_bus;
> +		goto disable_switch;
>  	}
>  
>  	platform_set_drvdata(pdev, priv);
> @@ -1109,6 +1109,9 @@ static int gswip_probe(struct platform_device *pdev)
>  		 (version & GSWIP_VERSION_MOD_MASK) >> GSWIP_VERSION_MOD_SHIFT);
>  	return 0;
>  
> +disable_switch:
> +	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
> +	dsa_unregister_switch(priv->ds);

This is correct. But it would be nice if somebody in the future could
move the disabling of the switch to the inverse of the gswip_setup()
function to make the code symmetrical.

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

    Andrew

Re: [PATCH 2/3] net: dsa: lantiq_gswip: fix OF child-node lookups

[Andrew Lunn]

On Wed, Jan 16, 2019 at 11:23:34AM +0100, Johan Hovold wrote:
> Use the new of_get_compatible_child() helper to look up child nodes to
> avoid ever matching non-child nodes elsewhere in the tree.
> 
> Also fix up the related struct device_node leaks.
> 
> Fixes: 14fceff4771e ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable <stable@vger.kernel.org>	# 4.20
> Cc: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>

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

    Andrew

Re: [PATCH 3/3] net: dsa: lantiq_gswip: drop bogus drvdata check

[Andrew Lunn]

On Wed, Jan 16, 2019 at 11:23:35AM +0100, Johan Hovold wrote:
> The platform-device driver data is set on successful probe and will
> never be NULL on remove (or we have much bigger problems).
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

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

    Andrew

Re: [PATCH 1/3] net: dsa: lantiq_gswip: fix use-after-free on failed probe

[Hauke Mehrtens]

On 1/16/19 11:23 AM, Johan Hovold wrote:
> Make sure to disable and deregister the switch on late probe errors to
> avoid use-after-free when the device-resource-managed switch is freed.
> 
> Fixes: 14fceff4771e ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable <stable@vger.kernel.org>	# 4.20
> Cc: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Acked-by: Hauke Mehrtens <hauke@hauke-m.de>

> ---
>  drivers/net/dsa/lantiq_gswip.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
> index 693a67f45bef..b06c41c98de9 100644
> --- a/drivers/net/dsa/lantiq_gswip.c
> +++ b/drivers/net/dsa/lantiq_gswip.c
> @@ -1099,7 +1099,7 @@ static int gswip_probe(struct platform_device *pdev)
>  		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
>  			priv->hw_info->cpu_port);
>  		err = -EINVAL;
> -		goto mdio_bus;
> +		goto disable_switch;
>  	}
>  
>  	platform_set_drvdata(pdev, priv);
> @@ -1109,6 +1109,9 @@ static int gswip_probe(struct platform_device *pdev)
>  		 (version & GSWIP_VERSION_MOD_MASK) >> GSWIP_VERSION_MOD_SHIFT);
>  	return 0;
>  
> +disable_switch:
> +	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
> +	dsa_unregister_switch(priv->ds);
>  mdio_bus:
>  	if (mdio_np)
>  		mdiobus_unregister(priv->ds->slave_mii_bus);
> 

Re: [PATCH 1/3] net: dsa: lantiq_gswip: fix use-after-free on failed probe

[Hauke Mehrtens]

On 1/16/19 4:00 PM, Andrew Lunn wrote:
> On Wed, Jan 16, 2019 at 11:23:33AM +0100, Johan Hovold wrote:
>> Make sure to disable and deregister the switch on late probe errors to
>> avoid use-after-free when the device-resource-managed switch is freed.
>>
>> Fixes: 14fceff4771e ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
>> Cc: stable <stable@vger.kernel.org>	# 4.20
>> Cc: Hauke Mehrtens <hauke@hauke-m.de>
>> Signed-off-by: Johan Hovold <johan@kernel.org>
>> ---
>>  drivers/net/dsa/lantiq_gswip.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/lantiq_gswip.c b/drivers/net/dsa/lantiq_gswip.c
>> index 693a67f45bef..b06c41c98de9 100644
>> --- a/drivers/net/dsa/lantiq_gswip.c
>> +++ b/drivers/net/dsa/lantiq_gswip.c
>> @@ -1099,7 +1099,7 @@ static int gswip_probe(struct platform_device *pdev)
>>  		dev_err(dev, "wrong CPU port defined, HW only supports port: %i",
>>  			priv->hw_info->cpu_port);
>>  		err = -EINVAL;
>> -		goto mdio_bus;
>> +		goto disable_switch;
>>  	}
>>  
>>  	platform_set_drvdata(pdev, priv);
>> @@ -1109,6 +1109,9 @@ static int gswip_probe(struct platform_device *pdev)
>>  		 (version & GSWIP_VERSION_MOD_MASK) >> GSWIP_VERSION_MOD_SHIFT);
>>  	return 0;
>>  
>> +disable_switch:
>> +	gswip_mdio_mask(priv, GSWIP_MDIO_GLOB_ENABLE, 0, GSWIP_MDIO_GLOB);
>> +	dsa_unregister_switch(priv->ds);
> 
> This is correct. But it would be nice if somebody in the future could
> move the disabling of the switch to the inverse of the gswip_setup()
> function to make the code symmetrical.

Should we add an uninit callback?

Hauke

Re: [PATCH 2/3] net: dsa: lantiq_gswip: fix OF child-node lookups

[Hauke Mehrtens]

On 1/16/19 11:23 AM, Johan Hovold wrote:
> Use the new of_get_compatible_child() helper to look up child nodes to
> avoid ever matching non-child nodes elsewhere in the tree.
> 
> Also fix up the related struct device_node leaks.
> 
> Fixes: 14fceff4771e ("net: dsa: Add Lantiq / Intel DSA driver for vrx200")
> Cc: stable <stable@vger.kernel.org>	# 4.20
> Cc: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Acked-by: Hauke Mehrtens <hauke@hauke-m.de>

> ---
>  drivers/net/dsa/lantiq_gswip.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 

Re: [PATCH 3/3] net: dsa: lantiq_gswip: drop bogus drvdata check

[Hauke Mehrtens]

On 1/16/19 11:23 AM, Johan Hovold wrote:
> The platform-device driver data is set on successful probe and will
> never be NULL on remove (or we have much bigger problems).
> 
> Signed-off-by: Johan Hovold <johan@kernel.org>

Acked-by: Hauke Mehrtens <hauke@hauke-m.de>

> ---
>  drivers/net/dsa/lantiq_gswip.c | 3 ---
>  1 file changed, 3 deletions(-)
> 

Re: [PATCH 1/3] net: dsa: lantiq_gswip: fix use-after-free on failed probe

[Andrew Lunn]

> > This is correct. But it would be nice if somebody in the future could
> > move the disabling of the switch to the inverse of the gswip_setup()
> > function to make the code symmetrical.
> 
> Should we add an uninit callback?

Yes, that would be good.

It looks like lan9303-core.c could use it as well for
lan9303_disable_processing(chip);

Thanks
	Andrew

Re: [PATCH 0/3] net: dsa: lantiq_gswip: probe fixes and remove cleanup

[David Miller]

From: Johan Hovold <johan@kernel.org>
Date: Wed, 16 Jan 2019 11:23:32 +0100

> This series fix a few issues found through inspection when fixing up new
> bad uses of of_find_compatible_node() that have crept in since 4.19.
> 
> Note that these have only been compile tested.

Series applied.