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

[PATCH v2] 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>
---
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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index ccba84b1344e..20f8c84e8ced 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -105,9 +105,7 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 		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;
+			goto err;
 		}
 		ctrl->dh_keysize = crypto_kpp_maxsize(ctrl->dh_tfm);
 		kfree_sensitive(ctrl->dh_key);
@@ -115,18 +113,26 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 		if (!ctrl->dh_key) {
 			pr_warn("ctrl %d failed to allocate public key\n",
 				ctrl->cntlid);
-			return -ENOMEM;
+			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);
-			kfree(ctrl->dh_key);
-			ctrl->dh_key = NULL;
+			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 v2] nvmet: clean up DH group state on initialization failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] nvmet: nvmet_setup_dhgroup() returns 0 when crypto_alloc_kpp()
  fails
--

commit a615cbc77d9efc8d40fbde68d3ff3d83eaa208bd
Author: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

nvmet: clean up DH group state on initialization failure

This commit adds a shared error label in nvmet_setup_dhgroup() to free
ctrl->dh_tfm and reset DH group state when private-key generation,
public-key allocation, or public-key generation fails.

> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index ccba84b1344e0..20f8c84e8ced5 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -105,9 +105,7 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
>  		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;
> +			goto err;
>  		}
>  		ctrl->dh_keysize = crypto_kpp_maxsize(ctrl->dh_tfm);
>  		kfree_sensitive(ctrl->dh_key);
> @@ -115,18 +113,26 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
>  		if (!ctrl->dh_key) {
>  			pr_warn("ctrl %d failed to allocate public key\n",
>  				ctrl->cntlid);
> -			return -ENOMEM;
> +			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);
> -			kfree(ctrl->dh_key);
> -			ctrl->dh_key = NULL;
> +			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;
>  }

[Severity: High]
Does nvmet_setup_dhgroup() now return 0 when crypto_alloc_kpp() fails?

drivers/nvme/target/auth.c:nvmet_setup_dhgroup() {
	...
	ctrl->dh_tfm = crypto_alloc_kpp(dhgroup_kpp, 0, 0);
	if (IS_ERR(ctrl->dh_tfm)) {
		...
		ret = PTR_ERR(ctrl->dh_tfm);
		ctrl->dh_tfm = NULL;
		ctrl->dh_gid = 0;
	} else {
	...
}

The IS_ERR(ctrl->dh_tfm) branch previously fell through the end of the
if-else block to "return ret;".  With "return 0;" placed right before
err, a crypto_alloc_kpp() failure falls through and reports success to
nvmet_setup_auth(), silently proceeding with ctrl->dh_tfm set to NULL and
ctrl->dh_gid set to NVME_AUTH_DHGROUP_NULL instead of failing with
NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE, and leaving any prior
ctrl->dh_key and ctrl->dh_keysize in place on re-authentication.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110093047.4290-1-sreekuttan2156239@gmail.com?part=1