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

[sashiko-bot@kernel.org]

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