tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 9d2f6060fe4c3b49d0cdc1dce1c99296f33379c8
commit: aa36d711e945e65fa87410927800f01878a8faed nvme-auth: convert dhchap_auth_list to an array
config: i386-randconfig-m021-20221219
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/nvme/host/auth.c:950 nvme_auth_init_ctrl() warn: missing error code? 'ret'
vim +/ret +950 drivers/nvme/host/auth.c
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 931 int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
f50fff73d620cd Hannes Reinecke 2022-06-27 932 {
aa36d711e945e6 Sagi Grimberg 2022-11-13 933 struct nvme_dhchap_queue_context *chap;
aa36d711e945e6 Sagi Grimberg 2022-11-13 934 int i, ret;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 935
f50fff73d620cd Hannes Reinecke 2022-06-27 936 mutex_init(&ctrl->dhchap_auth_mutex);
aa36d711e945e6 Sagi Grimberg 2022-11-13 937 INIT_WORK(&ctrl->dhchap_auth_work, nvme_ctrl_auth_work);
f50fff73d620cd Hannes Reinecke 2022-06-27 938 if (!ctrl->opts)
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 939 return 0;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 940 ret = nvme_auth_generate_key(ctrl->opts->dhchap_secret,
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 941 &ctrl->host_key);
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 942 if (ret)
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 943 return ret;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 944 ret = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret,
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 945 &ctrl->ctrl_key);
aa36d711e945e6 Sagi Grimberg 2022-11-13 946 if (ret)
aa36d711e945e6 Sagi Grimberg 2022-11-13 947 goto err_free_dhchap_secret;
aa36d711e945e6 Sagi Grimberg 2022-11-13 948
aa36d711e945e6 Sagi Grimberg 2022-11-13 949 if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret)
aa36d711e945e6 Sagi Grimberg 2022-11-13 @950 return ret;
Please return a literal here. Either return -EINVAL or return 0;
aa36d711e945e6 Sagi Grimberg 2022-11-13 951
aa36d711e945e6 Sagi Grimberg 2022-11-13 952 ctrl->dhchap_ctxs = kvcalloc(ctrl_max_dhchaps(ctrl),
aa36d711e945e6 Sagi Grimberg 2022-11-13 953 sizeof(*chap), GFP_KERNEL);
aa36d711e945e6 Sagi Grimberg 2022-11-13 954 if (!ctrl->dhchap_ctxs) {
aa36d711e945e6 Sagi Grimberg 2022-11-13 955 ret = -ENOMEM;
aa36d711e945e6 Sagi Grimberg 2022-11-13 956 goto err_free_dhchap_ctrl_secret;
aa36d711e945e6 Sagi Grimberg 2022-11-13 957 }
aa36d711e945e6 Sagi Grimberg 2022-11-13 958
aa36d711e945e6 Sagi Grimberg 2022-11-13 959 for (i = 0; i < ctrl_max_dhchaps(ctrl); i++) {
aa36d711e945e6 Sagi Grimberg 2022-11-13 960 chap = &ctrl->dhchap_ctxs[i];
aa36d711e945e6 Sagi Grimberg 2022-11-13 961 chap->qid = i;
aa36d711e945e6 Sagi Grimberg 2022-11-13 962 chap->ctrl = ctrl;
aa36d711e945e6 Sagi Grimberg 2022-11-13 963 INIT_WORK(&chap->auth_work, nvme_queue_auth_work);
aa36d711e945e6 Sagi Grimberg 2022-11-13 964 }
aa36d711e945e6 Sagi Grimberg 2022-11-13 965
aa36d711e945e6 Sagi Grimberg 2022-11-13 966 return 0;
aa36d711e945e6 Sagi Grimberg 2022-11-13 967 err_free_dhchap_ctrl_secret:
aa36d711e945e6 Sagi Grimberg 2022-11-13 968 nvme_auth_free_key(ctrl->ctrl_key);
aa36d711e945e6 Sagi Grimberg 2022-11-13 969 ctrl->ctrl_key = NULL;
aa36d711e945e6 Sagi Grimberg 2022-11-13 970 err_free_dhchap_secret:
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 971 nvme_auth_free_key(ctrl->host_key);
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 972 ctrl->host_key = NULL;
193a8c7e5f1a84 Sagi Grimberg 2022-11-13 973 return ret;
f50fff73d620cd Hannes Reinecke 2022-06-27 974 }
--
0-DAY CI Kernel Test Service
https://01.org/lkp
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)) {
+ ret = PTR_ERR(ctrl->host_key);
+ goto out;
+ }
+ ctrl->ctrl_key = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret);
+ if (IS_ERR(ctrl->ctrl_key)) {
+ ret = PTR_ERR(ctrl->ctrl_key);
goto err_free_dhchap_secret;
+ }
- if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret)
+ if (!ctrl->host_key && !ctrl->ctrl_key)
return ret;
ctrl->dhchap_ctxs = kvcalloc(ctrl_max_dhchaps(ctrl),
@@ -972,9 +975,10 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
return 0;
err_free_dhchap_ctrl_secret:
nvme_auth_free_key(ctrl->ctrl_key);
- ctrl->ctrl_key = NULL;
err_free_dhchap_secret:
+ ctrl->ctrl_key = NULL;
nvme_auth_free_key(ctrl->host_key);
+out:
ctrl->host_key = NULL;
return ret;
}
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f72fc3bd07c348..6245f1c87c5ba8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3751,11 +3751,10 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
nvme_auth_stop(ctrl);
if (strcmp(dhchap_secret, opts->dhchap_secret)) {
struct nvme_dhchap_key *key, *host_key;
- int ret;
- ret = nvme_auth_generate_key(dhchap_secret, &key);
- if (ret)
- return ret;
+ key = nvme_auth_generate_key(dhchap_secret);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
kfree(opts->dhchap_secret);
opts->dhchap_secret = dhchap_secret;
host_key = ctrl->host_key;
@@ -3805,11 +3804,10 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
nvme_auth_stop(ctrl);
if (strcmp(dhchap_secret, opts->dhchap_ctrl_secret)) {
struct nvme_dhchap_key *key, *ctrl_key;
- int ret;
- ret = nvme_auth_generate_key(dhchap_secret, &key);
- if (ret)
- return ret;
+ key = nvme_auth_generate_key(dhchap_secret);
+ if (IS_ERR(key))
+ return PTR_ERR(key);
kfree(opts->dhchap_ctrl_secret);
opts->dhchap_ctrl_secret = dhchap_secret;
ctrl_key = ctrl->ctrl_key;
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index dcb8030062ddaf..df82d7950ee204 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -28,7 +28,7 @@ struct nvme_dhchap_key *nvme_auth_extract_key(unsigned char *secret,
u8 key_hash);
void nvme_auth_free_key(struct nvme_dhchap_key *key);
u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn);
-int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key);
+struct nvme_dhchap_key *nvme_auth_generate_key(u8 *secret);
int nvme_auth_augmented_challenge(u8 hmac_id, u8 *skey, size_t skey_len,
u8 *challenge, u8 *aug, size_t hlen);
int nvme_auth_gen_privkey(struct crypto_kpp *dh_tfm, u8 dh_gid);
On Fri, Dec 23, 2022 at 04:47:54PM +0100, Christoph Hellwig wrote:
> 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)) {
> + ret = PTR_ERR(ctrl->host_key);
> + goto out;
> + }
> + ctrl->ctrl_key = nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret);
> + if (IS_ERR(ctrl->ctrl_key)) {
> + ret = PTR_ERR(ctrl->ctrl_key);
> goto err_free_dhchap_secret;
> + }
>
> - if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret)
> + if (!ctrl->host_key && !ctrl->ctrl_key)
> return ret;
ret is uninitialized now.
regards,
dan carpenter
On Fri, Dec 23, 2022 at 07:21:49PM +0300, Dan Carpenter wrote:
> > - if (!ctrl->opts->dhchap_secret && !ctrl->opts->dhchap_ctrl_secret)
> > + if (!ctrl->host_key && !ctrl->ctrl_key)
> > return ret;
>
> ret is uninitialized now.
Yes. Should be a 'return 0'.
We really need to turn the maybe uninitialized warnings back on.
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.
On Sun, Dec 25, 2022 at 12:36:07PM +0200, Sagi Grimberg wrote:
> Although I think that for this specific report, we should do a simple
> fix and then make the code look better.
It turned out the code worked fine as-is. It just wasn't clear.
regards,
dan carpenter