[PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Joe Hattori]

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;
 }
 
-- 
2.34.1

Re: [PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Michal Swiatkowski]

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

Re: [PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Dan Carpenter]

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

[PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Joe Hattori]

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 verification 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 v3:
- Insert a NULL check for port_platdev[n].
Changes in v2:
- Insert a NULL check before accessing the platform data.
---
 drivers/net/ethernet/marvell/mv643xx_eth.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
index a06048719e84..0e2ebfcaad1c 100644
--- a/drivers/net/ethernet/marvell/mv643xx_eth.c
+++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
@@ -2705,8 +2705,14 @@ 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++) {
+		if (!port_platdev[n])
+			continue;
+		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 +2775,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 +2800,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;
 }
 
-- 
2.34.1

Re: [PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Joe Hattori]

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

Re: [PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Paolo Abeni]

On 12/18/24 02:28, 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 verification 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 v3:
> - Insert a NULL check for port_platdev[n].
> Changes in v2:
> - Insert a NULL check before accessing the platform data.

I'm sorry for nit-picking, but many little things are adding-up and
should be noticed.

The subj prefix must include the correct revision number (v3 in this case).
You must avoid submitting new revisions within the 24h grace period, see:

https://elixir.bootlin.com/linux/v6.12.5/source/Documentation/process/maintainer-netdev.rst#L414

> ---
>  drivers/net/ethernet/marvell/mv643xx_eth.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index a06048719e84..0e2ebfcaad1c 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c
> @@ -2705,8 +2705,14 @@ static struct platform_device *port_platdev[3];
>  static void mv643xx_eth_shared_of_remove(void)
>  {
>  	int n;
> +	struct mv643xx_eth_platform_data *pd;

Please respect the reverse xmas tree above, see:

https://elixir.bootlin.com/linux/v6.12.5/source/Documentation/process/maintainer-netdev.rst#L360

Otherwise LGTM, thanks!

Paolo

Re: [PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Paolo Abeni]

On 12/19/24 11:21, Paolo Abeni wrote:
> On 12/18/24 02:28, 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 verification 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 v3:
>> - Insert a NULL check for port_platdev[n].
>> Changes in v2:
>> - Insert a NULL check before accessing the platform data.
> 
> I'm sorry for nit-picking, but many little things are adding-up and
> should be noticed.
> 
> The subj prefix must include the correct revision number (v3 in this case).

Oops, I almost forgot... the patch subj prefix must additionally include
the target tree - 'net' in this case - see:

https://elixir.bootlin.com/linux/v6.12.5/source/Documentation/process/maintainer-netdev.rst#L332

Thanks,

Paolo

Re: [PATCH v2] net: mv643xx_eth: fix an OF node reference leak

[Joe Hattori]

Thank you for your review.

On 12/19/24 19:23, Paolo Abeni wrote:
> On 12/19/24 11:21, Paolo Abeni wrote:
>> On 12/18/24 02:28, 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 verification 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 v3:
>>> - Insert a NULL check for port_platdev[n].
>>> Changes in v2:
>>> - Insert a NULL check before accessing the platform data.
>>
>> I'm sorry for nit-picking, but many little things are adding-up and
>> should be noticed.
>>
>> The subj prefix must include the correct revision number (v3 in this case).
> 
> Oops, I almost forgot... the patch subj prefix must additionally include
> the target tree - 'net' in this case - see:
> 
> https://elixir.bootlin.com/linux/v6.12.5/source/Documentation/process/maintainer-netdev.rst#L332

Thank you for pointing out. All the points you raised have been fixed in 
the v4 patch.

> 
> Thanks,
> 
> Paolo
> 

Best,
Joe