net: ethernet: mediatek: add NULL check on of_match_device() return value

PATCH In review Gustavo A. R. Silva 5 participants 7 messages Jul 7, 2017 – Jul 8, 2017

Subsystems (4)

Networking Network drivers Arm MediaTek MediaTek Ethernet driver

lore

Share & tools

lore

Gustavo A. R. Silva

author

2017-07-07 07:11 UTC #0 lore raw

PATCH 1/1 net: ethernet: mediatek: add NULL check on of_match_device() return value

Check return value from call to of_match_device()
in order to prevent a NULL pointer dereference.

In case of NULL print error message and return -ENODEV

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/net/ethernet/mediatek/mtk_eth_soc.c +5 -0

index adaaafc..6a77dea 100644
@@ -2408,6 +2408,11 @@ static int mtk_probe(struct platform_device *pdev)
 	int i;
 
 	match = of_match_device(of_mtk_match, &pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "failed to match device\n");
+		return -ENODEV;
+	}
+
 	soc = (struct mtk_soc_data *)match->data;
 
 	eth = devm_kzalloc(&pdev->dev, sizeof(*eth), GFP_KERNEL);

Signature · 1 line

2.5.0

Matthias Brugger

maintainer Reviewed-by on v1 1/1

2017-07-07 14:51 UTC #1 lore raw

Quoted 7 lines · Gustavo A. R. Silva

Check return value from call to of_match_device()
in order to prevent a NULL pointer dereference.
In case of NULL print error message and return -ENODEV
Signed-off-by Gustavo A. R. Silva <[email protected]>
---

Reviewed-by Matthias Brugger <[email protected]>

Quoted 20 lines

  drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 +++++
  1 file changed, 5 insertions(+)

drivers/net/ethernet/mediatek/mtk_eth_soc.c +5 -0

index adaaafc..6a77dea 100644
@@ -2408,6 +2408,11 @@ static int mtk_probe(struct platform_device *pdev)
  	int i;
 
  	match = of_match_device(of_mtk_match, &pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "failed to match device\n");
+		return -ENODEV;
+	}
+
  	soc = (struct mtk_soc_data *)match->data;
 
  	eth = devm_kzalloc(&pdev->dev, sizeof(*eth), GFP_KERNEL);

Andrew Lunn

2017-07-07 14:57 UTC #2 lore raw

In reply to Gustavo A. R. Silva

Quoted 15 lines · Gustavo A. R. Silva

Check return value from call to of_match_device()
in order to prevent a NULL pointer dereference.
In case of NULL print error message and return -ENODEV
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -0
index adaaafc..6a77dea 100644

drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_probe(struct platform_device *pdev) · line 2416

Quoted 9 lines

drivers/net/ethernet/mediatek/mtk_eth_soc.c +5 -0

@@ -2408,6 +2408,11 @@ static int mtk_probe(struct platform_device *pdev)
 	int i;
 
 	match = of_match_device(of_mtk_match, &pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "failed to match device\n");
+		return -ENODEV;
+	}
+
 	soc = (struct mtk_soc_data *)match->data;

Hi Gustavo

I think you are fixing the wrong thing. The soc variable is unused. Also,

const struct of_device_id of_mtk_match[] = {
        { .compatible = "mediatek,mt2701-eth" },
        {},
};

So match->data is NULL. This code is all pointless and should be removed, rather than try to avoid a NULL pointer dereference which can not actually happen.

Andrew

Gustavo A. R. Silva

author

2017-07-07 20:08 UTC #3 lore raw

Hi Andrew,

Quoting Andrew Lunn <[email protected]>:

Quoted 31 lines · Gustavo A. R. Silva

Quoted 16 lines
Check return value from call to of_match_device()
in order to prevent a NULL pointer dereference.
In case of NULL print error message and return -ENODEV
Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -0
index adaaafc..6a77dea 100644
drivers/net/ethernet/mediatek/mtk_eth_soc.c static int mtk_probe(struct platform_device *pdev) · line 2416
Quoted 9 lines
drivers/net/ethernet/mediatek/mtk_eth_soc.c +5 -0
@@ -2408,6 +2408,11 @@ static int mtk_probe(struct platform_device *pdev)
 	int i;
 
 	match = of_match_device(of_mtk_match, &pdev->dev);
