| Message ID | 20241216042247.492287-1-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: mv643xx_eth: fix an OF node reference leak | expand |
On Mon, Dec 16, 2024 at 01:22:47PM +0900, Joe Hattori wrote: > Current implementation of mv643xx_eth_shared_of_add_port() calls > of_parse_phandle(), but does not release the refcount on error. Call > of_node_put() in the error path and in mv643xx_eth_shared_of_remove(). > > This bug was found by an experimental static analysis tool that I am > developing. > > Fixes: 76723bca2802 ("net: mv643xx_eth: add DT parsing support") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > Changes in v2: > - Insert a null check before accessing the platform data. > --- > drivers/net/ethernet/marvell/mv643xx_eth.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c > index a06048719e84..917ff7bd43d4 100644 > --- a/drivers/net/ethernet/marvell/mv643xx_eth.c > +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c > @@ -2705,8 +2705,12 @@ static struct platform_device *port_platdev[3]; > static void mv643xx_eth_shared_of_remove(void) > { > int n; > + struct mv643xx_eth_platform_data *pd; > > for (n = 0; n < 3; n++) { > + pd = dev_get_platdata(&port_platdev[n]->dev); > + if (pd) > + of_node_put(pd->phy_node); > platform_device_del(port_platdev[n]); > port_platdev[n] = NULL; > } > @@ -2769,8 +2773,10 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev, > } > > ppdev = platform_device_alloc(MV643XX_ETH_NAME, dev_num); > - if (!ppdev) > - return -ENOMEM; > + if (!ppdev) { > + ret = -ENOMEM; > + goto put_err; > + } > ppdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > ppdev->dev.of_node = pnp; > > @@ -2792,6 +2798,8 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev, > > port_err: > platform_device_put(ppdev); > +put_err: > + of_node_put(ppd.phy_node); > return ret; > } > Looks good Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> Thanks > -- > 2.34.1
On Mon, Dec 16, 2024 at 01:22:47PM +0900, Joe Hattori wrote: > Current implementation of mv643xx_eth_shared_of_add_port() calls > of_parse_phandle(), but does not release the refcount on error. Call > of_node_put() in the error path and in mv643xx_eth_shared_of_remove(). > > This bug was found by an experimental static analysis tool that I am > developing. > > Fixes: 76723bca2802 ("net: mv643xx_eth: add DT parsing support") > Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> > --- > Changes in v2: > - Insert a null check before accessing the platform data. > --- > drivers/net/ethernet/marvell/mv643xx_eth.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c > index a06048719e84..917ff7bd43d4 100644 > --- a/drivers/net/ethernet/marvell/mv643xx_eth.c > +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c > @@ -2705,8 +2705,12 @@ static struct platform_device *port_platdev[3]; > static void mv643xx_eth_shared_of_remove(void) > { > int n; > + struct mv643xx_eth_platform_data *pd; > > for (n = 0; n < 3; n++) { > + pd = dev_get_platdata(&port_platdev[n]->dev); You need another NULL check here. port_platdev[n] can be NULL so &port_platdev[n]->dev is NULL + 16. The call to dev_get_platdata() will crash. > + if (pd) > + of_node_put(pd->phy_node); > platform_device_del(port_platdev[n]); > port_platdev[n] = NULL; > } regards, dan carpenter
Thank you for your review. On 12/17/24 23:05, Dan Carpenter wrote: > On Mon, Dec 16, 2024 at 01:22:47PM +0900, Joe Hattori wrote: >> Current implementation of mv643xx_eth_shared_of_add_port() calls >> of_parse_phandle(), but does not release the refcount on error. Call >> of_node_put() in the error path and in mv643xx_eth_shared_of_remove(). >> >> This bug was found by an experimental static analysis tool that I am >> developing. >> >> Fixes: 76723bca2802 ("net: mv643xx_eth: add DT parsing support") >> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> >> --- >> Changes in v2: >> - Insert a null check before accessing the platform data. >> --- >> drivers/net/ethernet/marvell/mv643xx_eth.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c >> index a06048719e84..917ff7bd43d4 100644 >> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c >> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c >> @@ -2705,8 +2705,12 @@ static struct platform_device *port_platdev[3]; >> static void mv643xx_eth_shared_of_remove(void) >> { >> int n; >> + struct mv643xx_eth_platform_data *pd; >> >> for (n = 0; n < 3; n++) { >> + pd = dev_get_platdata(&port_platdev[n]->dev); > > You need another NULL check here. port_platdev[n] can be NULL so > &port_platdev[n]->dev is NULL + 16. The call to dev_get_platdata() > will crash. Yes, should have realized that. Addressed in the v3 patch. > >> + if (pd) >> + of_node_put(pd->phy_node); >> platform_device_del(port_platdev[n]); >> port_platdev[n] = NULL; >> } > > regards, > dan carpenter > Best, Joe
diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c index a06048719e84..917ff7bd43d4 100644 --- a/drivers/net/ethernet/marvell/mv643xx_eth.c +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c @@ -2705,8 +2705,12 @@ static struct platform_device *port_platdev[3]; static void mv643xx_eth_shared_of_remove(void) { int n; + struct mv643xx_eth_platform_data *pd; for (n = 0; n < 3; n++) { + pd = dev_get_platdata(&port_platdev[n]->dev); + if (pd) + of_node_put(pd->phy_node); platform_device_del(port_platdev[n]); port_platdev[n] = NULL; } @@ -2769,8 +2773,10 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev, } ppdev = platform_device_alloc(MV643XX_ETH_NAME, dev_num); - if (!ppdev) - return -ENOMEM; + if (!ppdev) { + ret = -ENOMEM; + goto put_err; + } ppdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); ppdev->dev.of_node = pnp; @@ -2792,6 +2798,8 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev, port_err: platform_device_put(ppdev); +put_err: + of_node_put(ppd.phy_node); return ret; }
Current implementation of mv643xx_eth_shared_of_add_port() calls of_parse_phandle(), but does not release the refcount on error. Call of_node_put() in the error path and in mv643xx_eth_shared_of_remove(). This bug was found by an experimental static analysis tool that I am developing. Fixes: 76723bca2802 ("net: mv643xx_eth: add DT parsing support") Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp> --- Changes in v2: - Insert a null check before accessing the platform data. --- drivers/net/ethernet/marvell/mv643xx_eth.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)