Skip to contentSkip to conversation

null dereference in ethoc_probe()

PATCH Applied Dan Carpenter 4 participants 6 messages May 22, 2010 – May 25, 2010

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

The patch 0baa080c75c: "ethoc: use system memory as buffer" introduced a potential null dereference.

1060 free:
1061 if (priv->dma_alloc)
^^^^^^^^^^^^^^^
priv can be null here.

1062 dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
1063 netdev->mem_start);
1064 free_netdev(netdev);

Also I think the error handling is not as complete as it should be. It seems like we should call devm_iounmap() and release some of the other resources as well.

regards,
dan carpenter

#1 lore raw
Quoted 12 lines · Dan Carpenter

The patch 0baa080c75c: "ethoc: use system memory as buffer" introduced a potential null dereference.

1060 free:
1061 if (priv->dma_alloc)
^^^^^^^^^^^^^^^
priv can be null here.

1062 dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
1063 netdev->mem_start);
1064 free_netdev(netdev);

Quoted 9 lines

Also I think the error handling is not as complete as it should be. It seems like we should call devm_iounmap() and release some of the other resources as well.

regards,
dan carpenter

Hi Dan,

Thank you for reporting this bug. I will submit patch to fix this mistake.

Best regards,
Thomas

#2 lore raw

PATCH 1/1 ethoc: fix null dereference in ethoc_probe

In reply to Dan Carpenter

Dan reported the patch 0baa080c75c: "ethoc: use system memory as buffer" introduced a potential null dereference.

1060 free:
1061 if (priv->dma_alloc)
^^^^^^^^^^^^^^^
priv can be null here.

He also suggested that the error handling is not complete.

This patch fixes the null priv issue and improves resources releasing in ethoc_probe() and ethoc_remove().

Reported-by Dan Carpenter <[email protected]>
Signed-off-by Thomas Chou <[email protected]>
---
 drivers/net/ethoc.c |   34 ++++++++++++++++++++++++++++++----
 1 files changed, 30 insertions(+), 4 deletions(-)
drivers/net/ethoc.c +30 -4
index a8d9250..fddd5f5 100644
@@ -174,6 +174,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size");
  * @iobase:	pointer to I/O memory region
  * @membase:	pointer to buffer memory region
  * @dma_alloc:	dma allocated buffer size
+ * @io_region_size:	I/O memory region size
  * @num_tx:	number of send buffers
  * @cur_tx:	last send buffer written
  * @dty_tx:	last buffer actually sent

@@ -193,6 +194,7 @@ struct ethoc {
 	void __iomem *iobase;
 	void __iomem *membase;
 	int dma_alloc;
+	resource_size_t io_region_size;
 
 	unsigned int num_tx;
 	unsigned int cur_tx;

@@ -944,6 +946,7 @@ static int ethoc_probe(struct platform_device *pdev)
 	priv = netdev_priv(netdev);
 	priv->netdev = netdev;
 	priv->dma_alloc = 0;
+	priv->io_region_size = mmio->end - mmio->start + 1;
 
 	priv->iobase = devm_ioremap_nocache(&pdev->dev, netdev->base_addr,
 			resource_size(mmio));

@@ -1049,20 +1052,34 @@ static int ethoc_probe(struct platform_device *pdev)
 	ret = register_netdev(netdev);
 	if (ret < 0) {
 		dev_err(&netdev->dev, "failed to register interface\n");
-		goto error;
+		goto error2;
 	}
 
 	goto out;
 
+error2:
+	netif_napi_del(&priv->napi);
 error:
 	mdiobus_unregister(priv->mdio);
 free_mdio:
 	kfree(priv->mdio->irq);
 	mdiobus_free(priv->mdio);
 free:
-	if (priv->dma_alloc)
-		dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
-			netdev->mem_start);
+	if (priv) {
+		if (priv->dma_alloc)
+			dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
+					  netdev->mem_start);
+		else if (priv->membase)
+			devm_iounmap(&pdev->dev, priv->membase);
+		if (priv->iobase)
+			devm_iounmap(&pdev->dev, priv->iobase);
+	}
+	if (mem)
+		devm_release_mem_region(&pdev->dev, mem->start,
+					mem->end - mem->start + 1);
+	if (mmio)
+		devm_release_mem_region(&pdev->dev, mmio->start,
+					mmio->end - mmio->start + 1);
 	free_netdev(netdev);
 out:
 	return ret;

