[PATCH v4] nvmet: clean up DH group state on initialization failure

[PATCH v4] nvmet: clean up DH group state on initialization failure

[Sreeraj S Kurup]

nvmet_setup_dhgroup() leaves the DH transform and group ID
initialized when private-key generation, public-key allocation,
or public-key generation fails.

A subsequent invocation with the same DH group incorrectly
returns success because the existing transform and group ID
are treated as proof of successful initialization.

Release the transform and reset the group state on initialization
failures so that subsequent attempts can properly retry setup.

Fixes: 7a277c37d352 ("nvmet-auth: Diffie-Hellman key exchange support")
Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
---
v4:
 - Consolidate DH group error handling and clear stale key state
   when switching DH groups.
v3:
 - Clear stale DH key state when crypto_alloc_kpp() fails during
   re-authentication.
 - Fix the error path when crypto_alloc_kpp() fails by returning
   the allocation error instead of incorrectly reporting success.
v2:
 - Consolidate error handling in "nvmet_setup_dhgroup()" using
   a shared cleanup label to avoid duplicated teardown code.
 - Add Fixes trailer.

 drivers/nvme/target/auth.c | 67 +++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index ccba84b1344e..681459b5db3b 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -76,8 +76,11 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 			return 0;
 		}
 		crypto_free_kpp(ctrl->dh_tfm);
+		kfree_sensitive(ctrl->dh_key);
 		ctrl->dh_tfm = NULL;
 		ctrl->dh_gid = 0;
+		ctrl->dh_key = NULL;
+		ctrl->dh_keysize = 0;
 	}
 
 	if (dhgroup_id == NVME_AUTH_DHGROUP_NULL)
@@ -95,38 +98,42 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 			 __func__, ctrl->cntlid, dhgroup_id,
 			 PTR_ERR(ctrl->dh_tfm));
 		ret = PTR_ERR(ctrl->dh_tfm);
-		ctrl->dh_tfm = NULL;
-		ctrl->dh_gid = 0;
-	} else {
-		ctrl->dh_gid = dhgroup_id;
-		pr_debug("%s: ctrl %d setup DH group %d\n",
-			 __func__, ctrl->cntlid, ctrl->dh_gid);
-		ret = nvme_auth_gen_privkey(ctrl->dh_tfm, ctrl->dh_gid);
-		if (ret < 0) {
-			pr_debug("%s: ctrl %d failed to generate private key, err %d\n",
-				 __func__, ctrl->cntlid, ret);
-			kfree_sensitive(ctrl->dh_key);
-			ctrl->dh_key = NULL;
-			return ret;
-		}
-		ctrl->dh_keysize = crypto_kpp_maxsize(ctrl->dh_tfm);
-		kfree_sensitive(ctrl->dh_key);
-		ctrl->dh_key = kzalloc(ctrl->dh_keysize, GFP_KERNEL);
-		if (!ctrl->dh_key) {
-			pr_warn("ctrl %d failed to allocate public key\n",
-				ctrl->cntlid);
-			return -ENOMEM;
-		}
-		ret = nvme_auth_gen_pubkey(ctrl->dh_tfm, ctrl->dh_key,
-					   ctrl->dh_keysize);
-		if (ret < 0) {
-			pr_warn("ctrl %d failed to generate public key\n",
-				ctrl->cntlid);
-			kfree(ctrl->dh_key);
-			ctrl->dh_key = NULL;
-		}
+		goto err;
+	}
+
+	ctrl->dh_gid = dhgroup_id;
+	pr_debug("%s: ctrl %d setup DH group %d\n",
+		 __func__, ctrl->cntlid, ctrl->dh_gid);
+	ret = nvme_auth_gen_privkey(ctrl->dh_tfm, ctrl->dh_gid);
+	if (ret < 0) {
+		pr_debug("%s: ctrl %d failed to generate private key, err %d\n",
+			 __func__, ctrl->cntlid, ret);
+		goto err;
+	}
+	ctrl->dh_keysize = crypto_kpp_maxsize(ctrl->dh_tfm);
+	ctrl->dh_key = kzalloc(ctrl->dh_keysize, GFP_KERNEL);
+	if (!ctrl->dh_key) {
+		pr_warn("ctrl %d failed to allocate public key\n",
+			ctrl->cntlid);
+		ret = -ENOMEM;
+		goto err;
+	}
+	ret = nvme_auth_gen_pubkey(ctrl->dh_tfm, ctrl->dh_key,
+				   ctrl->dh_keysize);
+	if (ret < 0) {
+		pr_warn("ctrl %d failed to generate public key\n",
+			ctrl->cntlid);
+		goto err;
 	}
 
+	return 0;
+err:
+	kfree_sensitive(ctrl->dh_key);
+	ctrl->dh_key = NULL;
+	crypto_free_kpp(ctrl->dh_tfm);
+	ctrl->dh_tfm = NULL;
+	ctrl->dh_gid = 0;
+	ctrl->dh_keysize = 0;
 	return ret;
 }
 
-- 
2.55.0

Re: [PATCH v4] nvmet: clean up DH group state on initialization failure

[Hannes Reinecke]

