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
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 <[email protected]> # 4.20
Cc: Hauke Mehrtens <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]> # 4.20
Cc: Hauke Mehrtens <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
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
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 <[email protected]> # 4.20
> Cc: Hauke Mehrtens <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
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 <[email protected]>
Acked-by: Hauke Mehrtens <[email protected]>
> ---
> drivers/net/dsa/lantiq_gswip.c | 3 ---
> 1 file changed, 3 deletions(-)
>
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 <[email protected]> # 4.20
> Cc: Hauke Mehrtens <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Acked-by: Hauke Mehrtens <[email protected]>
> ---
> 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);
>
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 <[email protected]> # 4.20
>> Cc: Hauke Mehrtens <[email protected]>
>> Signed-off-by: Johan Hovold <[email protected]>
>> ---
>> 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
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 <[email protected]> # 4.20
> Cc: Hauke Mehrtens <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Acked-by: Hauke Mehrtens <[email protected]>
> ---
> drivers/net/dsa/lantiq_gswip.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> > 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
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 <[email protected]> # 4.20
> Cc: Hauke Mehrtens <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> 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 <[email protected]>
Andrew
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 <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
From: Johan Hovold <[email protected]>
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.