[PATCHv3 0/2] nvme: restrict authentication to the admin queue

[PATCHv3 0/2] nvme: restrict authentication to the admin queue

[Hannes Reinecke]

Hi all,

with secure concatenation the spec got more explicit to state that it
would be perfectly fine to implement authentication on the admin queue only.
But once a partner implemented that he found that re-authentication was
failing as we continue to start authentication on all queues.
So these two patches implement this functionalify, the first one to
modify the target to request authentication on the admin queue only,
and the second one to the host to not start authentication on I/O
queues during re-authentication if it wasn't requested initially.

As usual, comments and reviews are welcome.

Changes to the original submission:
- Rebased to nvme-6.14

Changes to v2:
- Include reviews from Sagi
- Drop the configfs attribute

Hannes Reinecke (2):
  nvmet: Authenticate on admin queue only
  nvme: Do not re-authenticate queues with no prior authentication

 drivers/nvme/host/auth.c          | 30 ++++++++++++++++++++++--------
 drivers/nvme/target/auth.c        |  9 ++++++---
 drivers/nvme/target/fabrics-cmd.c |  4 ++--
 3 files changed, 30 insertions(+), 13 deletions(-)

-- 
2.35.3

[PATCH 1/2] nvmet: Authenticate on admin queue only

[Hannes Reinecke]

Do not start authentication on I/O queues as it doesn't really add value,
and secure concatenation disallows it anyway.
Authentication commands on I/O queues are not aborted, so the host may still
run the authentication protocol on I/O queues.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/target/auth.c        | 9 ++++++---
 drivers/nvme/target/fabrics-cmd.c | 4 ++--
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 0eabe1fbfe10..703d514ad42c 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -378,9 +378,12 @@ void nvmet_destroy_auth(struct nvmet_ctrl *ctrl)
 
 bool nvmet_check_auth_status(struct nvmet_req *req)
 {
-	if (req->sq->ctrl->host_key &&
-	    !req->sq->authenticated)
-		return false;
+	if (req->sq->ctrl->host_key) {
+		if (req->sq->qid > 0)
+			return true;
+		if (!req->sq->authenticated)
+			return false;
+	}
 	return true;
 }
 
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index f012bdf89850..14f55192367e 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -239,8 +239,8 @@ static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
 	bool needs_auth = nvmet_has_auth(ctrl, sq);
 	key_serial_t keyid = nvmet_queue_tls_keyid(sq);
 
-	/* Do not authenticate I/O queues for secure concatenation */
-	if (ctrl->concat && sq->qid)
+	/* Do not authenticate I/O queues */
+	if (sq->qid)
 		needs_auth = false;
 
 	if (keyid)
-- 
2.35.3

Re: [PATCH 1/2] nvmet: Authenticate on admin queue only

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

[PATCH 2/2] nvme: Do not re-authenticate queues with no prior authentication

[Hannes Reinecke]

When sending 'connect' the queues can figure out from the return code
whether authentication is required or not. But reauthentication doesn't
disconnect the queues, so this check is not available.
Rather we need to check whether the queue had been authenticated initially
to figure out if we need to reauthenticate.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/nvme/host/auth.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index ebc30071df6b..be83b93aa525 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -30,6 +30,7 @@ struct nvme_dhchap_queue_context {
 	u32 s1;
 	u32 s2;
 	bool bi_directional;
+	bool authenticated;
 	u16 transaction;
 	u8 status;
 	u8 dhgroup_id;
@@ -699,6 +700,7 @@ static void nvme_auth_reset_dhchap(struct nvme_dhchap_queue_context *chap)
 static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap)
 {
 	nvme_auth_reset_dhchap(chap);
+	chap->authenticated = false;
 	if (chap->shash_tfm)
 		crypto_free_shash(chap->shash_tfm);
 	if (chap->dh_tfm)
@@ -947,12 +949,14 @@ static void nvme_queue_auth_work(struct work_struct *work)
 	}
 	if (!ret) {
 		chap->error = 0;
+		chap->authenticated = true;
 		if (ctrl->opts->concat &&
 		    (ret = nvme_auth_secure_concat(ctrl, chap))) {
 			dev_warn(ctrl->device,
 				 "%s: qid %d failed to enable secure concatenation\n",
 				 __func__, chap->qid);
 			chap->error = ret;
+			chap->authenticated = false;
 		}
 		return;
 	}
