[PATCH] net: davicom: fix UAF in dm9000_drv_remove

[PATCH] net: davicom: fix UAF in dm9000_drv_remove

[Chenyuan Yang]

dm is netdev private data and it cannot be
used after free_netdev() call. Using adpt after free_netdev()
can cause UAF bug. Fix it by moving free_netdev() at the end of the
function.

This is similar to the issue fixed in commit
ad297cd2db8953e2202970e9504cab247b6c7cb4 ("net: qcom/emac: fix UAF in emac_remove").

Fixes: cf9e60aa69ae ("net: davicom: Fix regulator not turned off on driver removal")
Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
---
 drivers/net/ethernet/davicom/dm9000.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
index 8735e333034c..b87eaf0c250c 100644
--- a/drivers/net/ethernet/davicom/dm9000.c
+++ b/drivers/net/ethernet/davicom/dm9000.c
@@ -1777,10 +1777,11 @@ static void dm9000_drv_remove(struct platform_device *pdev)
 
 	unregister_netdev(ndev);
 	dm9000_release_board(pdev, dm);
-	free_netdev(ndev);		/* free device structure */
 	if (dm->power_supply)
 		regulator_disable(dm->power_supply);
 
+	free_netdev(ndev);		/* free device structure */
+
 	dev_dbg(&pdev->dev, "released and freed device\n");
 }
 
-- 
2.34.1

Re: [PATCH] net: davicom: fix UAF in dm9000_drv_remove

[Michal Swiatkowski]

On Mon, Jan 20, 2025 at 04:25:57PM -0600, Chenyuan Yang wrote:
> dm is netdev private data and it cannot be
> used after free_netdev() call. Using adpt after free_netdev()
> can cause UAF bug. Fix it by moving free_netdev() at the end of the
> function.
> 
> This is similar to the issue fixed in commit
> ad297cd2db8953e2202970e9504cab247b6c7cb4 ("net: qcom/emac: fix UAF in emac_remove").
> 
> Fixes: cf9e60aa69ae ("net: davicom: Fix regulator not turned off on driver removal")
> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> ---
>  drivers/net/ethernet/davicom/dm9000.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index 8735e333034c..b87eaf0c250c 100644
> --- a/drivers/net/ethernet/davicom/dm9000.c
> +++ b/drivers/net/ethernet/davicom/dm9000.c
> @@ -1777,10 +1777,11 @@ static void dm9000_drv_remove(struct platform_device *pdev)
>  
>  	unregister_netdev(ndev);
>  	dm9000_release_board(pdev, dm);
> -	free_netdev(ndev);		/* free device structure */
>  	if (dm->power_supply)
>  		regulator_disable(dm->power_supply);
>  
> +	free_netdev(ndev);		/* free device structure */
> +
>  	dev_dbg(&pdev->dev, "released and freed device\n");
>  }

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

>  
> -- 
> 2.34.1

Re: [PATCH] net: davicom: fix UAF in dm9000_drv_remove

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

Hello,

On Mon, Jan 20, 2025 at 04:25:57PM -0600, Chenyuan Yang wrote:
> dm is netdev private data and it cannot be
> used after free_netdev() call. Using adpt after free_netdev()

What is adpt?

> can cause UAF bug. Fix it by moving free_netdev() at the end of the
> function.

"can cause"? Doesn't that trigger reliable?

How did you find that issue? Did this actually trigger for you, or is it
a static checker that found it? Please mention that in the commit log.
 
> This is similar to the issue fixed in commit
> ad297cd2db8953e2202970e9504cab247b6c7cb4 ("net: qcom/emac: fix UAF in emac_remove").

Please shorten the commit id, typically to 12 chars as you did in the
Fixes line below.

> Fixes: cf9e60aa69ae ("net: davicom: Fix regulator not turned off on driver removal")
> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: davicom: fix UAF in dm9000_drv_remove

[Chenyuan Yang]

Hello,

Thanks for pointing this out!

On Mon, Jan 20, 2025 at 11:33 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>
> Hello,
>
> On Mon, Jan 20, 2025 at 04:25:57PM -0600, Chenyuan Yang wrote:
> > dm is netdev private data and it cannot be
> > used after free_netdev() call. Using adpt after free_netdev()
>
> What is adpt?

This should be "dm".

> > can cause UAF bug. Fix it by moving free_netdev() at the end of the
> > function.
>
> "can cause"? Doesn't that trigger reliable?
>
> How did you find that issue? Did this actually trigger for you, or is it
> a static checker that found it? Please mention that in the commit log.

This is detected by our static checker. Thus, we don't have a
test-case to trigger it stably.
Basically, it has the buggy pattern as the commit mentioned below.

> > This is similar to the issue fixed in commit
> > ad297cd2db8953e2202970e9504cab247b6c7cb4 ("net: qcom/emac: fix UAF in emac_remove").
>
> Please shorten the commit id, typically to 12 chars as you did in the
> Fixes line below.

Sure! Should I send a Patch v2 for this commit?

> > Fixes: cf9e60aa69ae ("net: davicom: Fix regulator not turned off on driver removal")
> > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
>
> Best regards
> Uwe

Best,
Chenyuan

Re: [PATCH] net: davicom: fix UAF in dm9000_drv_remove

[Paolo Abeni]

On 1/21/25 9:04 PM, Chenyuan Yang wrote:
> On Mon, Jan 20, 2025 at 11:33 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
>> On Mon, Jan 20, 2025 at 04:25:57PM -0600, Chenyuan Yang wrote:
>>> dm is netdev private data and it cannot be
>>> used after free_netdev() call. Using adpt after free_netdev()
>>
>> What is adpt?
> 
> This should be "dm".
> 
>>> can cause UAF bug. Fix it by moving free_netdev() at the end of the
>>> function.
>>
>> "can cause"? Doesn't that trigger reliable?
>>
>> How did you find that issue? Did this actually trigger for you, or is it
>> a static checker that found it? Please mention that in the commit log.
> 
> This is detected by our static checker. Thus, we don't have a
> test-case to trigger it stably.
> Basically, it has the buggy pattern as the commit mentioned below.
> 
>>> This is similar to the issue fixed in commit
>>> ad297cd2db8953e2202970e9504cab247b6c7cb4 ("net: qcom/emac: fix UAF in emac_remove").
>>
>> Please shorten the commit id, typically to 12 chars as you did in the
>> Fixes line below.
> 
> Sure! Should I send a Patch v2 for this commit?

Please do! while at it, please also include the target tree ('net') in
the subj prefix.

Thanks,

Paolo