+	if (!match) {
+		dev_err(&pdev->dev, "failed to match device\n");
+		return -ENODEV;
+	}
+
 	soc = (struct mtk_soc_data *)match->data;
Hi Gustavo
I think you are fixing the wrong thing. The soc variable is unused. Also,

Quoted 9 lines

const struct of_device_id of_mtk_match[] = {
        { .compatible = "mediatek,mt2701-eth" },
        {},
};
So match->data is NULL. This code is all pointless and should be removed, rather than try to avoid a NULL pointer dereference which can not actually happen.

You are right. Thank you for pointing it out. I'll remove that code and send a new patch shortly.

Thanks!
--
Gustavo A. R. Silva

Gustavo A. R. Silva

author

2017-07-07 20:23 UTC #4 lore raw

PATCH 1/1 net: ethernet: mediatek: remove useless code in mtk_probe()

Remove useless local variables _match_, _soc_ and the code related.

Notice that

const struct of_device_id of_mtk_match[] = {
        { .compatible = "mediatek,mt2701-eth" },
        {},
};

So match->data is NULL.

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 -----
 1 file changed, 5 deletions(-)

drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -5

index adaaafc..b9a5a65 100644
@@ -2401,15 +2401,10 @@ static int mtk_probe(struct platform_device *pdev)
 {
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct device_node *mac_np;
-	const struct of_device_id *match;
-	struct mtk_soc_data *soc;
 	struct mtk_eth *eth;
 	int err;
 	int i;
 
-	match = of_match_device(of_mtk_match, &pdev->dev);
-	soc = (struct mtk_soc_data *)match->data;
-
 	eth = devm_kzalloc(&pdev->dev, sizeof(*eth), GFP_KERNEL);
 	if (!eth)
 		return -ENOMEM;

Signature · 1 line

2.5.0

Sean Wang

2017-07-08 06:56 UTC #5 lore raw

Re: [PATCH] net: ethernet: mediatek: remove useless code in mtk_probe()

Hi, Gustavo

It indeed is useless at the current time point.

but actually I will add new SoC support to the driver in the next week, which requires the variable match :-(

Sean

Quoted 37 lines · Gustavo A. R. Silva

Remove useless local variables _match_, _soc_ and the code related.

Notice that

const struct of_device_id of_mtk_match[] = {
        { .compatible = "mediatek,mt2701-eth" },
        {},
};

So match->data is NULL.

Signed-off-by Gustavo A. R. Silva <[email protected]>

---
 drivers/net/ethernet/mediatek/mtk_eth_soc.c | 5 -----
 1 file changed, 5 deletions(-)

drivers/net/ethernet/mediatek/mtk_eth_soc.c +0 -5

index adaaafc..b9a5a65 100644
@@ -2401,15 +2401,10 @@ static int mtk_probe(struct platform_device *pdev)
 {
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct device_node *mac_np;
-	const struct of_device_id *match;
-	struct mtk_soc_data *soc;
 	struct mtk_eth *eth;
 	int err;
 	int i;
 
-	match = of_match_device(of_mtk_match, &pdev->dev);
-	soc = (struct mtk_soc_data *)match->data;
-
 	eth = devm_kzalloc(&pdev->dev, sizeof(*eth), GFP_KERNEL);
 	if (!eth)
 		return -ENOMEM;

David Miller

maintainer Applied

2017-07-08 10:28 UTC #6 lore raw

Re: [PATCH] net: ethernet: mediatek: remove useless code in mtk_probe()

In reply to Gustavo A. R. Silva

From: "Gustavo A. R. Silva" <[email protected]>
Date: Fri, 7 Jul 2017 15:23:34 -0500

Quoted 9 lines

Remove useless local variables _match_, _soc_ and the code related.
Notice that
const struct of_device_id of_mtk_match[] = {
        { .compatible = "mediatek,mt2701-eth" },
        {},
};

Quoted 4 lines

So match->data is NULL.
Suggested-by Andrew Lunn <[email protected]>
Signed-off-by Gustavo A. R. Silva <[email protected]>

Applied, thanks.

If someone needs this they can it back, in a less buggy form.