On 11/10/26 12:35 PM, Sreeraj S Kurup wrote:
> nvmet_setup_dhgroup() leaves the DH transform and group ID
> initialized when private-key generation, public-key allocation,
> or public-key generation fails.
> 
> A subsequent invocation with the same DH group incorrectly
> returns success because the existing transform and group ID
> are treated as proof of successful initialization.
> 
> Release the transform and reset the group state on initialization
> failures so that subsequent attempts can properly retry setup.
> 
> Fixes: 7a277c37d352 ("nvmet-auth: Diffie-Hellman key exchange support")
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>
> ---
> v4:
>   - Consolidate DH group error handling and clear stale key state
>     when switching DH groups.
> v3:
>   - Clear stale DH key state when crypto_alloc_kpp() fails during
>     re-authentication.
>   - Fix the error path when crypto_alloc_kpp() fails by returning
>     the allocation error instead of incorrectly reporting success.
> v2:
>   - Consolidate error handling in "nvmet_setup_dhgroup()" using
>     a shared cleanup label to avoid duplicated teardown code.
>   - Add Fixes trailer.
> 
>   drivers/nvme/target/auth.c | 67 +++++++++++++++++++++-----------------
>   1 file changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index ccba84b1344e..681459b5db3b 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -76,8 +76,11 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
>   			return 0;
>   		}
>   		crypto_free_kpp(ctrl->dh_tfm);
> +		kfree_sensitive(ctrl->dh_key);
>   		ctrl->dh_tfm = NULL;
>   		ctrl->dh_gid = 0;
> +		ctrl->dh_key = NULL;
> +		ctrl->dh_keysize = 0;
>   	}
>   
>   	if (dhgroup_id == NVME_AUTH_DHGROUP_NULL)
> @@ -95,38 +98,42 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
>   			 __func__, ctrl->cntlid, dhgroup_id,
>   			 PTR_ERR(ctrl->dh_tfm));
>   		ret = PTR_ERR(ctrl->dh_tfm);
> -		ctrl->dh_tfm = NULL;
> -		ctrl->dh_gid = 0;
> -	} else {
> -		ctrl->dh_gid = dhgroup_id;
> -		pr_debug("%s: ctrl %d setup DH group %d\n",
> -			 __func__, ctrl->cntlid, ctrl->dh_gid);
> -		ret = nvme_auth_gen_privkey(ctrl->dh_tfm, ctrl->dh_gid);
> -		if (ret < 0) {
> -			pr_debug("%s: ctrl %d failed to generate private key, err %d\n",
> -				 __func__, ctrl->cntlid, ret);
> -			kfree_sensitive(ctrl->dh_key);
> -			ctrl->dh_key = NULL;
> -			return ret;
> -		}
> -		ctrl->dh_keysize = crypto_kpp_maxsize(ctrl->dh_tfm);
> -		kfree_sensitive(ctrl->dh_key);
> -		ctrl->dh_key = kzalloc(ctrl->dh_keysize, GFP_KERNEL);
> -		if (!ctrl->dh_key) {
> -			pr_warn("ctrl %d failed to allocate public key\n",
> -				ctrl->cntlid);
> -			return -ENOMEM;
> -		}
> -		ret = nvme_auth_gen_pubkey(ctrl->dh_tfm, ctrl->dh_key,
> -					   ctrl->dh_keysize);
> -		if (ret < 0) {
> -			pr_warn("ctrl %d failed to generate public key\n",
> -				ctrl->cntlid);
> -			kfree(ctrl->dh_key);
> -			ctrl->dh_key = NULL;
> -		}
> +		goto err;
> +	}
> +
> +	ctrl->dh_gid = dhgroup_id;
> +	pr_debug("%s: ctrl %d setup DH group %d\n",
> +		 __func__, ctrl->cntlid, ctrl->dh_gid);
> +	ret = nvme_auth_gen_privkey(ctrl->dh_tfm, ctrl->dh_gid);
> +	if (ret < 0) {
> +		pr_debug("%s: ctrl %d failed to generate private key, err %d\n",
> +			 __func__, ctrl->cntlid, ret);
> +		goto err;
> +	}
> +	ctrl->dh_keysize = crypto_kpp_maxsize(ctrl->dh_tfm);
> +	ctrl->dh_key = kzalloc(ctrl->dh_keysize, GFP_KERNEL);
> +	if (!ctrl->dh_key) {
> +		pr_warn("ctrl %d failed to allocate public key\n",
> +			ctrl->cntlid);
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	ret = nvme_auth_gen_pubkey(ctrl->dh_tfm, ctrl->dh_key,
> +				   ctrl->dh_keysize);
> +	if (ret < 0) {
> +		pr_warn("ctrl %d failed to generate public key\n",
> +			ctrl->cntlid);
> +		goto err;
>   	}
>   
> +	return 0;
> +err:
> +	kfree_sensitive(ctrl->dh_key);
> +	ctrl->dh_key = NULL;
> +	crypto_free_kpp(ctrl->dh_tfm);
> +	ctrl->dh_tfm = NULL;
> +	ctrl->dh_gid = 0;
> +	ctrl->dh_keysize = 0;
>   	return ret;
>   }
>   

Still wonder if a helper wouldn't be better here.
But anyway.

Reviewed-by: Hannes Reinecke <hare@kernel.org>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v4] nvmet: clean up DH group state on initialization failure

[Sreeraj S Kurup]

Thanks, Hannes.

I kept the cleanup in a common "err:" path to avoid duplicating the teardown 
logic across multiple failure paths.

The diff looks larger mainly because I removed the existing "else" block after 
introducing "goto err" for error handling. This allows the successful initialization 
path to remain linear, with all failures handled through the common cleanup section.

Thanks for the review!

Sreeraj