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

[Hannes Reinecke <hare@suse.de>]

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