@@ -1080,6 +1097,7 @@ static int ethoc_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 
 	if (netdev) {
+		netif_napi_del(&priv->napi);
 		phy_disconnect(priv->phy);
 		priv->phy = NULL;
 

@@ -1091,6 +1109,14 @@ static int ethoc_remove(struct platform_device *pdev)
 		if (priv->dma_alloc)
 			dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
 				netdev->mem_start);
+		else {
+			devm_iounmap(&pdev->dev, priv->membase);
+			devm_release_mem_region(&pdev->dev, netdev->mem_start,
+				netdev->mem_end - netdev->mem_start + 1);
+		}
+		devm_iounmap(&pdev->dev, priv->iobase);
+		devm_release_mem_region(&pdev->dev, netdev->base_addr,
+			priv->io_region_size);
 		unregister_netdev(netdev);
 		free_netdev(netdev);
 	}
Signature · 1 line
1.7.1.86.g0e460
maintainer Applied
#3 lore raw

Re: [PATCH] ethoc: fix null dereference in ethoc_probe

From: Thomas Chou <[email protected]>
Date: Mon, 24 May 2010 10:44:02 +0800

Quoted 8 lines

Dan reported the patch 0baa080c75c: "ethoc: use system memory as buffer" introduced a potential null dereference.

1060 free:
1061 if (priv->dma_alloc)
^^^^^^^^^^^^^^^
priv can be null here.

Quoted 7 lines

He also suggested that the error handling is not complete.

This patch fixes the null priv issue and improves resources releasing in ethoc_probe() and ethoc_remove().

Reported-by Dan Carpenter <[email protected]>
Signed-off-by Thomas Chou <[email protected]>

Applied.

#4 lore raw

Re: [Nios2-dev] [PATCH] ethoc: fix null dereference in ethoc_probe

In reply to Thomas Chou
Quoted 40 lines · Thomas Chou

Dan reported the patch 0baa080c75c: "ethoc: use system memory as buffer" introduced a potential null dereference.

1060 free:
1061 if (priv->dma_alloc)
^^^^^^^^^^^^^^^
priv can be null here.

He also suggested that the error handling is not complete.

This patch fixes the null priv issue and improves resources releasing in ethoc_probe() and ethoc_remove().

Reported-by Dan Carpenter <[email protected]>
Signed-off-by Thomas Chou <[email protected]>
---
 drivers/net/ethoc.c |   34 ++++++++++++++++++++++++++++++----
 1 files changed, 30 insertions(+), 4 deletions(-)
drivers/net/ethoc.c +2 -0
index a8d9250..fddd5f5 100644
@@ -174,6 +174,7 @@ MODULE_PARM_DESC(buffer_size, "DMA buffer allocation size");
  * @iobase:	pointer to I/O memory region
  * @membase:	pointer to buffer memory region
  * @dma_alloc:	dma allocated buffer size
+ * @io_region_size:	I/O memory region size
  * @num_tx:	number of send buffers
  * @cur_tx:	last send buffer written
  * @dty_tx:	last buffer actually sent

