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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] nvmet: missing Fixes trailer for nvmet_setup_dhgroup() error
  cleanup
- [Low] nvmet: duplicated DH group cleanup on error in
  nvmet_setup_dhgroup()
--

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

> nvmet: clean up DH group state on initialization failure
>
> 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.
>
> Signed-off-by: Sreeraj S Kurup <sreekuttan2156239@gmail.com>

[Severity: Low]
This fixes incomplete error cleanup in nvmet_setup_dhgroup().  Should the
commit message include:

  Fixes: 7a277c37d352 ("nvmet-auth: Diffie-Hellman key exchange support")

> diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
> index ccba84b1344e0..7733d50de708f 100644
> --- a/drivers/nvme/target/auth.c
> +++ b/drivers/nvme/target/auth.c
> @@ -105,6 +105,10 @@ 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);
> +			crypto_free_kpp(ctrl->dh_tfm);
> +			ctrl->dh_tfm = NULL;
> +			ctrl->dh_gid = 0;
> +			ctrl->dh_keysize = 0;
>  			kfree_sensitive(ctrl->dh_key);
>  			ctrl->dh_key = NULL;
>  			return ret;
> @@ -115,6 +119,10 @@ 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);
> +			crypto_free_kpp(ctrl->dh_tfm);
> +			ctrl->dh_tfm = NULL;
> +			ctrl->dh_gid = 0;
> +			ctrl->dh_keysize = 0;
>  			return -ENOMEM;
>  		}
>  		ret = nvme_auth_gen_pubkey(ctrl->dh_tfm, ctrl->dh_key,
> @@ -122,8 +130,12 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
>  		if (ret < 0) {
>  			pr_warn("ctrl %d failed to generate public key\n",
>  				ctrl->cntlid);
> -			kfree(ctrl->dh_key);
> +			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;
>  		}
>  	}

[Severity: Low]
This isn't a bug, but all three error paths in nvmet_setup_dhgroup() repeat
the same teardown of ctrl->dh_tfm, ctrl->dh_gid, ctrl->dh_keysize and
ctrl->dh_key.  Since kfree_sensitive(NULL) is a no-op, could these branches
jump to a single error label at the end of the function instead?

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