linux-kernel - Re: drivers/nvme/host/auth.c:950 nvme_auth_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 25 Dec 2022 12:36:07 +0200
From:   Sagi Grimberg <sagi@...mberg.me>
To:     Christoph Hellwig <hch@....de>, Dan Carpenter <error27@...il.com>
Cc:     oe-kbuild@...ts.linux.dev, lkp@...el.com,
        oe-kbuild-all@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing
 error code? 'ret'



On 12/23/22 17:47, Christoph Hellwig wrote:
> Based on the code in nvme_auth_generate_key I assume this is intentional,
> but the code looks really confusing.
> 
> Hannes, Sagi, what do you think of something like this:
> 
> 
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index d90e4f0c08b7b9..a07eb4cd9ce173 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -455,28 +455,18 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
>   }
>   EXPORT_SYMBOL_GPL(nvme_auth_gen_shared_secret);
>   
> -int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key)
> +struct nvme_dhchap_key *nvme_auth_generate_key(u8 *secret)
>   {
> -	struct nvme_dhchap_key *key;
>   	u8 key_hash;
>   
> -	if (!secret) {
> -		*ret_key = NULL;
> -		return 0;
> -	}
> +	if (!secret)
> +		return NULL;
>   
>   	if (sscanf(secret, "DHHC-1:%hhd:%*s:", &key_hash) != 1)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>   
>   	/* Pass in the secret without the 'DHHC-1:XX:' prefix */
> -	key = nvme_auth_extract_key(secret + 10, key_hash);
> -	if (IS_ERR(key)) {
> -		*ret_key = NULL;
> -		return PTR_ERR(key);
> -	}
> -
> -	*ret_key = key;
> -	return 0;
> +	return nvme_auth_extract_key(secret + 10, key_hash);
>   }
>   EXPORT_SYMBOL_GPL(nvme_auth_generate_key);
>   
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index bb0abbe4491cdc..c808652966a94f 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -943,16 +943,19 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
>   	INIT_WORK(&ctrl->dhchap_auth_work, nvme_ctrl_auth_work);
>   	if (!ctrl->opts)
>   		return 0;
> -	ret = nvme_auth_generate_key(ctrl->opts->dhchap_secret,
> -			&ctrl->host_key);
> -	if (ret)
> -		return ret;
> -	ret = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret,
> -			&ctrl->ctrl_key);
> -	if (ret)
> +
> +	ctrl->host_key = nvme_auth_generate_key(ctrl->opts->dhchap_secret);
> +	if (IS_ERR(ctrl->host_key)) {

nvme_auth_generate_key can return NULL, so in this case we should avoid
calling it if the secret is null here.

Other than that, this looks good.
Although I think that for this specific report, we should do a simple
fix and then make the code look better.