* null dereference in ethoc_probe()
@ 2010-05-22 15:55 Dan Carpenter
2010-05-24 0:29 ` Thomas Chou
2010-05-24 2:44 ` [PATCH] ethoc: fix null dereference in ethoc_probe Thomas Chou
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2010-05-22 15:55 UTC (permalink / raw)
To: thomas; +Cc: netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: null dereference in ethoc_probe() 2010-05-22 15:55 null dereference in ethoc_probe() Dan Carpenter @ 2010-05-24 0:29 ` Thomas Chou 2010-05-24 2:44 ` [PATCH] ethoc: fix null dereference in ethoc_probe Thomas Chou 1 sibling, 0 replies; 6+ messages in thread From: Thomas Chou @ 2010-05-24 0:29 UTC (permalink / raw) To: Dan Carpenter; +Cc: netdev Dan Carpenter wrote: > 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 > > > Hi Dan, Thank you for reporting this bug. I will submit patch to fix this mistake. Best regards, Thomas ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ethoc: fix null dereference in ethoc_probe 2010-05-22 15:55 null dereference in ethoc_probe() Dan Carpenter 2010-05-24 0:29 ` Thomas Chou @ 2010-05-24 2:44 ` Thomas Chou 2010-05-24 6:11 ` David Miller 2010-05-25 6:16 ` [Nios2-dev] " Tobias Klauser 1 sibling, 2 replies; 6+ messages in thread From: Thomas Chou @ 2010-05-24 2:44 UTC (permalink / raw) To: netdev; +Cc: linux-kernel, thierry.reding, nios2-dev, Dan Carpenter, 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 <error27@gmail.com> Signed-off-by: Thomas Chou <thomas@wytron.com.tw> --- drivers/net/ethoc.c | 34 ++++++++++++++++++++++++++++++---- 1 files changed, 30 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethoc.c b/drivers/net/ethoc.c index a8d9250..fddd5f5 100644 --- a/drivers/net/ethoc.c +++ b/drivers/net/ethoc.c @@ -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); } -- 1.7.1.86.g0e460 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ethoc: fix null dereference in ethoc_probe 2010-05-24 2:44 ` [PATCH] ethoc: fix null dereference in ethoc_probe Thomas Chou @ 2010-05-24 6:11 ` David Miller 2010-05-25 6:16 ` [Nios2-dev] " Tobias Klauser 1 sibling, 0 replies; 6+ messages in thread From: David Miller @ 2010-05-24 6:11 UTC (permalink / raw) To: thomas; +Cc: netdev, linux-kernel, thierry.reding, nios2-dev, error27 From: Thomas Chou <thomas@wytron.com.tw> Date: Mon, 24 May 2010 10:44:02 +0800 > 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 <error27@gmail.com> > Signed-off-by: Thomas Chou <thomas@wytron.com.tw> Applied. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Nios2-dev] [PATCH] ethoc: fix null dereference in ethoc_probe 2010-05-24 2:44 ` [PATCH] ethoc: fix null dereference in ethoc_probe Thomas Chou 2010-05-24 6:11 ` David Miller @ 2010-05-25 6:16 ` Tobias Klauser 2010-05-25 6:51 ` Thomas Chou 1 sibling, 1 reply; 6+ messages in thread From: Tobias Klauser @ 2010-05-25 6:16 UTC (permalink / raw) To: nios2-dev; +Cc: netdev, thierry.reding, linux-kernel, Dan Carpenter On 05/24/2010 04:44 AM, Thomas Chou wrote: > 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 <error27@gmail.com> > Signed-off-by: Thomas Chou <thomas@wytron.com.tw> > --- > drivers/net/ethoc.c | 34 ++++++++++++++++++++++++++++++---- > 1 files changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethoc.c b/drivers/net/ethoc.c > index a8d9250..fddd5f5 100644 > --- a/drivers/net/ethoc.c > +++ b/drivers/net/ethoc.c > @@ -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; Better use the resource_size inline here. > 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. > @@ -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); resource_size again > + if (mmio) > + devm_release_mem_region(&pdev->dev, mmio->start, > + mmio->end - mmio->start + 1); ditto > 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); > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Nios2-dev] [PATCH] ethoc: fix null dereference in ethoc_probe 2010-05-25 6:16 ` [Nios2-dev] " Tobias Klauser @ 2010-05-25 6:51 ` Thomas Chou 0 siblings, 0 replies; 6+ messages in thread From: Thomas Chou @ 2010-05-25 6:51 UTC (permalink / raw) To: nios2-dev; +Cc: netdev, thierry.reding, linux-kernel, Dan Carpenter Tobias Klauser wrote: >> @@ -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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-05-25 6:51 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-05-22 15:55 null dereference in ethoc_probe() Dan Carpenter 2010-05-24 0:29 ` Thomas Chou 2010-05-24 2:44 ` [PATCH] ethoc: fix null dereference in ethoc_probe Thomas Chou 2010-05-24 6:11 ` David Miller 2010-05-25 6:16 ` [Nios2-dev] " Tobias Klauser 2010-05-25 6:51 ` Thomas Chou
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).