* [PATCH 1/6] nvme-auth: rename __nvme_auth_[reset|free] to nvme_auth[reset|free]_dhchap
2022-10-25 13:43 [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Sagi Grimberg
@ 2022-10-25 13:43 ` Sagi Grimberg
2022-10-25 17:18 ` Hannes Reinecke
2022-10-25 17:34 ` Chaitanya Kulkarni
2022-10-25 13:43 ` [PATCH 2/6] nvme-auth: remove symbol export from nvme_auth_reset Sagi Grimberg
` (5 subsequent siblings)
6 siblings, 2 replies; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 13:43 UTC (permalink / raw)
To: linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
nvme_auth_[reset|free] operate on the controller while
__nvme_auth_[reset|free] operate on a chap struct (which maps to a queue
context). Rename it for clarity.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/auth.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index c8a6db7c4498..d45333268fcf 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -654,7 +654,7 @@ static int nvme_auth_dhchap_exponential(struct nvme_ctrl *ctrl,
return 0;
}
-static void __nvme_auth_reset(struct nvme_dhchap_queue_context *chap)
+static void nvme_auth_reset_dhchap(struct nvme_dhchap_queue_context *chap)
{
kfree_sensitive(chap->host_response);
chap->host_response = NULL;
@@ -676,9 +676,9 @@ static void __nvme_auth_reset(struct nvme_dhchap_queue_context *chap)
memset(chap->c2, 0, sizeof(chap->c2));
}
-static void __nvme_auth_free(struct nvme_dhchap_queue_context *chap)
+static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap)
{
- __nvme_auth_reset(chap);
+ nvme_auth_reset_dhchap(chap);
if (chap->shash_tfm)
crypto_free_shash(chap->shash_tfm);
if (chap->dh_tfm)
@@ -868,7 +868,7 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
dev_dbg(ctrl->device, "qid %d: re-using context\n", qid);
mutex_unlock(&ctrl->dhchap_auth_mutex);
flush_work(&chap->auth_work);
- __nvme_auth_reset(chap);
+ nvme_auth_reset_dhchap(chap);
queue_work(nvme_wq, &chap->auth_work);
return 0;
}
@@ -928,7 +928,7 @@ void nvme_auth_reset(struct nvme_ctrl *ctrl)
list_for_each_entry(chap, &ctrl->dhchap_auth_list, entry) {
mutex_unlock(&ctrl->dhchap_auth_mutex);
flush_work(&chap->auth_work);
- __nvme_auth_reset(chap);
+ nvme_auth_reset_dhchap(chap);
}
mutex_unlock(&ctrl->dhchap_auth_mutex);
}
@@ -1002,7 +1002,7 @@ void nvme_auth_free(struct nvme_ctrl *ctrl)
list_for_each_entry_safe(chap, tmp, &ctrl->dhchap_auth_list, entry) {
list_del_init(&chap->entry);
flush_work(&chap->auth_work);
- __nvme_auth_free(chap);
+ nvme_auth_free_dhchap(chap);
}
mutex_unlock(&ctrl->dhchap_auth_mutex);
if (ctrl->host_key) {
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/6] nvme-auth: rename __nvme_auth_[reset|free] to nvme_auth[reset|free]_dhchap
2022-10-25 13:43 ` [PATCH 1/6] nvme-auth: rename __nvme_auth_[reset|free] to nvme_auth[reset|free]_dhchap Sagi Grimberg
@ 2022-10-25 17:18 ` Hannes Reinecke
2022-10-25 17:34 ` Chaitanya Kulkarni
1 sibling, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2022-10-25 17:18 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
On 10/25/22 15:43, Sagi Grimberg wrote:
> nvme_auth_[reset|free] operate on the controller while
> __nvme_auth_[reset|free] operate on a chap struct (which maps to a queue
> context). Rename it for clarity.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/host/auth.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/6] nvme-auth: rename __nvme_auth_[reset|free] to nvme_auth[reset|free]_dhchap
2022-10-25 13:43 ` [PATCH 1/6] nvme-auth: rename __nvme_auth_[reset|free] to nvme_auth[reset|free]_dhchap Sagi Grimberg
2022-10-25 17:18 ` Hannes Reinecke
@ 2022-10-25 17:34 ` Chaitanya Kulkarni
1 sibling, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2022-10-25 17:34 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
On 10/25/2022 6:43 AM, Sagi Grimberg wrote:
> nvme_auth_[reset|free] operate on the controller while
> __nvme_auth_[reset|free] operate on a chap struct (which maps to a queue
> context). Rename it for clarity.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/6] nvme-auth: remove symbol export from nvme_auth_reset
2022-10-25 13:43 [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Sagi Grimberg
2022-10-25 13:43 ` [PATCH 1/6] nvme-auth: rename __nvme_auth_[reset|free] to nvme_auth[reset|free]_dhchap Sagi Grimberg
@ 2022-10-25 13:43 ` Sagi Grimberg
2022-10-25 17:18 ` Hannes Reinecke
2022-10-25 17:34 ` Chaitanya Kulkarni
2022-10-25 13:43 ` [PATCH 3/6] nvme-auth: don't re-authenticate if the controller is not LIVE Sagi Grimberg
` (4 subsequent siblings)
6 siblings, 2 replies; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 13:43 UTC (permalink / raw)
To: linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
Only the nvme module calls it.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/auth.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index d45333268fcf..734928282d3e 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -932,7 +932,6 @@ void nvme_auth_reset(struct nvme_ctrl *ctrl)
}
mutex_unlock(&ctrl->dhchap_auth_mutex);
}
-EXPORT_SYMBOL_GPL(nvme_auth_reset);
static void nvme_dhchap_auth_work(struct work_struct *work)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/6] nvme-auth: remove symbol export from nvme_auth_reset
2022-10-25 13:43 ` [PATCH 2/6] nvme-auth: remove symbol export from nvme_auth_reset Sagi Grimberg
@ 2022-10-25 17:18 ` Hannes Reinecke
2022-10-25 17:34 ` Chaitanya Kulkarni
1 sibling, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2022-10-25 17:18 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
On 10/25/22 15:43, Sagi Grimberg wrote:
> Only the nvme module calls it.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/host/auth.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index d45333268fcf..734928282d3e 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -932,7 +932,6 @@ void nvme_auth_reset(struct nvme_ctrl *ctrl)
> }
> mutex_unlock(&ctrl->dhchap_auth_mutex);
> }
> -EXPORT_SYMBOL_GPL(nvme_auth_reset);
>
> static void nvme_dhchap_auth_work(struct work_struct *work)
> {
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/6] nvme-auth: remove symbol export from nvme_auth_reset
2022-10-25 13:43 ` [PATCH 2/6] nvme-auth: remove symbol export from nvme_auth_reset Sagi Grimberg
2022-10-25 17:18 ` Hannes Reinecke
@ 2022-10-25 17:34 ` Chaitanya Kulkarni
1 sibling, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2022-10-25 17:34 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
On 10/25/2022 6:43 AM, Sagi Grimberg wrote:
> Only the nvme module calls it.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/6] nvme-auth: don't re-authenticate if the controller is not LIVE
2022-10-25 13:43 [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Sagi Grimberg
2022-10-25 13:43 ` [PATCH 1/6] nvme-auth: rename __nvme_auth_[reset|free] to nvme_auth[reset|free]_dhchap Sagi Grimberg
2022-10-25 13:43 ` [PATCH 2/6] nvme-auth: remove symbol export from nvme_auth_reset Sagi Grimberg
@ 2022-10-25 13:43 ` Sagi Grimberg
2022-10-25 17:22 ` Hannes Reinecke
2022-10-25 13:43 ` [PATCH 4/6] nvme-auth: remove redundant buffer deallocations Sagi Grimberg
` (3 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 13:43 UTC (permalink / raw)
To: linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
The connect sequence will re-authenticate.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/auth.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 734928282d3e..93c0fc71bc7c 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -939,6 +939,13 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
container_of(work, struct nvme_ctrl, dhchap_auth_work);
int ret, q;
+ /*
+ * If the ctrl is no connected, bail as reconnect will handle
+ * authentication.
+ */
+ if (ctrl->state != NVME_CTRL_LIVE)
+ return;
+
/* Authenticate admin queue first */
ret = nvme_auth_negotiate(ctrl, 0);
if (ret) {
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/6] nvme-auth: don't re-authenticate if the controller is not LIVE
2022-10-25 13:43 ` [PATCH 3/6] nvme-auth: don't re-authenticate if the controller is not LIVE Sagi Grimberg
@ 2022-10-25 17:22 ` Hannes Reinecke
2022-10-25 20:22 ` Sagi Grimberg
0 siblings, 1 reply; 19+ messages in thread
From: Hannes Reinecke @ 2022-10-25 17:22 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
On 10/25/22 15:43, Sagi Grimberg wrote:
> The connect sequence will re-authenticate.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/host/auth.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 734928282d3e..93c0fc71bc7c 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -939,6 +939,13 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
> container_of(work, struct nvme_ctrl, dhchap_auth_work);
> int ret, q;
>
> + /*
> + * If the ctrl is no connected, bail as reconnect will handle
> + * authentication.
> + */
> + if (ctrl->state != NVME_CTRL_LIVE)
> + return;
> +
> /* Authenticate admin queue first */
> ret = nvme_auth_negotiate(ctrl, 0);
> if (ret) {
Hmm. That is only part of the story; the controller can be reset at any
time during the workqueue function.
Doing so will invalidate the digest hash, but we only find out about
queue being invalid once we _send_ data. So we crash in
skb_copy_datagram_iter().
I got a patch here somewhere (which is still under testing, hence I
didn't send it off), but can do so now if requested.
Cheers,
Hannes
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/6] nvme-auth: don't re-authenticate if the controller is not LIVE
2022-10-25 17:22 ` Hannes Reinecke
@ 2022-10-25 20:22 ` Sagi Grimberg
0 siblings, 0 replies; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 20:22 UTC (permalink / raw)
To: Hannes Reinecke, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
On 10/25/22 20:22, Hannes Reinecke wrote:
> On 10/25/22 15:43, Sagi Grimberg wrote:
>> The connect sequence will re-authenticate.
>>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>> drivers/nvme/host/auth.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>> index 734928282d3e..93c0fc71bc7c 100644
>> --- a/drivers/nvme/host/auth.c
>> +++ b/drivers/nvme/host/auth.c
>> @@ -939,6 +939,13 @@ static void nvme_dhchap_auth_work(struct
>> work_struct *work)
>> container_of(work, struct nvme_ctrl, dhchap_auth_work);
>> int ret, q;
>> + /*
>> + * If the ctrl is no connected, bail as reconnect will handle
>> + * authentication.
>> + */
>> + if (ctrl->state != NVME_CTRL_LIVE)
>> + return;
>> +
>> /* Authenticate admin queue first */
>> ret = nvme_auth_negotiate(ctrl, 0);
>> if (ret) {
>
> Hmm. That is only part of the story; the controller can be reset at any
> time during the workqueue function.
Won't controller reset flush/cancel all the pending auth works?
> Doing so will invalidate the digest hash, but we only find out about
> queue being invalid once we _send_ data. So we crash in
> skb_copy_datagram_iter().
> I got a patch here somewhere (which is still under testing, hence I
> didn't send it off), but can do so now if requested.
It would be good if you can share. I think we need some more work on
the whole re-authentication piece.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/6] nvme-auth: remove redundant buffer deallocations
2022-10-25 13:43 [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Sagi Grimberg
` (2 preceding siblings ...)
2022-10-25 13:43 ` [PATCH 3/6] nvme-auth: don't re-authenticate if the controller is not LIVE Sagi Grimberg
@ 2022-10-25 13:43 ` Sagi Grimberg
2022-10-25 17:23 ` Hannes Reinecke
2022-10-25 17:36 ` Chaitanya Kulkarni
2022-10-25 13:43 ` [PATCH 5/6] nvme-auth: don't ignore key generation failures when initializing ctrl keys Sagi Grimberg
` (2 subsequent siblings)
6 siblings, 2 replies; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 13:43 UTC (permalink / raw)
To: linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
host_response, host_key, ctrl_key and sess_key are
freed in nvme_auth_reset_dhchap which is called from
nvme_auth_free_dhchap.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/auth.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 93c0fc71bc7c..15cddc2bb14d 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -683,10 +683,6 @@ static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap)
crypto_free_shash(chap->shash_tfm);
if (chap->dh_tfm)
crypto_free_kpp(chap->dh_tfm);
- kfree_sensitive(chap->ctrl_key);
- kfree_sensitive(chap->host_key);
- kfree_sensitive(chap->sess_key);
- kfree_sensitive(chap->host_response);
kfree(chap->buf);
kfree(chap);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/6] nvme-auth: remove redundant buffer deallocations
2022-10-25 13:43 ` [PATCH 4/6] nvme-auth: remove redundant buffer deallocations Sagi Grimberg
@ 2022-10-25 17:23 ` Hannes Reinecke
2022-10-25 17:36 ` Chaitanya Kulkarni
1 sibling, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2022-10-25 17:23 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
On 10/25/22 15:43, Sagi Grimberg wrote:
> host_response, host_key, ctrl_key and sess_key are
> freed in nvme_auth_reset_dhchap which is called from
> nvme_auth_free_dhchap.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/host/auth.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 93c0fc71bc7c..15cddc2bb14d 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -683,10 +683,6 @@ static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap)
> crypto_free_shash(chap->shash_tfm);
> if (chap->dh_tfm)
> crypto_free_kpp(chap->dh_tfm);
> - kfree_sensitive(chap->ctrl_key);
> - kfree_sensitive(chap->host_key);
> - kfree_sensitive(chap->sess_key);
> - kfree_sensitive(chap->host_response);
> kfree(chap->buf);
> kfree(chap);
> }
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/6] nvme-auth: remove redundant buffer deallocations
2022-10-25 13:43 ` [PATCH 4/6] nvme-auth: remove redundant buffer deallocations Sagi Grimberg
2022-10-25 17:23 ` Hannes Reinecke
@ 2022-10-25 17:36 ` Chaitanya Kulkarni
1 sibling, 0 replies; 19+ messages in thread
From: Chaitanya Kulkarni @ 2022-10-25 17:36 UTC (permalink / raw)
To: linux-nvme
On 10/25/2022 6:43 AM, Sagi Grimberg wrote:
> host_response, host_key, ctrl_key and sess_key are
> freed in nvme_auth_reset_dhchap which is called from
> nvme_auth_free_dhchap.
>
Perhaps :-
host_response, host_key, ctrl_key and sess_key are freed in
nvme_auth_reset_dhchap which is called from nvme_auth_free_dhchap.
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/host/auth.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 93c0fc71bc7c..15cddc2bb14d 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -683,10 +683,6 @@ static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap)
> crypto_free_shash(chap->shash_tfm);
> if (chap->dh_tfm)
> crypto_free_kpp(chap->dh_tfm);
> - kfree_sensitive(chap->ctrl_key);
> - kfree_sensitive(chap->host_key);
> - kfree_sensitive(chap->sess_key);
> - kfree_sensitive(chap->host_response);
> kfree(chap->buf);
> kfree(chap);
> }
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/6] nvme-auth: don't ignore key generation failures when initializing ctrl keys
2022-10-25 13:43 [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Sagi Grimberg
` (3 preceding siblings ...)
2022-10-25 13:43 ` [PATCH 4/6] nvme-auth: remove redundant buffer deallocations Sagi Grimberg
@ 2022-10-25 13:43 ` Sagi Grimberg
2022-10-25 17:24 ` Hannes Reinecke
2022-10-25 13:43 ` [PATCH 6/6] nvme-auth: don't override ctrl keys before validation Sagi Grimberg
2022-10-25 15:32 ` [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Christoph Hellwig
6 siblings, 1 reply; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 13:43 UTC (permalink / raw)
To: linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
nvme_auth_generate_key can fail, don't ignore it upon initialization.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/auth.c | 19 +++++++++++++++----
drivers/nvme/host/core.c | 6 +++++-
drivers/nvme/host/nvme.h | 2 +-
3 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 15cddc2bb14d..a6312a8846f8 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -972,15 +972,26 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
*/
}
-void nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
+int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
{
+ int ret;
+
INIT_LIST_HEAD(&ctrl->dhchap_auth_list);
INIT_WORK(&ctrl->dhchap_auth_work, nvme_dhchap_auth_work);
mutex_init(&ctrl->dhchap_auth_mutex);
if (!ctrl->opts)
- return;
- nvme_auth_generate_key(ctrl->opts->dhchap_secret, &ctrl->host_key);
- nvme_auth_generate_key(ctrl->opts->dhchap_ctrl_secret, &ctrl->ctrl_key);
+ 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) {
+ nvme_auth_free_key(ctrl->host_key);
+ ctrl->host_key = NULL;
+ }
+ return ret;
}
EXPORT_SYMBOL_GPL(nvme_auth_init_ctrl);
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b92da6155773..9b0b11191ed9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5088,9 +5088,13 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
nvme_fault_inject_init(&ctrl->fault_inject, dev_name(ctrl->device));
nvme_mpath_init_ctrl(ctrl);
- nvme_auth_init_ctrl(ctrl);
+ ret = nvme_auth_init_ctrl(ctrl);
+ if (ret)
+ goto out_free_cdev;
return 0;
+out_free_cdev:
+ cdev_device_del(&ctrl->cdev, ctrl->device);
out_free_name:
nvme_put_ctrl(ctrl);
kfree_const(ctrl->device->kobj.name);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index d250efe93c8d..8d9973b00547 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1032,7 +1032,7 @@ static inline bool nvme_ctrl_sgl_supported(struct nvme_ctrl *ctrl)
}
#ifdef CONFIG_NVME_AUTH
-void nvme_auth_init_ctrl(struct nvme_ctrl *ctrl);
+int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl);
void nvme_auth_stop(struct nvme_ctrl *ctrl);
int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid);
int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid);
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 5/6] nvme-auth: don't ignore key generation failures when initializing ctrl keys
2022-10-25 13:43 ` [PATCH 5/6] nvme-auth: don't ignore key generation failures when initializing ctrl keys Sagi Grimberg
@ 2022-10-25 17:24 ` Hannes Reinecke
0 siblings, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2022-10-25 17:24 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni
On 10/25/22 15:43, Sagi Grimberg wrote:
> nvme_auth_generate_key can fail, don't ignore it upon initialization.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
> ---
> drivers/nvme/host/auth.c | 19 +++++++++++++++----
> drivers/nvme/host/core.c | 6 +++++-
> drivers/nvme/host/nvme.h | 2 +-
> 3 files changed, 21 insertions(+), 6 deletions(-)
>
That was actually deliberately, as dhchap keys are optional, so wasn't
sure if a failure in parsing/generating keys should count as a 'real'
failure. But if you say so:
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cheers,
Hannes
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/6] nvme-auth: don't override ctrl keys before validation
2022-10-25 13:43 [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Sagi Grimberg
` (4 preceding siblings ...)
2022-10-25 13:43 ` [PATCH 5/6] nvme-auth: don't ignore key generation failures when initializing ctrl keys Sagi Grimberg
@ 2022-10-25 13:43 ` Sagi Grimberg
2022-10-25 17:25 ` Hannes Reinecke
2022-10-25 15:32 ` [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Christoph Hellwig
6 siblings, 1 reply; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 13:43 UTC (permalink / raw)
To: linux-nvme
Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni, Hannes Reinecke
Replace ctrl ctrl_key/host_key only after nvme_auth_generate_key is successful.
Also, this fixes a bug where the keys are leaked.
Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/core.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9b0b11191ed9..ae1bc79cd712 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3756,13 +3756,17 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
memcpy(dhchap_secret, buf, count);
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, &ctrl->host_key);
+ ret = nvme_auth_generate_key(dhchap_secret, &key);
if (ret)
return ret;
kfree(opts->dhchap_secret);
opts->dhchap_secret = dhchap_secret;
+ host_key = ctrl->host_key;
+ ctrl->host_key = key;
+ nvme_auth_free_key(host_key);
/* Key has changed; re-authentication with new key */
nvme_auth_reset(ctrl);
}
@@ -3806,13 +3810,17 @@ static ssize_t nvme_ctrl_dhchap_ctrl_secret_store(struct device *dev,
memcpy(dhchap_secret, buf, count);
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, &ctrl->ctrl_key);
+ ret = nvme_auth_generate_key(dhchap_secret, &key);
if (ret)
return ret;
kfree(opts->dhchap_ctrl_secret);
opts->dhchap_ctrl_secret = dhchap_secret;
+ ctrl_key = ctrl->ctrl_key;
+ ctrl->ctrl_key = key;
+ nvme_auth_free_key(ctrl_key);
/* Key has changed; re-authentication with new key */
nvme_auth_reset(ctrl);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing
2022-10-25 13:43 [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Sagi Grimberg
` (5 preceding siblings ...)
2022-10-25 13:43 ` [PATCH 6/6] nvme-auth: don't override ctrl keys before validation Sagi Grimberg
@ 2022-10-25 15:32 ` Christoph Hellwig
2022-10-25 15:46 ` Sagi Grimberg
6 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2022-10-25 15:32 UTC (permalink / raw)
To: Sagi Grimberg
Cc: linux-nvme, Christoph Hellwig, Keith Busch, Chaitanya Kulkarni,
Hannes Reinecke
The changes all look reasonable to me, but I think we really need to fix
the grave locking bug in nvme_auth_reset before doing more changs in
this area.
^ permalink raw reply [flat|nested] 19+ messages in thread