linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing
@ 2022-10-25 13:43 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
                   ` (6 more replies)
  0 siblings, 7 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

Sagi Grimberg (6):
  nvme-auth: rename __nvme_auth_[reset|free] to
    nvme_auth[reset|free]_dhchap
  nvme-auth: remove symbol export from nvme_auth_reset
  nvme-auth: don't re-authenticate if the controller is not LIVE
  nvme-auth: remove redundant buffer deallocations
  nvme-auth: don't ignore key generation failures when initializing ctrl
    keys
  nvme-auth: don't override ctrl keys before validation

 drivers/nvme/host/auth.c | 41 +++++++++++++++++++++++++---------------
 drivers/nvme/host/core.c | 16 +++++++++++++---
 drivers/nvme/host/nvme.h |  2 +-
 3 files changed, 40 insertions(+), 19 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [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

* [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

* [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

* [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

* [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

* [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

* Re: [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing
  2022-10-25 15:32 ` [PATCH 0/6] nvme: a few nvme-auth fixes found during code browsing Christoph Hellwig
@ 2022-10-25 15:46   ` Sagi Grimberg
  0 siblings, 0 replies; 19+ messages in thread
From: Sagi Grimberg @ 2022-10-25 15:46 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: linux-nvme, 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.

In the works... this is part one.


^ 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: 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 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 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 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 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

* Re: [PATCH 6/6] nvme-auth: don't override ctrl keys before validation
  2022-10-25 13:43 ` [PATCH 6/6] nvme-auth: don't override ctrl keys before validation Sagi Grimberg
@ 2022-10-25 17:25   ` Hannes Reinecke
  0 siblings, 0 replies; 19+ messages in thread
From: Hannes Reinecke @ 2022-10-25 17:25 UTC (permalink / raw)
  To: Sagi Grimberg, linux-nvme
  Cc: Christoph Hellwig, Keith Busch, Chaitanya Kulkarni

On 10/25/22 15:43, Sagi Grimberg wrote:
> 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(-)
> 
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

* 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

* 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

* 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

end of thread, other threads:[~2022-10-25 20:22 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 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
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
2022-10-25 17:22   ` Hannes Reinecke
2022-10-25 20:22     ` Sagi Grimberg
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
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
2022-10-25 13:43 ` [PATCH 6/6] nvme-auth: don't override ctrl keys before validation 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
2022-10-25 15:46   ` Sagi Grimberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).