[PATCH] net: mana: Fix possible double free in error handling path

[PATCH] net: mana: Fix possible double free in error handling path

[Ma Ke]

When auxiliary_device_add() returns error and then calls
auxiliary_device_uninit(), callback function adev_release
calls kfree(madev) to free memory. We shouldn't call kfree(padev)
again in the error handling path.

Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/net/ethernet/microsoft/mana/mana_en.c | 31 +++++++++----------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c b/drivers/net/ethernet/microsoft/mana/mana_en.c
index d087cf954f75..1754c92a6c15 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_en.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
@@ -2785,8 +2785,10 @@ static int add_adev(struct gdma_dev *gd)
 
 	adev = &madev->adev;
 	ret = mana_adev_idx_alloc();
-	if (ret < 0)
-		goto idx_fail;
+	if (ret < 0) {
+		kfree(madev);
+		return ret;
+	}
 	adev->id = ret;
 
 	adev->name = "rdma";
@@ -2795,26 +2797,21 @@ static int add_adev(struct gdma_dev *gd)
 	madev->mdev = gd;
 
 	ret = auxiliary_device_init(adev);
-	if (ret)
-		goto init_fail;
+	if (ret) {
+		mana_adev_idx_free(adev->id);
+		kfree(madev);
+		return ret;
+	}
 
 	ret = auxiliary_device_add(adev);
-	if (ret)
-		goto add_fail;
+	if (ret) {
+		auxiliary_device_uninit(adev);
+		mana_adev_idx_free(adev->id);
+		return ret;
+	}
 
 	gd->adev = adev;
 	return 0;
-
-add_fail:
-	auxiliary_device_uninit(adev);
-
-init_fail:
-	mana_adev_idx_free(adev->id);
-
-idx_fail:
-	kfree(madev);
-
-	return ret;
 }
 
 int mana_probe(struct gdma_dev *gd, bool resuming)
-- 
2.25.1

RE: [PATCH] net: mana: Fix possible double free in error handling path

[Sai Krishna Gajula]

> -----Original Message-----
> From: Ma Ke <make24@iscas.ac.cn>
> Sent: Monday, June 24, 2024 8:51 AM
> To: kys@microsoft.com; haiyangz@microsoft.com; wei.liu@kernel.org;
> decui@microsoft.com; davem@davemloft.net; edumazet@google.com;
> kuba@kernel.org; pabeni@redhat.com; shradhagupta@linux.microsoft.com;
> horms@kernel.org; kotaranov@microsoft.com; linyunsheng@huawei.com;
> schakrabarti@linux.microsoft.com; make24@iscas.ac.cn;
> erick.archer@outlook.com
> Cc: linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH] net: mana: Fix possible double free in error
> handling path
> 
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), callback function adev_release calls kfree(madev)
> to free memory. We shouldn't call kfree(padev) again in the error handling
> path. Signed-off-by: Ma Ke <make24@ iscas. ac. cn>

> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), callback function adev_release calls kfree(madev)
> to free memory. We shouldn't call kfree(padev) again in the error handling
> path.
> 
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 31 +++++++++----------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microsoft/mana/mana_en.c
> b/drivers/net/ethernet/microsoft/mana/mana_en.c
> index d087cf954f75..1754c92a6c15 100644
> --- a/drivers/net/ethernet/microsoft/mana/mana_en.c
> +++ b/drivers/net/ethernet/microsoft/mana/mana_en.c
> @@ -2785,8 +2785,10 @@ static int add_adev(struct gdma_dev *gd)
> 
>  	adev = &madev->adev;
>  	ret = mana_adev_idx_alloc();
> -	if (ret < 0)
> -		goto idx_fail;
> +	if (ret < 0) {
> +		kfree(madev);
> +		return ret;
> +	}
>  	adev->id = ret;
> 
>  	adev->name = "rdma";
> @@ -2795,26 +2797,21 @@ static int add_adev(struct gdma_dev *gd)
>  	madev->mdev = gd;
> 
>  	ret = auxiliary_device_init(adev);
> -	if (ret)
> -		goto init_fail;
> +	if (ret) {
> +		mana_adev_idx_free(adev->id);
> +		kfree(madev);
> +		return ret;
> +	}
> 
>  	ret = auxiliary_device_add(adev);
> -	if (ret)
> -		goto add_fail;
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		mana_adev_idx_free(adev->id);
> +		return ret;
> +	}
> 
>  	gd->adev = adev;
>  	return 0;
> -
> -add_fail:
> -	auxiliary_device_uninit(adev);
> -
> -init_fail:
> -	mana_adev_idx_free(adev->id);
> -
> -idx_fail:
> -	kfree(madev);
I think you can just avoid using add_fail and keep/retain rest of init_fail, idx_fail conditions in old way right?
> -
> -	return ret;
>  }
> 
>  int mana_probe(struct gdma_dev *gd, bool resuming)
> --
> 2.25.1
> 

Re: [PATCH] net: mana: Fix possible double free in error handling path

[Przemek Kitszel]

On 6/24/24 05:21, Ma Ke wrote:
> When auxiliary_device_add() returns error and then calls
> auxiliary_device_uninit(), callback function adev_release

I would add () after function name

> calls kfree(madev) to free memory. We shouldn't call kfree(padev)

there is no `padev` in the patch :)
"to free memory" part sounds redundant.

> again in the error handling path.
> 
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>

you need a Fixes: tag

> ---
>   drivers/net/ethernet/microsoft/mana/mana_en.c | 31 +++++++++----------
>   1 file changed, 14 insertions(+), 17 deletions(-)
> 

RE: [PATCH] net: mana: Fix possible double free in error handling path

[Konstantin Taranov]

> > -     kfree(madev);
> I think you can just avoid using add_fail and keep/retain rest of init_fail, idx_fail
> conditions in old way right?

I do agree with Sai. I think the patch can be just:
@@ -2797,7 +2797,8 @@ static int add_adev(struct gdma_dev *gd)
        ret = auxiliary_device_init(adev);
        if (ret)
                goto init_fail;
-
+       /* madev is owned by the auxiliary device */
+       madev = NULL;
        ret = auxiliary_device_add(adev);
        if (ret)
                goto add_fail;


- Konstantin