@@ -1035,13 +1039,16 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
 		return;
 
 	for (q = 1; q < ctrl->queue_count; q++) {
-		ret = nvme_auth_negotiate(ctrl, q);
-		if (ret) {
-			dev_warn(ctrl->device,
-				 "qid %d: error %d setting up authentication\n",
-				 q, ret);
-			break;
-		}
+		struct nvme_dhchap_queue_context *chap =
+			&ctrl->dhchap_ctxs[q];
+		/*
+		 * Skip re-authentication if the queue had
+		 * not been authenticated initially.
+		 */
+		if (!chap->authenticated)
+			continue;
+		cancel_work_sync(&chap->auth_work);
+		queue_work(nvme_auth_wq, &chap->auth_work);
 	}
 
 	/*
@@ -1049,7 +1056,13 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
 	 * the controller terminates the connection.
 	 */
 	for (q = 1; q < ctrl->queue_count; q++) {
-		ret = nvme_auth_wait(ctrl, q);
+		struct nvme_dhchap_queue_context *chap =
+			&ctrl->dhchap_ctxs[q];
+		if (!chap->authenticated)
+			continue;
+		flush_work(&chap->auth_work);
+		ret = chap->error;
+		nvme_auth_reset_dhchap(chap);
 		if (ret)
 			dev_warn(ctrl->device,
 				 "qid %d: authentication failed\n", q);
@@ -1168,6 +1181,7 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
 		chap = &ctrl->dhchap_ctxs[i];
 		chap->qid = i;
 		chap->ctrl = ctrl;
+		chap->authenticated = false;
 		INIT_WORK(&chap->auth_work, nvme_queue_auth_work);
 	}
 
-- 
2.35.3

Re: [PATCHv3 0/2] nvme: restrict authentication to the admin queue

[Christoph Hellwig]

Thanks,

applied to nvme-6.16.

[PATCHv2 0/2] nvme: restrict authentication to the admin queue

[hare]

From: Hannes Reinecke <hare@kernel.org>

Hi all,

with secure concatenation the spec got more explicit to state that it
would be perfectly fine to implement authentication on the admin queue only.
But once a partner implemented that he found that re-authentication was
failing as we continue to start authentication on all queues.
So these two patches implement this functionalify, the first one on
the target (to have a testbed to test against), and the second one
to the host to have it fixed.
Patches are on top of my 'secure-concat.v14' branch on kernel.org.

As usual, comments and reviews are welcome.

Changes to the original submission:
- Rebased to nvme-6.14

Hannes Reinecke (2):
  nvmet: Implement 'admin_only' authentication
  nvme: Do not re-authenticate queues with no prior authentication

 drivers/nvme/host/auth.c               | 12 ++++++++++++
 drivers/nvme/target/auth.c             | 11 +++++++----
 drivers/nvme/target/configfs.c         | 24 ++++++++++++++++++++++++
 drivers/nvme/target/fabrics-cmd-auth.c |  7 +++++++
 drivers/nvme/target/fabrics-cmd.c      |  4 ++--
 drivers/nvme/target/nvmet.h            |  2 ++
 6 files changed, 54 insertions(+), 6 deletions(-)

-- 
2.35.3

[PATCH 2/2] nvme: Do not re-authenticate queues with no prior authentication

[hare]

From: Hannes Reinecke <hare@kernel.org>

When sending 'connect' the queues can figure out from the return code
whether authentication is required or not. But reauthentication doesn't
disconnect the queues, so this check is not available.
Rather we need to check whether the queue had been authenticated initially
to figure out if we need to reauthenticate.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 drivers/nvme/host/auth.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 000c0a4197ba..1a304eb78809 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -31,6 +31,7 @@ struct nvme_dhchap_queue_context {
 	u32 s1;
 	u32 s2;
 	bool bi_directional;
+	bool authenticated;
 	u16 transaction;
 	u8 status;
 	u8 dhgroup_id;
@@ -927,12 +928,14 @@ static void nvme_queue_auth_work(struct work_struct *work)
 	}
 	if (!ret) {
 		chap->error = 0;
+		chap->authenticated = true;
 		if (ctrl->opts->concat &&
 		    (ret = nvme_auth_secure_concat(ctrl, chap))) {
 			dev_warn(ctrl->device,
 				 "%s: qid %d failed to enable secure concatenation\n",
 				 __func__, chap->qid);
 			chap->error = ret;
+			chap->authenticated = false;
 		}
 		return;
 	}
@@ -1021,6 +1024,14 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
 		return;
 
 	for (q = 1; q < ctrl->queue_count; q++) {
+		/*
+		 * Skip re-authentication if the queue had
+		 * not been authenticated initially.
+		 */
+		if (!ctrl->dhchap_ctxs[q].authenticated) {
+			ctrl->dhchap_ctxs[q].error = 0;
+			continue;
+		}
 		ret = nvme_auth_negotiate(ctrl, q);
 		if (ret) {
 			dev_warn(ctrl->device,
@@ -1074,6 +1085,7 @@ int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
 		chap = &ctrl->dhchap_ctxs[i];
 		chap->qid = i;
 		chap->ctrl = ctrl;
+		chap->authenticated = false;
 		INIT_WORK(&chap->auth_work, nvme_queue_auth_work);
 	}
 
-- 
2.35.3

Re: [PATCH 2/2] nvme: Do not re-authenticate queues with no prior authentication

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>