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

[PATCH v3] 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>
---
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 | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index ccba84b1344e..33f2584973ec 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -97,6 +97,10 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 		ret = PTR_ERR(ctrl->dh_tfm);
 		ctrl->dh_tfm = NULL;
 		ctrl->dh_gid = 0;
+		kfree_sensitive(ctrl->dh_key);
+		ctrl->dh_key = NULL;
+		ctrl->dh_keysize = 0;
+		return ret;
 	} else {
 		ctrl->dh_gid = dhgroup_id;
 		pr_debug("%s: ctrl %d setup DH group %d\n",
@@ -105,9 +109,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 +117,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 v3] 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:
- [Low] nvmet: duplicated error cleanup and stale key state in
  nvmet_setup_dhgroup()
--

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

nvmet: clean up DH group state on initialization failure

This commit cleans up the Diffie-Hellman transform, key, and group state
when DH group initialization fails in nvmet_setup_dhgroup().

> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index ccba84b1344e0..33f2584973ec1 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -97,6 +97,10 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
>  		ret = PTR_ERR(ctrl->dh_tfm);
>  		ctrl->dh_tfm = NULL;
>  		ctrl->dh_gid = 0;
> +		kfree_sensitive(ctrl->dh_key);
> +		ctrl->dh_key = NULL;
> +		ctrl->dh_keysize = 0;
> +		return ret;
>  	} else {
>  		ctrl->dh_gid = dhgroup_id;
>  		pr_debug("%s: ctrl %d setup DH group %d\n",
> @@ -105,9 +109,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 +117,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: Low]
Can the IS_ERR(ctrl->dh_tfm) branch set ret = PTR_ERR(ctrl->dh_tfm) and
jump to err instead of duplicating the teardown sequence and keeping the
else block after a return?  crypto_free_kpp() calls crypto_destroy_tfm(),
which handles error pointers safely.

Also, when ctrl->dh_tfm is already set and nvmet_setup_dhgroup() switches
to a different dhgroup_id earlier in the function, ctrl->dh_tfm is freed
without clearing ctrl->dh_key or ctrl->dh_keysize:

drivers/nvme/target/auth.c:nvmet_setup_dhgroup() {
    ...
	if (ctrl->dh_tfm) {
		if (ctrl->dh_gid == dhgroup_id) {
			pr_debug("%s: ctrl %d reuse existing DH group %d\n",
				 __func__, ctrl->cntlid, dhgroup_id);
			return 0;
		}
		crypto_free_kpp(ctrl->dh_tfm);
		ctrl->dh_tfm = NULL;
		ctrl->dh_gid = 0;
	}

	if (dhgroup_id == NVME_AUTH_DHGROUP_NULL)
		return 0;
    ...
}

If the new dhgroup_id is NVME_AUTH_DHGROUP_NULL (or an invalid group ID),
the early return leaves the previous group's ctrl->dh_key allocated and
ctrl->dh_keysize non-zero.  Freeing ctrl->dh_key and zeroing
ctrl->dh_keysize alongside crypto_free_kpp(ctrl->dh_tfm) there would keep
all DH state cleared on group changes and make the extra
kfree_sensitive(ctrl->dh_key) before kzalloc() unnecessary.

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