Re: [PATCH v2] 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:
- [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