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

[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 1/2] nvmet: Implement 'admin_only' authentication

[hare]

From: Hannes Reinecke <hare@kernel.org>

The spec allows for authentication to run on admin queues only, and secure
concatenation even requires it. So add a configfs attribute 'dhchap_admin_only'
to the target configuration to allow for testing independently of secure
concatenation.

Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
 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 ++
 5 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 0b0645ac5df4..70c8ad25277f 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -190,6 +190,8 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
 		ctrl->shash_id = host->dhchap_hash_id;
 	}
 
+	ctrl->dh_admin_only = host->dhchap_admin_only;
+
 	/* Skip the 'DHHC-1:XX:' prefix */
 	nvme_auth_free_key(ctrl->host_key);
 	ctrl->host_key = nvme_auth_extract_key(host->dhchap_secret + 10,
@@ -280,10 +282,11 @@ 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;
-	return true;
+	if (!req->sq->ctrl->host_key)
+		return true;
+	if (req->sq->qid && req->sq->ctrl->dh_admin_only)
+		return true;
+	return req->sq->authenticated;
 }
 
 int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index f59598766fce..9fd98395f219 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -2219,11 +2219,34 @@ static ssize_t nvmet_host_dhchap_dhgroup_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_host_, dhchap_dhgroup);
 
+static ssize_t nvmet_host_dhchap_admin_only_show(struct config_item *item,
+		char *page)
+{
+	struct nvmet_host *host = to_host(item);
+
+	return sprintf(page, "%d\n", host->dhchap_admin_only);
+}
+
+static ssize_t nvmet_host_dhchap_admin_only_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_host *host = to_host(item);
+	bool val;
+
+	if (kstrtobool(page, &val))
+		return -EINVAL;
+	host->dhchap_admin_only = val;
+	return count;
+}
+
+CONFIGFS_ATTR(nvmet_host_, dhchap_admin_only);
+
 static struct configfs_attribute *nvmet_host_attrs[] = {
 	&nvmet_host_attr_dhchap_key,
 	&nvmet_host_attr_dhchap_ctrl_key,
 	&nvmet_host_attr_dhchap_hash,
 	&nvmet_host_attr_dhchap_dhgroup,
+	&nvmet_host_attr_dhchap_admin_only,
 	NULL,
 };
 #endif /* CONFIG_NVME_TARGET_AUTH */
@@ -2263,6 +2286,7 @@ static struct config_group *nvmet_hosts_make_group(struct config_group *group,
 #ifdef CONFIG_NVME_TARGET_AUTH
 	/* Default to SHA256 */
 	host->dhchap_hash_id = NVME_AUTH_HASH_SHA256;
+	host->dhchap_admin_only = false;
 #endif
 
 	config_group_init_type_name(&host->group, name, &nvmet_host_type);
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index a7135b90f915..96d56ab2465f 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -62,6 +62,7 @@ static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
 			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
 		}
 		ctrl->concat = true;
+		ctrl->dh_admin_only = true;
 	}
 
 	if (data->napd != 1)
@@ -253,6 +254,12 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 			offsetof(struct nvmf_auth_send_command, tl);
 		goto done;
 	}
+	if (req->sq->qid && ctrl->dh_admin_only) {
+		pr_debug("%s: ctrl %d qid %d reject authentication on I/O queues\n",
+			 __func__, ctrl->cntlid, req->sq->qid);
+		status = NVME_SC_INVALID_OPCODE | NVME_STATUS_DNR;
+		goto done;
+	}
 	if (!nvmet_check_transfer_len(req, tl)) {
 		pr_debug("%s: transfer length mismatch (%u)\n", __func__, tl);
 		return;
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 9c01a4b6e543..068494616a3e 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)
+	/* Disable authentication on I/O queues if requested */
+	if (ctrl->dh_admin_only && sq->qid)
 		needs_auth = false;
 
 	if (keyid)
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 4dc7ba5d02a7..49e964b321c0 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -301,6 +301,7 @@ struct nvmet_ctrl {
 	u8			dh_gid;
 	u8			*dh_key;
 	size_t			dh_keysize;
+	bool			dh_admin_only;
 #endif
 #ifdef CONFIG_NVME_TARGET_TCP_TLS
 	struct key		*tls_key;
@@ -379,6 +380,7 @@ struct nvmet_host {
 	u8			dhchap_ctrl_key_hash;
 	u8			dhchap_hash_id;
 	u8			dhchap_dhgroup_id;
+	bool			dhchap_admin_only;
 };
 
 static inline struct nvmet_host *to_host(struct config_item *item)
-- 
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 1/2] nvmet: Implement 'admin_only' authentication

