Re: [PATCH 06/11] nvme: Implement In-Band authentication

[Hannes Reinecke <hare@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

On 6/21/22 16:59, Sagi Grimberg wrote:
> 
>> Looks like if I pass a malformed ctrl key to nvme connect I am able to
>> crash the system:
> 
> This was what I used in this:
> $ nvme connect -a 192.168.123.1 -t tcp  -s 8009 -n testnqn1 -S 
> "DHHC-1:00:QpxVGpctx5J+4SeW2MClUI8rfZO3WdP1llImvsPsx7e3TK+I:" -C 
> "DHHC-1:00:Jc/My1o0qtLCWRp+sHhAVafdfaS7YQOMYhk9zSmlatobqB8C:"
> 
> The dhchap_ctrl_key is the offending string...
> 
Right. Should be fixed with the attached patch.
Can you check?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

[-- Attachment #2: 0001-nvme-auth-do-not-use-ctrl-opts-dhchap_ctrl_secret.patch --]
[-- Type: text/x-patch, Size: 3356 bytes --]

From 1f4d34016e7cad41f6947143f07f802b05415e26 Mon Sep 17 00:00:00 2001
From: Hannes Reinecke <hare@suse.de>
Date: Wed, 22 Jun 2022 07:57:06 +0200
Subject: [PATCH] nvme-auth: do not use ctrl->opts->dhchap_ctrl_secret

The user might have passed in an invalid controller secret, causing
ctrl->opts->dhchap_ctrl_secret to be present, but ctrl->ctrl_key to
be empty. So always use ctrl->ctrl_key when checking if a valid
controller secret is present.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/auth.c | 13 +++++++------
 drivers/nvme/host/auth.c   |  8 ++++----
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 945f6bb6eb1f..0c86ebce59d2 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -237,21 +237,21 @@ EXPORT_SYMBOL_GPL(nvme_auth_free_key);
 
 u8 *nvme_auth_transform_key(struct nvme_dhchap_key *key, char *nqn)
 {
-	const char *hmac_name = nvme_auth_hmac_name(key->hash);
+	const char *hmac_name;
 	struct crypto_shash *key_tfm;
 	struct shash_desc *shash;
 	u8 *transformed_key;
 	int ret;
 
-	if (key->hash == 0) {
-		transformed_key = kmemdup(key->key, key->len, GFP_KERNEL);
-		return transformed_key ? transformed_key : ERR_PTR(-ENOMEM);
-	}
-
 	if (!key || !key->key) {
 		pr_warn("No key specified\n");
 		return ERR_PTR(-ENOKEY);
 	}
+	if (key->hash == 0) {
+		transformed_key = kmemdup(key->key, key->len, GFP_KERNEL);
+		return transformed_key ? transformed_key : ERR_PTR(-ENOMEM);
+	}
+	hmac_name = nvme_auth_hmac_name(key->hash);
 	if (!hmac_name) {
 		pr_warn("Invalid key hash id %d\n", key->hash);
 		return ERR_PTR(-EINVAL);
@@ -470,6 +470,7 @@ int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key)
 	/* 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);
 	}
 
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 9b84e1e54ca7..53184ac76240 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -314,7 +314,7 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
 	data->hl = chap->hash_len;
 	data->dhvlen = cpu_to_le16(chap->host_key_len);
 	memcpy(data->rval, chap->response, chap->hash_len);
-	if (ctrl->opts->dhchap_ctrl_secret) {
+	if (ctrl->ctrl_key) {
 		get_random_bytes(chap->c2, chap->hash_len);
 		data->cvalid = 1;
 		chap->s2 = nvme_auth_get_seqnum();
@@ -344,7 +344,7 @@ static int nvme_auth_process_dhchap_success1(struct nvme_ctrl *ctrl,
 	struct nvmf_auth_dhchap_success1_data *data = chap->buf;
 	size_t size = sizeof(*data);
 
-	if (ctrl->opts->dhchap_ctrl_secret)
+	if (ctrl->ctrl_key)
 		size += chap->hash_len;
 
 	if (chap->buf_size < size) {
@@ -791,7 +791,7 @@ static void __nvme_auth_work(struct work_struct *work)
 		return;
 	}
 
-	if (ctrl->opts->dhchap_ctrl_secret) {
+	if (ctrl->ctrl_key) {
 		dev_dbg(ctrl->device,
 			"%s: qid %d controller response\n",
 			__func__, chap->qid);
@@ -809,7 +809,7 @@ static void __nvme_auth_work(struct work_struct *work)
 		goto fail2;
 	}
 
-	if (ctrl->opts->dhchap_ctrl_secret) {
+	if (ctrl->ctrl_key) {
 		/* DH-HMAC-CHAP Step 5: send success2 */
 		dev_dbg(ctrl->device, "%s: qid %d send success2\n",
 			__func__, chap->qid);
-- 
2.26.2