@@ -193,6 +194,7 @@ struct ethoc {
 	void __iomem *iobase;
 	void __iomem *membase;
 	int dma_alloc;
+	resource_size_t io_region_size;
 
 	unsigned int num_tx;
 	unsigned int cur_tx;

drivers/net/ethoc.c static int ethoc_probe(struct platform_device *pdev) · line 949

Quoted 4 lines
drivers/net/ethoc.c +1 -0
@@ -944,6 +946,7 @@ static int ethoc_probe(struct platform_device *pdev)
 	priv = netdev_priv(netdev);
 	priv->netdev = netdev;
 	priv->dma_alloc = 0;
+	priv->io_region_size = mmio->end - mmio->start + 1;

Better use the resource_size inline here.

drivers/net/ethoc.c static int ethoc_probe(struct platform_device *pdev) · line 952

Quoted 2 lines
drivers/net/ethoc.c +0 -0
 	priv->iobase = devm_ioremap_nocache(&pdev->dev, netdev->base_addr,
 			resource_size(mmio));

                        ^^^

Then you could use the io_region_size here to avoid calculating the size again.

Quoted 23 lines
drivers/net/ethoc.c +5 -4
@@ -1049,20 +1052,34 @@ static int ethoc_probe(struct platform_device *pdev)
 	ret = register_netdev(netdev);
 	if (ret < 0) {
 		dev_err(&netdev->dev, "failed to register interface\n");
-		goto error;
+		goto error2;
 	}
 
 	goto out;
 
+error2:
+	netif_napi_del(&priv->napi);
 error:
 	mdiobus_unregister(priv->mdio);
 free_mdio:
 	kfree(priv->mdio->irq);
 	mdiobus_free(priv->mdio);
 free:
-	if (priv->dma_alloc)
-		dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
-			netdev->mem_start);
+	if (priv) {
+		if (priv->dma_alloc)

drivers/net/ethoc.c static int ethoc_probe(struct platform_device *pdev) · line 1079

Quoted 10 lines
drivers/net/ethoc.c +10 -0
+			dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
+					  netdev->mem_start);
+		else if (priv->membase)
+			devm_iounmap(&pdev->dev, priv->membase);
+		if (priv->iobase)
+			devm_iounmap(&pdev->dev, priv->iobase);
+	}
+	if (mem)
+		devm_release_mem_region(&pdev->dev, mem->start,
+					mem->end - mem->start + 1);

resource_size again

drivers/net/ethoc.c static int ethoc_probe(struct platform_device *pdev) · line 1082

Quoted 3 lines
drivers/net/ethoc.c +3 -0
+	if (mmio)
+		devm_release_mem_region(&pdev->dev, mmio->start,
+					mmio->end - mmio->start + 1);

ditto

Quoted 26 lines
drivers/net/ethoc.c +9 -0
 	free_netdev(netdev);
 out:
 	return ret;

@@ -1080,6 +1097,7 @@ static int ethoc_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 
 	if (netdev) {
+		netif_napi_del(&priv->napi);
 		phy_disconnect(priv->phy);
 		priv->phy = NULL;
 

@@ -1091,6 +1109,14 @@ static int ethoc_remove(struct platform_device *pdev)
 		if (priv->dma_alloc)
 			dma_free_coherent(NULL, priv->dma_alloc, priv->membase,
 				netdev->mem_start);
+		else {
+			devm_iounmap(&pdev->dev, priv->membase);
+			devm_release_mem_region(&pdev->dev, netdev->mem_start,
+				netdev->mem_end - netdev->mem_start + 1);
+		}
+		devm_iounmap(&pdev->dev, priv->iobase);
+		devm_release_mem_region(&pdev->dev, netdev->base_addr,
+			priv->io_region_size);
 		unregister_netdev(netdev);
 		free_netdev(netdev);
 	}
#5 lore raw

Re: [Nios2-dev] [PATCH] ethoc: fix null dereference in ethoc_probe

Quoted 7 lines · Tobias Klauser

drivers/net/ethoc.c static int ethoc_probe(struct platform_device *pdev) · line 949

Quoted 5 lines
drivers/net/ethoc.c +1 -0
@@ -944,6 +946,7 @@ static int ethoc_probe(struct platform_device *pdev)
 	priv = netdev_priv(netdev);
 	priv->netdev = netdev;
 	priv->dma_alloc = 0;
+	priv->io_region_size = mmio->end - mmio->start + 1;

Better use the resource_size inline here.

Hi Tobias,

I just see your patch of resource_size from the git log. Sorry, I missed this one. I will send update on this. Thank you.

Cheers,
Thomas