[Sagi Grimberg]

On 24/01/2025 13:47, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@kernel.org>
>
> The spec allows for authentication to run on admin queues only, and secure
> concatenation even requires it. So add a configfs attribute 'dhchap_admin_only'
> to the target configuration to allow for testing independently of secure
> concatenation.

Why do we want it conditionally? why not always?

Re: [PATCH 1/2] nvmet: Implement 'admin_only' authentication

[Hannes Reinecke]

On 1/24/25 14:49, Sagi Grimberg wrote:
> 
> 
> 
> On 24/01/2025 13:47, hare@kernel.org wrote:
>> From: Hannes Reinecke <hare@kernel.org>
>>
>> The spec allows for authentication to run on admin queues only, and 
>> secure
>> concatenation even requires it. So add a configfs attribute 
>> 'dhchap_admin_only'
>> to the target configuration to allow for testing independently of secure
>> concatenation.
> 
> Why do we want it conditionally? why not always?

Because we did support it originally, so I thought to play it safe.
Plus it'll cause a regression with the host implementation if applied
on its own.

But if you say so ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Sagi Grimberg]

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

Re: [PATCH 1/2] nvmet: Implement 'admin_only' authentication

[Sagi Grimberg]

> On 1/24/25 14:49, Sagi Grimberg wrote:
>>
>>
>>
>> On 24/01/2025 13:47, hare@kernel.org wrote:
>>> From: Hannes Reinecke <hare@kernel.org>
>>>
>>> The spec allows for authentication to run on admin queues only, and 
>>> secure
>>> concatenation even requires it. So add a configfs attribute 
>>> 'dhchap_admin_only'
>>> to the target configuration to allow for testing independently of 
>>> secure
>>> concatenation.
>>
>> Why do we want it conditionally? why not always?
>
> Because we did support it originally, so I thought to play it safe.
> Plus it'll cause a regression with the host implementation if applied
> on its own.
>
> But if you say so ...

I'd like to avoid the extra configuration if possible, however we cannot 
break
existing hosts.

Is there a way to not authenticate I/O queues but still allow it?

Re: [PATCH 1/2] nvmet: Implement 'admin_only' authentication

[Hannes Reinecke]

On 1/28/25 09:11, Sagi Grimberg wrote:
> 
>> On 1/24/25 14:49, Sagi Grimberg wrote:
>>>
>>>
>>>
>>> On 24/01/2025 13:47, hare@kernel.org wrote:
>>>> From: Hannes Reinecke <hare@kernel.org>
>>>>
>>>> The spec allows for authentication to run on admin queues only, and 
>>>> secure
>>>> concatenation even requires it. So add a configfs attribute 
>>>> 'dhchap_admin_only'
>>>> to the target configuration to allow for testing independently of 
>>>> secure
>>>> concatenation.
>>>
>>> Why do we want it conditionally? why not always?
>>
>> Because we did support it originally, so I thought to play it safe.
>> Plus it'll cause a regression with the host implementation if applied
>> on its own.
>>
>> But if you say so ...
> 
> I'd like to avoid the extra configuration if possible, however we cannot 
> break
> existing hosts.
> 
> Is there a way to not authenticate I/O queues but still allow it?

Not really. Authentication is driven by the AUTHREQ_ATR / AUTHREQ_ASCR
return status code in the connect response. And that is sent by the
target, so the target has to decide whether it wants authentication on
the queue.
Sadly there is no status for 'whatever, I don't care' ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

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

[Hannes Reinecke]

On 3/13/25 18:29, Kamaljit Singh wrote:
> Hi Hannes,
> 
> One of the key TLS handshake issues we faced is with SHA384 and I see 
> that you had requested this gnutls fix ~2 years ago. I’m curious how you 
> got around that.
> 
> https://gitlab.com/gnutls/gnutls/-/issues/386 <https://gitlab.com/ 
> gnutls/gnutls/-/issues/386>
> 
> We had to hardcode gnutls v3.8.9 to change its default SHA256 to SHA384 
> for using the appropriate binder length.
> 
Yeah, that is quite annoying.
Guess I'll have to bite the bullet and fix gnutls.
Especially as even the maintainer acknowledged that it's an
issue.

BTW, the secure concatenation fixes are now merged with the nvme-6.15 
branch at git.infradead.org/nvme.git, and I've opened a pull request
for blktest:
https://github.com/osandov/blktests/pull/158
which should test possible combinations.
No test for the binder hash, I'm afraid; if you have ideas how to test
this I'd be happy to take them.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich