[PATCHv2 00/13] nvme: implement secure concatenation

[PATCHv2 00/13] nvme: implement secure concatenation

[hare]

From: Hannes Reinecke <hare@suse.de>

Hi all,

here's my attempt to implement secure concatenation for NVMe-of TCP
as outlined in TP8018.
Secure concatenation means that a TLS PSK is generated from the key
material negotiated by the DH-HMAC-CHAP protocol, and the TLS PSK
is then used for a subsequent TLS connection.
The difference between the original definition of secure concatenation
and the method outlined in TP8018 is that with TP8018 the connection
is reset after DH-HMAC-CHAP negotiation, and a new connection is setup
with the generated TLS PSK.

To implement that I have decided on resetting the connection from the
nvme-tcp driver after the initial connection has been set up.
Another way would have been to offload the connection reset to userspace,
and let nvme-cli reset the connection. But that would be a modification
to the userspace interface, and hence I didn't go that way.
The drawback with this approach is that we'll create all I/O queues
before resetting for TLS, even though these queues should never be used.
But fixing that requires a larger rewrite of the TCP driver to unify the
setup and reconnect paths. So keep it that way for now.

As usual, comments and reviews are welcome.

Changes to the original submission:
- Sanitize TLS key handling
- Fixup modconfig compilation

Hannes Reinecke (13):
  crypto,fs: Separate out hkdf_extract() and hkdf_expand()
  nvme: add nvme_auth_generate_psk()
  nvme: add nvme_auth_generate_digest()
  nvme: add nvme_auth_derive_tls_psk()
  nvme-keyring: add nvme_tls_psk_refresh()
  nvme-keyring: restrict match length for version '1' identifiers
  nvme-tcp: check for invalidated or revoked key
  nvme-fabrics: authentication errors are not retryable
  nvme-tcp: sanitize TLS key handling
  nvme-tcp: request secure channel concatenation
  nvme-tcp: combine reset and recovery
  nvme-tcp: reset after recovery for secure concatenation
  nvmet-tcp: support secure channel concatenation

 crypto/Makefile                        |   1 +
 crypto/hkdf.c                          | 111 +++++++++++
 drivers/nvme/common/auth.c             | 252 +++++++++++++++++++++++++
 drivers/nvme/common/keyring.c          |  71 +++++++
 drivers/nvme/host/auth.c               | 108 ++++++++++-
 drivers/nvme/host/core.c               |   1 -
 drivers/nvme/host/fabrics.c            |  46 ++++-
 drivers/nvme/host/fabrics.h            |   3 +
 drivers/nvme/host/nvme.h               |   1 -
 drivers/nvme/host/sysfs.c              |   9 +-
 drivers/nvme/host/tcp.c                | 106 ++++++++---
 drivers/nvme/target/auth.c             |  73 ++++++-
 drivers/nvme/target/fabrics-cmd-auth.c |  46 ++++-
 drivers/nvme/target/fabrics-cmd.c      |  30 ++-
 drivers/nvme/target/nvmet.h            |  30 ++-
 drivers/nvme/target/tcp.c              |  27 +++
 fs/crypto/hkdf.c                       |  68 +------
 include/crypto/hkdf.h                  |  18 ++
 include/linux/nvme-auth.h              |   5 +
 include/linux/nvme-keyring.h           |   7 +
 include/linux/nvme.h                   |   7 +
 21 files changed, 898 insertions(+), 122 deletions(-)
 create mode 100644 crypto/hkdf.c
 create mode 100644 include/crypto/hkdf.h

-- 
2.35.3

[PATCH 01/13] crypto,fs: Separate out hkdf_extract() and hkdf_expand()

[hare]

From: Hannes Reinecke <hare@suse.de>

Separate out the HKDF functions into a separate file to make them
available to other callers.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 crypto/Makefile       |   1 +
 crypto/hkdf.c         | 111 ++++++++++++++++++++++++++++++++++++++++++
 fs/crypto/hkdf.c      |  68 ++++----------------------
 include/crypto/hkdf.h |  18 +++++++
 4 files changed, 139 insertions(+), 59 deletions(-)
 create mode 100644 crypto/hkdf.c
 create mode 100644 include/crypto/hkdf.h

diff --git a/crypto/Makefile b/crypto/Makefile
index 5ac6876f935a..282aefa1d74e 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o
 
 crypto_hash-y += ahash.o
 crypto_hash-y += shash.o
+crypto_hash-y += hkdf.o
 obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
 
 obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
diff --git a/crypto/hkdf.c b/crypto/hkdf.c
new file mode 100644
index 000000000000..dbe85c441600
--- /dev/null
+++ b/crypto/hkdf.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
+ * Function"), aka RFC 5869.  See also the original paper (Krawczyk 2010):
+ * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
+ *
+ * This is used to derive keys from the fscrypt master keys.
+ *
+ * Copyright 2019 Google LLC
+ */
+
+#include <crypto/hash.h>
+#include <crypto/sha2.h>
+
+/*
+ * HKDF consists of two steps:
+ *
+ * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from
+ *    the input keying material and optional salt.
+ * 2. HKDF-Expand: expand the pseudorandom key into output keying material of
+ *    any length, parameterized by an application-specific info string.
+ *
+ */
+
+/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
+int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
+		 unsigned int ikmlen, u8 *prk)
+{
+	unsigned int prklen = crypto_shash_digestsize(hmac_tfm);
+	u8 *default_salt;
+	int err;
+
+	default_salt = kzalloc(prklen, GFP_KERNEL);
+	if (!default_salt)
+		return -ENOMEM;
+	err = crypto_shash_setkey(hmac_tfm, default_salt, prklen);
+	if (!err)
+		err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
+
+	kfree(default_salt);
+	return err;
+}
+EXPORT_SYMBOL_GPL(hkdf_extract);
+
+/*
+ * HKDF-Expand (RFC 5869 section 2.3).
+ * This expands the pseudorandom key, which was already keyed into @hmac_tfm,
+ * into @okmlen bytes of output keying material parameterized by the
+ * application-specific @info of length @infolen bytes.
+ * This is thread-safe and may be called by multiple threads in parallel.
+ */
+int hkdf_expand(struct crypto_shash *hmac_tfm,
+		const u8 *info, unsigned int infolen,
+		u8 *okm, unsigned int okmlen)
+{
+	SHASH_DESC_ON_STACK(desc, hmac_tfm);
+	unsigned int i, hashlen = crypto_shash_digestsize(hmac_tfm);
+	int err;
+	const u8 *prev = NULL;
+	u8 counter = 1;
+	u8 *tmp;
+
+	if (WARN_ON(okmlen > 255 * hashlen))
+		return -EINVAL;
+
+	tmp = kzalloc(hashlen, GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+
+	desc->tfm = hmac_tfm;
+
+	for (i = 0; i < okmlen; i += hashlen) {
+
+		err = crypto_shash_init(desc);
+		if (err)
+			goto out;
+
+		if (prev) {
+			err = crypto_shash_update(desc, prev, hashlen);
+			if (err)
+				goto out;
+		}
+
+		err = crypto_shash_update(desc, info, infolen);
+		if (err)
+			goto out;
+
+		BUILD_BUG_ON(sizeof(counter) != 1);
+		if (okmlen - i < hashlen) {
+			err = crypto_shash_finup(desc, &counter, 1, tmp);
+			if (err)
+				goto out;
+			memcpy(&okm[i], tmp, okmlen - i);
+			memzero_explicit(tmp, sizeof(tmp));
+		} else {
+			err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
+			if (err)
+				goto out;
+		}
+		counter++;
+		prev = &okm[i];
+	}
+	err = 0;
+out:
+	if (unlikely(err))
+		memzero_explicit(okm, okmlen); /* so caller doesn't need to */
+	shash_desc_zero(desc);
+	kfree(tmp);
+	return err;
+}
+EXPORT_SYMBOL_GPL(hkdf_expand);
diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
index 5a384dad2c72..9c2f9aca9412 100644
--- a/fs/crypto/hkdf.c
+++ b/fs/crypto/hkdf.c
@@ -11,6 +11,7 @@
 
 #include <crypto/hash.h>
 #include <crypto/sha2.h>
+#include <crypto/hkdf.h>
 
 #include "fscrypt_private.h"
 
@@ -44,20 +45,6 @@
  * there's no way to persist a random salt per master key from kernel mode.
  */
 
-/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
-static int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
-			unsigned int ikmlen, u8 prk[HKDF_HASHLEN])
-{
-	static const u8 default_salt[HKDF_HASHLEN];
-	int err;
-
-	err = crypto_shash_setkey(hmac_tfm, default_salt, HKDF_HASHLEN);
-	if (err)
-		return err;
-
-	return crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
-}
-
 /*
  * Compute HKDF-Extract using the given master key as the input keying material,
  * and prepare an HMAC transform object keyed by the resulting pseudorandom key.
@@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
 			u8 *okm, unsigned int okmlen)
 {
 	SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
-	u8 prefix[9];
-	unsigned int i;
+	u8 *prefix;
 	int err;
-	const u8 *prev = NULL;
-	u8 counter = 1;
-	u8 tmp[HKDF_HASHLEN];
 
 	if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN))
 		return -EINVAL;
 
+	prefix = kzalloc(okmlen + 9, GFP_KERNEL);
+	if (!prefix)
+		return -ENOMEM;
 	desc->tfm = hkdf->hmac_tfm;
 
 	memcpy(prefix, "fscrypt\0", 8);
 	prefix[8] = context;
+	memcpy(prefix + 9, info, infolen);
 
-	for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
-
-		err = crypto_shash_init(desc);
-		if (err)
-			goto out;
-
-		if (prev) {
-			err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
-			if (err)
-				goto out;
-		}
-
-		err = crypto_shash_update(desc, prefix, sizeof(prefix));
-		if (err)
-			goto out;
-
-		err = crypto_shash_update(desc, info, infolen);
-		if (err)
-			goto out;
-
-		BUILD_BUG_ON(sizeof(counter) != 1);
-		if (okmlen - i < HKDF_HASHLEN) {
-			err = crypto_shash_finup(desc, &counter, 1, tmp);
-			if (err)
-				goto out;
-			memcpy(&okm[i], tmp, okmlen - i);
-			memzero_explicit(tmp, sizeof(tmp));
-		} else {
-			err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
-			if (err)
-				goto out;
-		}
-		counter++;
-		prev = &okm[i];
-	}
-	err = 0;
-out:
-	if (unlikely(err))
-		memzero_explicit(okm, okmlen); /* so caller doesn't need to */
-	shash_desc_zero(desc);
+	err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8,
+			  okm, okmlen);
+	kfree(prefix);
 	return err;
 }
 
diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
new file mode 100644
index 000000000000..bf06c080d7ed
--- /dev/null
+++ b/include/crypto/hkdf.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869
+ *
+ * Extracted from fs/crypto/hkdf.c, which has
+ * Copyright 2019 Google LLC
+ */
+
+#ifndef _CRYPTO_HKDF_H
+#define _CRYPTO_HKDF_H
+
+int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
+		 unsigned int ikmlen, u8 *prk);
+int hkdf_expand(struct crypto_shash *hmac_tfm,
+		const u8 *info, unsigned int infolen,
+		u8 *okm, unsigned int okmlen);
+
+#endif
-- 
2.35.3

[PATCH 02/13] nvme: add nvme_auth_generate_psk()

[hare]

From: Hannes Reinecke <hare@suse.de>

Add a function to generate a NVMe PSK from the shared credentials
negotiated by DH-HMAC-CHAP.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/auth.c | 76 ++++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |  2 +
 2 files changed, 78 insertions(+)

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index a23ab5c968b9..9ee459a9400e 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -11,6 +11,7 @@
 #include <asm/unaligned.h>
 #include <crypto/hash.h>
 #include <crypto/dh.h>
+#include <crypto/hkdf.h>
 #include <linux/nvme.h>
 #include <linux/nvme-auth.h>
 
@@ -471,4 +472,79 @@ int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key)
 }
 EXPORT_SYMBOL_GPL(nvme_auth_generate_key);
 
+u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len)
+{
+	struct crypto_shash *tfm;
+	struct shash_desc *shash;
+	u8 *psk;
+	const char *hmac_name;
+	int ret, psk_len;
+
+	if (!c1 || !c2) {
+		pr_warn("%s: invalid parameter\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	hmac_name = nvme_auth_hmac_name(hmac_id);
+	if (!hmac_name) {
+		pr_warn("%s: invalid hash algoritm %d\n",
+			__func__, hmac_id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	tfm = crypto_alloc_shash(hmac_name, 0, 0);
+	if (IS_ERR(tfm))
+		return (u8 *)tfm;
+
+	psk_len = crypto_shash_digestsize(tfm);
+	psk = kzalloc(psk_len, GFP_KERNEL);
+	if (!psk) {
+		ret = -ENOMEM;
+		goto out_free_tfm;
+	}
+
+	shash = kmalloc(sizeof(struct shash_desc) +
+			crypto_shash_descsize(tfm),
+			GFP_KERNEL);
+	if (!shash) {
+		ret = -ENOMEM;
+		goto out_free_psk;
+	}
+
+	shash->tfm = tfm;
+	ret = crypto_shash_setkey(tfm, skey, skey_len);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_init(shash);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_update(shash, c1, hash_len);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_update(shash, c2, hash_len);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_final(shash, psk);
+	if (!ret)
+		*ret_len = psk_len;
+
+out_free_shash:
+	kfree_sensitive(shash);
+out_free_psk:
+	if (ret) {
+		kfree_sensitive(psk);
+		psk = NULL;
+	}
+out_free_tfm:
+	crypto_free_shash(tfm);
+
+	return ret ? ERR_PTR(ret) : psk;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_generate_psk);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index c1d0bc5d9624..31dc1db2d4d6 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -40,5 +40,7 @@ int nvme_auth_gen_pubkey(struct crypto_kpp *dh_tfm,
 int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
 				u8 *ctrl_key, size_t ctrl_key_len,
 				u8 *sess_key, size_t sess_key_len);
+u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
 
 #endif /* _NVME_AUTH_H */
-- 
2.35.3

[PATCH 03/13] nvme: add nvme_auth_generate_digest()

[hare]

From: Hannes Reinecke <hare@suse.de>

Add a function to calculate the PSK digest as specified in TP8018.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/auth.c | 105 +++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |   2 +
 2 files changed, 107 insertions(+)

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 9ee459a9400e..87e095fe3852 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -547,4 +547,109 @@ u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 }
 EXPORT_SYMBOL_GPL(nvme_auth_generate_psk);
 
+u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+			      char *subsysnqn, char *hostnqn)
+{
+	struct crypto_shash *tfm;
+	struct shash_desc *shash;
+	u8 *digest, *hmac;
+	const char *hmac_name;
+	size_t digest_len, hmac_len;
+	int ret;
+
+	if (WARN_ON(!subsysnqn || !hostnqn))
+		return ERR_PTR(-EINVAL);
+
+	hmac_name = nvme_auth_hmac_name(hmac_id);
+	if (!hmac_name) {
+		pr_warn("%s: invalid hash algoritm %d\n",
+			__func__, hmac_id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	switch (nvme_auth_hmac_hash_len(hmac_id)) {
+	case 32:
+		hmac_len = 44;
+		break;
+	case 48:
+		hmac_len = 64;
+		break;
+	default:
+		pr_warn("%s: invalid hash algorithm '%s'\n",
+			__func__, hmac_name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	hmac = kzalloc(hmac_len + 1, GFP_KERNEL);
+	if (!hmac)
+		return ERR_PTR(-ENOMEM);
+
+	tfm = crypto_alloc_shash(hmac_name, 0, 0);
+	if (IS_ERR(tfm))
+		goto out_free_hmac;
+
+	digest_len = crypto_shash_digestsize(tfm);
+	digest = kzalloc(digest_len, GFP_KERNEL);
+	if (!digest) {
+		ret = -ENOMEM;
+		goto out_free_tfm;
+	}
+
+	shash = kmalloc(sizeof(struct shash_desc) +
+			crypto_shash_descsize(tfm),
+			GFP_KERNEL);
+	if (!shash) {
+		ret = -ENOMEM;
+		goto out_free_digest;
+	}
+
+	shash->tfm = tfm;
+	ret = crypto_shash_setkey(tfm, psk, psk_len);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_init(shash);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_update(shash, hostnqn, strlen(hostnqn));
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_update(shash, " ", 1);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_update(shash, subsysnqn, strlen(subsysnqn));
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_update(shash, " NVMe-over-Fabrics", 18);
+	if (ret)
+		goto out_free_shash;
+
+	ret = crypto_shash_final(shash, digest);
+	if (ret)
+		goto out_free_shash;
+
+	ret = base64_encode(digest, digest_len, hmac);
+	if (ret < hmac_len)
+		ret = -ENOKEY;
+	ret = 0;
+
+out_free_shash:
+	kfree_sensitive(shash);
+out_free_digest:
+	kfree_sensitive(digest);
+out_free_tfm:
+	crypto_free_shash(tfm);
+out_free_hmac:
+	if (ret) {
+		kfree_sensitive(hmac);
+		hmac = NULL;
+	}
+	return ret ? ERR_PTR(ret) : hmac;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_generate_digest);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index 31dc1db2d4d6..2cbb9249a8b3 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -42,5 +42,7 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
 				u8 *sess_key, size_t sess_key_len);
 u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
+u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+			      char *subsysnqn, char *hostnqn);
 
 #endif /* _NVME_AUTH_H */
-- 
2.35.3

[PATCH 04/13] nvme: add nvme_auth_derive_tls_psk()

[hare]

From: Hannes Reinecke <hare@suse.de>

Add a function to derive the TLS PSK as specified TP8018.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/auth.c | 71 ++++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |  1 +
 2 files changed, 72 insertions(+)

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 87e095fe3852..20eb809929e2 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -652,4 +652,75 @@ u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
 }
 EXPORT_SYMBOL_GPL(nvme_auth_generate_digest);
 
+u8 *nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len, u8 *psk_digest)
+{
+	struct crypto_shash *hmac_tfm;
+	const char *hmac_name;
+	const char *psk_prefix = "tls13 nvme-tls-psk";
+	size_t info_len, prk_len;
+	char *info;
+	unsigned char *prk, *tls_key;
+	int ret;
+
+	hmac_name = nvme_auth_hmac_name(hmac_id);
+	if (!hmac_name) {
+		pr_warn("%s: invalid hash algoritm %d\n",
+			__func__, hmac_id);
+		return ERR_PTR(-EINVAL);
+	}
+	if (hmac_id == NVME_AUTH_HASH_SHA512) {
+		pr_warn("%s: unsupported hash algorithm %s\n",
+			__func__, hmac_name);
+		return ERR_PTR(-EINVAL);
+	}
+
+	hmac_tfm = crypto_alloc_shash(hmac_name, 0, 0);
+	if (IS_ERR(hmac_tfm))
+		return (u8 *)hmac_tfm;
+
+	prk_len = crypto_shash_digestsize(hmac_tfm);
+	prk = kzalloc(prk_len, GFP_KERNEL);
+	if (!prk) {
+		ret = -ENOMEM;
+		goto out_free_shash;
+	}
+
+	ret = hkdf_extract(hmac_tfm, psk, psk_len, prk);
+	if (ret)
+		goto out_free_prk;
+
+	ret = crypto_shash_setkey(hmac_tfm, prk, prk_len);
+	if (ret)
+		goto out_free_prk;
+
+	info_len = strlen(psk_digest) + strlen(psk_prefix) + 1;
+	info = kzalloc(info_len, GFP_KERNEL);
+	if (!info)
+		goto out_free_prk;
+
+	memcpy(info, psk_prefix, strlen(psk_prefix));
+	memcpy(info + strlen(psk_prefix), psk_digest, strlen(psk_digest));
+
+	tls_key = kzalloc(psk_len, GFP_KERNEL);
+	if (!tls_key) {
+		ret = -ENOMEM;
+		goto out_free_info;
+	}
+	ret = hkdf_expand(hmac_tfm, info, strlen(info), tls_key, psk_len);
+	if (ret)
+		goto out_free_key;
+
+out_free_key:
+	kfree(tls_key);
+out_free_info:
+	kfree(info);
+out_free_prk:
+	kfree(prk);
+out_free_shash:
+	crypto_free_shash(hmac_tfm);
+
+	return ret ? ERR_PTR(ret) : tls_key;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_derive_tls_psk);
+
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index 2cbb9249a8b3..335236fb2b73 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -44,5 +44,6 @@ u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
 u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
 			      char *subsysnqn, char *hostnqn);
+u8 *nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len, u8 *psk_digest);
 
 #endif /* _NVME_AUTH_H */
-- 
2.35.3

[PATCH 05/13] nvme-keyring: add nvme_tls_psk_refresh()

[hare]

From: Hannes Reinecke <hare@suse.de>

Add a function to refresh a generated PSK in the specified keyring.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/keyring.c | 50 +++++++++++++++++++++++++++++++++++
 include/linux/nvme-keyring.h  |  7 +++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index a5c0431c101c..c16f9496643f 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -104,6 +104,56 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
 	return key_ref_to_ptr(keyref);
 }
 
+struct key *nvme_tls_psk_refresh(struct key *keyring, char *hostnqn, char *subnqn,
+		u8 hmac_id, bool generated, u8 *data, size_t data_len, char *digest)
+{
+	key_perm_t keyperm =
+		KEY_POS_SEARCH | KEY_POS_VIEW | KEY_POS_READ |
+		KEY_POS_WRITE | KEY_POS_LINK | KEY_POS_SETATTR |
+		KEY_USR_SEARCH | KEY_USR_VIEW | KEY_USR_READ;
+	char *identity;
+	size_t identity_len = (NVMF_NQN_SIZE) * 2 + 77;
+	key_ref_t keyref;
+	key_serial_t keyring_id;
+	struct key *key;
+
+	if (!hostnqn || !subnqn || !data || !data_len)
+		return ERR_PTR(-EINVAL);
+
+	identity = kzalloc(identity_len, GFP_KERNEL);
+	if (!identity)
+		return ERR_PTR(-ENOMEM);
+
+	snprintf(identity, identity_len, "NVMe1%c%02d %s %s %s",
+		 generated ? 'G' : 'R', hmac_id, hostnqn, subnqn, digest);
+
+	if (!keyring)
+		keyring = nvme_keyring;
+	keyring_id = key_serial(keyring);
+	pr_debug("keyring %x refresh tls psk '%s'\n",
+		 keyring_id, identity);
+	keyref = key_create_or_update(make_key_ref(keyring, true),
+				"psk", identity, data, data_len,
+				keyperm, KEY_ALLOC_NOT_IN_QUOTA |
+				      KEY_ALLOC_BUILT_IN |
+				      KEY_ALLOC_BYPASS_RESTRICTION);
+	if (IS_ERR(keyref)) {
+		pr_debug("refresh tls psk '%s' failed, error %ld\n",
+			 identity, PTR_ERR(keyref));
+		kfree(identity);
+		return ERR_PTR(-ENOKEY);
+	}
+	kfree(identity);
+	/*
+	 * Set the default timeout to 1 hour
+	 * as suggested in TP8018.
+	 */
+	key = key_ref_to_ptr(keyref);
+	key_set_timeout(key, 3600);
+	return key;
+}
+EXPORT_SYMBOL_GPL(nvme_tls_psk_refresh);
+
 /*
  * NVMe PSK priority list
  *
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index e10333d78dbb..d2b8a0783ad7 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
@@ -8,6 +8,8 @@
 
 #if IS_ENABLED(CONFIG_NVME_KEYRING)
 
+struct key *nvme_tls_psk_refresh(struct key *keyring, char *hostnqn, char *subnqn,
+				 u8 hmac_id, bool generated, u8 *data, size_t data_len, char *digest);
 key_serial_t nvme_tls_psk_default(struct key *keyring,
 		const char *hostnqn, const char *subnqn);
 
@@ -15,6 +17,11 @@ key_serial_t nvme_keyring_id(void);
 
 #else
 
+static struct key *nvme_tls_psk_refresh(struct key *keyring, char *hostnqn, char *subnqn,
+		u8 hmac_id, bool generated, u8 *data, size_t data_len, char *digest)
+{
+	return -ENOTSUPP;
+}
 static inline key_serial_t nvme_tls_psk_default(struct key *keyring,
 		const char *hostnqn, const char *subnqn)
 {
-- 
2.35.3

[PATCH 06/13] nvme-keyring: restrict match length for version '1' identifiers

[hare]

From: Hannes Reinecke <hare@suse.de>

TP8018 changed the TLS PSK identifiers to append a PSK hash value,
so to lookup any version '1' identifiers we need to restrict the
match length to exclude the PSK hash value (which we don't have
when looking up keys).

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index c16f9496643f..51b99b34e100 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -44,6 +44,27 @@ static bool nvme_tls_psk_match(const struct key *key,
 		return false;
 	}
 	match_id = match_data->raw_data;
+	if (memcmp(match_id, "NVMe1", 5)) {
+		char *e = (char *)match_id;
+		size_t offset = 0;
+		int n = 0;
+
+		while (*e != ' ' && offset < match_len) {
+			if (*e == ' ') {
+				n++;
+				if (n == 3)
+					break;
+			}
+			e++;
+			offset++;
+		}
+		if (n != 3) {
+			pr_debug("%s: error parsing '%s'\n",
+				 __func__, match_id);
+			return false;
+		}
+		match_len = offset;
+	}
 	pr_debug("%s: match '%s' '%s' len %zd\n",
 		 __func__, match_id, key->description, match_len);
 	return !memcmp(key->description, match_id, match_len);
-- 
2.35.3

[PATCH 07/13] nvme-tcp: check for invalidated or revoked key

[hare]

From: Hannes Reinecke <hare@suse.de>

key_lookup() will always return a key, even if that key is revoked
or invalidated. So check for invalid keys before continuing.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/fabrics.c | 7 ++++++-
 drivers/nvme/host/sysfs.c   | 9 +++++++--
 drivers/nvme/host/tcp.c     | 8 +++++++-
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index aa88606a44c4..a7da088331dc 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -635,7 +635,12 @@ static struct key *nvmf_parse_key(int key_id)
 	key = key_lookup(key_id);
 	if (!IS_ERR(key))
 		pr_err("key id %08x not found\n", key_id);
-	else
+	else if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
+		 test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
+		pr_err("key id %08x invalid\n", key_id);
+		key_put(key);
+		key = ERR_PTR(-EKEYREVOKED);
+	} else
 		pr_debug("Using key id %08x\n", key_id);
 	return key;
 }
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 754e91111042..1076b5b59b35 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -617,10 +617,15 @@ static ssize_t tls_key_show(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	struct key *key = ctrl->tls_key;
 
-	if (!ctrl->tls_key)
+	if (!key)
 		return 0;
-	return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
+	if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
+	    test_bit(KEY_FLAG_INVALIDATED, &key->flags))
+		return -EKEYREVOKED;
+
+	return sysfs_emit(buf, "%08x", key_serial(key));
 }
 static DEVICE_ATTR_RO(tls_key);
 #endif
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index c160b1a64ec0..65d9a817e752 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1571,9 +1571,15 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
 
 	tls_key = key_lookup(pskid);
 	if (IS_ERR(tls_key)) {
-		dev_warn(ctrl->ctrl.device, "queue %d: Invalid key %x\n",
+		dev_warn(ctrl->ctrl.device, "queue %d: key %08x not found\n",
 			 qid, pskid);
 		queue->tls_err = -ENOKEY;
+	} else if (test_bit(KEY_FLAG_REVOKED, &tls_key->flags) ||
+		   test_bit(KEY_FLAG_INVALIDATED, &tls_key->flags)) {
+		dev_warn(ctrl->ctrl.device, "queue %d: key %08x invalid\n",
+			 qid, pskid);
+		key_put(tls_key);
+		queue->tls_err = -EKEYREVOKED;
 	} else {
 		ctrl->ctrl.tls_key = tls_key;
 		queue->tls_err = 0;
-- 
2.35.3

[PATCH 08/13] nvme-fabrics: authentication errors are not retryable

[hare]

From: Hannes Reinecke <hare@suse.de>

Whenever authentication failed the error is non-retryable as the
authentication solely depends on the passed parameters, which do
not change during retry.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/fabrics.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index a7da088331dc..c6feb6e1e05f 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -465,7 +465,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 		if (result & NVME_CONNECT_AUTHREQ_ASCR) {
 			dev_warn(ctrl->device,
 				 "qid 0: secure concatenation is not supported\n");
-			ret = NVME_SC_AUTH_REQUIRED;
+			ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 			goto out_free_data;
 		}
 		/* Authentication required */
@@ -473,7 +473,7 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 		if (ret) {
 			dev_warn(ctrl->device,
 				 "qid 0: authentication setup failed\n");
-			ret = NVME_SC_AUTH_REQUIRED;
+			ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 			goto out_free_data;
 		}
 		ret = nvme_auth_wait(ctrl, 0);
@@ -537,7 +537,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
 		if (result & NVME_CONNECT_AUTHREQ_ASCR) {
 			dev_warn(ctrl->device,
 				 "qid 0: secure concatenation is not supported\n");
-			ret = NVME_SC_AUTH_REQUIRED;
+			ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 			goto out_free_data;
 		}
 		/* Authentication required */
@@ -545,7 +545,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
 		if (ret) {
 			dev_warn(ctrl->device,
 				 "qid %d: authentication setup failed\n", qid);
-			ret = NVME_SC_AUTH_REQUIRED;
+			ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 		} else {
 			ret = nvme_auth_wait(ctrl, qid);
 			if (ret)
-- 
2.35.3

[PATCH 09/13] nvme-tcp: sanitize TLS key handling

[hare]

From: Hannes Reinecke <hare@suse.de>

There is a difference between TLS configured (ie the user has
provisioned/requested a key) and TLS enabled (ie the connection
is encrypted with TLS).
So to differentiate between those two states store the provisioned
key in opts->tls_key (as we're using the same TLS key for all queues)
and the key serial of the key negotiated by the TLS handshake
in queue->tls_key.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c  |  1 -
 drivers/nvme/host/nvme.h  |  1 -
 drivers/nvme/host/sysfs.c |  2 +-
 drivers/nvme/host/tcp.c   | 40 +++++++++++++++++++++++----------------
 4 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 50818dbcfa1a..376efb310eb2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4450,7 +4450,6 @@ static void nvme_free_ctrl(struct device *dev)
 
 	if (!subsys || ctrl->instance != subsys->instance)
 		ida_free(&nvme_instance_ida, ctrl->instance);
-	key_put(ctrl->tls_key);
 	nvme_free_cels(ctrl);
 	nvme_mpath_uninit(ctrl);
 	nvme_auth_stop(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 6092cc361837..396e6e72b6a3 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -357,7 +357,6 @@ struct nvme_ctrl {
 	struct nvme_dhchap_key *ctrl_key;
 	u16 transaction;
 #endif
-	struct key *tls_key;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 1076b5b59b35..dd18a118d053 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -617,7 +617,7 @@ static ssize_t tls_key_show(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	struct key *key = ctrl->tls_key;
+	struct key *key = ctrl->opts->tls_key;
 
 	if (!key)
 		return 0;
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 65d9a817e752..642c0fc0941b 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -163,6 +163,7 @@ struct nvme_tcp_queue {
 	__le32			recv_ddgst;
 	struct completion       tls_complete;
 	int                     tls_err;
+	key_serial_t		tls_key;
 	struct page_frag_cache	pf_cache;
 
 	void (*state_change)(struct sock *);
@@ -205,7 +206,15 @@ static inline int nvme_tcp_queue_id(struct nvme_tcp_queue *queue)
 	return queue - queue->ctrl->queues;
 }
 
-static inline bool nvme_tcp_tls(struct nvme_ctrl *ctrl)
+static inline bool nvme_tcp_tls_enabled(struct nvme_tcp_queue *queue)
+{
+	if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
+		return 0;
+
+	return (queue->tls_key != 0);
+}
+
+static inline bool nvme_tcp_tls_configured(struct nvme_ctrl *ctrl)
 {
 	if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
 		return 0;
@@ -1418,7 +1427,7 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
 	memset(&msg, 0, sizeof(msg));
 	iov.iov_base = icresp;
 	iov.iov_len = sizeof(*icresp);
-	if (nvme_tcp_tls(&queue->ctrl->ctrl)) {
+	if (nvme_tcp_tls_enabled(queue)) {
 		msg.msg_control = cbuf;
 		msg.msg_controllen = sizeof(cbuf);
 	}
@@ -1430,7 +1439,7 @@ static int nvme_tcp_init_connection(struct nvme_tcp_queue *queue)
 		goto free_icresp;
 	}
 	ret = -ENOTCONN;
-	if (nvme_tcp_tls(&queue->ctrl->ctrl)) {
+	if (nvme_tcp_tls_enabled(queue)) {
 		ctype = tls_get_record_type(queue->sock->sk,
 					    (struct cmsghdr *)cbuf);
 		if (ctype != TLS_RECORD_TYPE_DATA) {
@@ -1581,7 +1590,8 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
 		key_put(tls_key);
 		queue->tls_err = -EKEYREVOKED;
 	} else {
-		ctrl->ctrl.tls_key = tls_key;
+		queue->tls_key = pskid;
+		key_put(tls_key);
 		queue->tls_err = 0;
 	}
 
@@ -1762,7 +1772,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
 	}
 
 	/* If PSKs are configured try to start TLS */
-	if (IS_ENABLED(CONFIG_NVME_TCP_TLS) && pskid) {
+	if (nvme_tcp_tls_configured(nctrl) && pskid) {
 		ret = nvme_tcp_start_tls(nctrl, queue, pskid);
 		if (ret)
 			goto err_init_connect;
@@ -1919,16 +1929,17 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
 	int ret;
 	key_serial_t pskid = 0;
 
-	if (nvme_tcp_tls(ctrl)) {
+	if (nvme_tcp_tls_configured(ctrl)) {
 		if (ctrl->opts->tls_key)
 			pskid = key_serial(ctrl->opts->tls_key);
-		else
+		else {
 			pskid = nvme_tls_psk_default(ctrl->opts->keyring,
 						      ctrl->opts->host->nqn,
 						      ctrl->opts->subsysnqn);
-		if (!pskid) {
-			dev_err(ctrl->device, "no valid PSK found\n");
-			return -ENOKEY;
+			if (!pskid) {
+				dev_err(ctrl->device, "no valid PSK found\n");
+				return -ENOKEY;
+			}
 		}
 	}
 
@@ -1949,15 +1960,12 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
 
 static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
 {
+	struct nvme_tcp_ctrl *tcp_ctrl = to_tcp_ctrl(ctrl);
+	key_serial_t pskid = tcp_ctrl->queues[0].tls_key;
 	int i, ret;
 
-	if (nvme_tcp_tls(ctrl) && !ctrl->tls_key) {
-		dev_err(ctrl->device, "no PSK negotiated\n");
-		return -ENOKEY;
-	}
 	for (i = 1; i < ctrl->queue_count; i++) {
-		ret = nvme_tcp_alloc_queue(ctrl, i,
-				key_serial(ctrl->tls_key));
+		ret = nvme_tcp_alloc_queue(ctrl, i, pskid);
 		if (ret)
 			goto out_free_queues;
 	}
-- 
2.35.3

[PATCH 10/13] nvme-tcp: request secure channel concatenation

[hare]

From: Hannes Reinecke <hare@suse.de>

Add a fabrics option 'concat' to request secure channel concatenation.
When secure channel concatenation is enabled a 'generated PSK' is inserted
into the keyring such that it's available for the next connection attempt.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c    | 108 ++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/fabrics.c |  31 ++++++++++-
 drivers/nvme/host/fabrics.h |   3 +
 drivers/nvme/host/tcp.c     |   9 ++-
 include/linux/nvme.h        |   7 +++
 5 files changed, 148 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 72c0525c75f5..35a460cfb4bf 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -12,6 +12,7 @@
 #include "nvme.h"
 #include "fabrics.h"
 #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
 
 #define CHAP_BUF_SIZE 4096
 static struct kmem_cache *nvme_chap_buf_cache;
@@ -132,7 +133,12 @@ static int nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl,
 	data->auth_type = NVME_AUTH_COMMON_MESSAGES;
 	data->auth_id = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE;
 	data->t_id = cpu_to_le16(chap->transaction);
-	data->sc_c = 0; /* No secure channel concatenation */
+	if (!ctrl->opts->concat)
+		data->sc_c = NVME_AUTH_SECP_NOSC;
+	else if (ctrl->opts->tls_key)
+		data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
+	else
+		data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
 	data->napd = 1;
 	data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
 	data->auth_protocol[0].dhchap.halen = 3;
@@ -312,8 +318,9 @@ 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->ctrl_key) {
+	if (ctrl->ctrl_key)
 		chap->bi_directional = true;
+	if (ctrl->ctrl_key || ctrl->opts->concat) {
 		get_random_bytes(chap->c2, chap->hash_len);
 		data->cvalid = 1;
 		memcpy(data->rval + chap->hash_len, chap->c2,
@@ -323,7 +330,10 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
 	} else {
 		memset(chap->c2, 0, chap->hash_len);
 	}
-	chap->s2 = nvme_auth_get_seqnum();
+	if (ctrl->opts->concat)
+		chap->s2 = 0;
+	else
+		chap->s2 = nvme_auth_get_seqnum();
 	data->seqnum = cpu_to_le32(chap->s2);
 	if (chap->host_key_len) {
 		dev_dbg(ctrl->device, "%s: qid %d host public key %*ph\n",
@@ -678,6 +688,79 @@ static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap)
 		crypto_free_kpp(chap->dh_tfm);
 }
 
+static int nvme_auth_secure_concat(struct nvme_ctrl *ctrl,
+				   struct nvme_dhchap_queue_context *chap)
+{
+	u8 *psk, *digest, *tls_psk;
+	struct key *tls_key;
+	size_t psk_len;
+	int ret = 0;
+
+	if (!chap->sess_key) {
+		dev_warn(ctrl->device,
+			 "%s: qid %d no session key negotiated\n",
+			 __func__, chap->qid);
+		return -ENOKEY;
+	}
+
+	psk = nvme_auth_generate_psk(chap->hash_id, chap->sess_key,
+				     chap->sess_key_len,
+				     chap->c1, chap->c2,
+				     chap->hash_len, &psk_len);
+	if (IS_ERR(psk)) {
+		ret = PTR_ERR(psk);
+		dev_warn(ctrl->device,
+			 "%s: qid %d failed to generate PSK, error %d\n",
+			 __func__, chap->qid, ret);
+		return ret;
+	}
+	dev_dbg(ctrl->device,
+		  "%s: generated psk %*ph\n", __func__, (int)psk_len, psk);
+
+	digest = nvme_auth_generate_digest(chap->hash_id, psk, psk_len,
+					   ctrl->opts->subsysnqn,
+					   ctrl->opts->host->nqn);
+	if (IS_ERR(digest)) {
+		ret = PTR_ERR(digest);
+		dev_warn(ctrl->device,
+			 "%s: qid %d failed to generate digest, error %d\n",
+			 __func__, chap->qid, ret);
+		goto out_free_psk;
+	};
+	dev_dbg(ctrl->device, "%s: generated digest %s\n",
+		 __func__, digest);
+	tls_psk = nvme_auth_derive_tls_psk(chap->hash_id, psk, psk_len, digest);
+	if (IS_ERR(tls_psk)) {
+		ret = PTR_ERR(tls_psk);
+		dev_warn(ctrl->device,
+			 "%s: qid %d failed to derive TLS psk, error %d\n",
+			 __func__, chap->qid, ret);
+		goto out_free_digest;
+	};
+
+	tls_key = nvme_tls_psk_refresh(ctrl->opts->keyring, ctrl->opts->host->nqn,
+				       ctrl->opts->subsysnqn, chap->hash_id,
+				       true, tls_psk, psk_len, digest);
+	if (IS_ERR(tls_key)) {
+		ret = PTR_ERR(tls_key);
+		dev_warn(ctrl->device,
+			 "%s: qid %d failed to insert generated key, error %d\n",
+			 __func__, chap->qid, ret);
+		tls_key = NULL;
+		kfree_sensitive(tls_psk);
+	}
+	if (ctrl->opts->tls_key) {
+		key_revoke(ctrl->opts->tls_key);
+		key_put(ctrl->opts->tls_key);
+	}
+	ctrl->opts->tls_key = tls_key;
+out_free_digest:
+	kfree_sensitive(digest);
+out_free_psk:
+	kfree_sensitive(psk);
+	return ret;
+}
+
 static void nvme_queue_auth_work(struct work_struct *work)
 {
 	struct nvme_dhchap_queue_context *chap =
@@ -832,10 +915,21 @@ static void nvme_queue_auth_work(struct work_struct *work)
 		if (ret)
 			chap->error = ret;
 	}
-	if (!ret) {
+	if (ret)
+		goto fail2;
+	if (chap->qid || !ctrl->opts->concat) {
 		chap->error = 0;
 		return;
 	}
+	ret = nvme_auth_secure_concat(ctrl, chap);
+	if (ret) {
+		dev_warn(ctrl->device,
+			 "%s: qid %d failed to enable secure concatenation\n",
+			 __func__, chap->qid);
+		chap->error = ret;
+	} else
+		chap->error = 0;
+	return;
 
 fail2:
 	if (chap->status == 0)
@@ -913,6 +1007,12 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
 			 "qid 0: authentication failed\n");
 		return;
 	}
+	/*
+	* Only run authentication on the admin queue for
+	* secure concatenation
+	 */
+	if (ctrl->opts->concat)
+		return;
 
 	for (q = 1; q < ctrl->queue_count; q++) {
 		ret = nvme_auth_negotiate(ctrl, q);
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index c6feb6e1e05f..8140d8166c3e 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -461,10 +461,11 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
 	result = le32_to_cpu(res.u32);
 	ctrl->cntlid = result & 0xFFFF;
 	if (result & (NVME_CONNECT_AUTHREQ_ATR | NVME_CONNECT_AUTHREQ_ASCR)) {
-		/* Secure concatenation is not implemented */
-		if (result & NVME_CONNECT_AUTHREQ_ASCR) {
+		/* Check for secure concatenation */
+		if ((result & NVME_CONNECT_AUTHREQ_ASCR) &&
+		    !ctrl->opts->concat) {
 			dev_warn(ctrl->device,
-				 "qid 0: secure concatenation is not supported\n");
+				 "qid 0: secure concatenation is not enabled\n");
 			ret = NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
 			goto out_free_data;
 		}
@@ -678,6 +679,7 @@ static const match_table_t opt_tokens = {
 #endif
 #ifdef CONFIG_NVME_TCP_TLS
 	{ NVMF_OPT_TLS,			"tls"			},
+	{ NVMF_OPT_CONCAT,		"concat"		},
 #endif
 	{ NVMF_OPT_ERR,			NULL			}
 };
@@ -707,6 +709,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	opts->tls = false;
 	opts->tls_key = NULL;
 	opts->keyring = NULL;
+	opts->concat = false;
 
 	options = o = kstrdup(buf, GFP_KERNEL);
 	if (!options)
@@ -1025,6 +1028,14 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			}
 			opts->tls = true;
 			break;
+		case NVMF_OPT_CONCAT:
+			if (!IS_ENABLED(CONFIG_NVME_TCP_TLS)) {
+				pr_err("TLS is not supported\n");
+				ret = -EINVAL;
+				goto out;
+			}
+			opts->concat = true;
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);
@@ -1051,6 +1062,16 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			pr_warn("failfast tmo (%d) larger than controller loss tmo (%d)\n",
 				opts->fast_io_fail_tmo, ctrl_loss_tmo);
 	}
+	if (opts->concat && opts->tls) {
+		pr_err("Secure concatenation over TLS is not supported\n");
+		ret = -EINVAL;
+		goto out;
+	}
+	if (opts->concat && opts->tls_key) {
+		pr_err("Cannot specify a TLS key for secure concatenation\n");
+		ret = -EINVAL;
+		goto out;
+	}
 
 	opts->host = nvmf_host_add(hostnqn, &hostid);
 	if (IS_ERR(opts->host)) {
@@ -1306,6 +1327,10 @@ nvmf_create_ctrl(struct device *dev, const char *buf)
 		goto out_module_put;
 	}
 
+	/* Reset controller to start TLS */
+	if (opts->concat)
+		nvme_reset_ctrl(ctrl);
+
 	module_put(ops->module);
 	return ctrl;
 
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index fbaee5a7be19..b6b731935dbf 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -73,6 +73,7 @@ enum {
 	NVMF_OPT_TLS		= 1 << 25,
 	NVMF_OPT_KEYRING	= 1 << 26,
 	NVMF_OPT_TLS_KEY	= 1 << 27,
+	NVMF_OPT_CONCAT		= 1 << 28,
 };
 
 /**
@@ -108,6 +109,7 @@ enum {
  * @keyring:    Keyring to use for key lookups
  * @tls_key:    TLS key for encrypted connections (TCP)
  * @tls:        Start TLS encrypted connections (TCP)
+ * @concat:     Enabled Secure channel concatenation (TCP)
  * @disable_sqflow: disable controller sq flow control
  * @hdr_digest: generate/verify header digest (TCP)
  * @data_digest: generate/verify data digest (TCP)
@@ -137,6 +139,7 @@ struct nvmf_ctrl_options {
 	struct key		*keyring;
 	struct key		*tls_key;
 	bool			tls;
+	bool			concat;
 	bool			disable_sqflow;
 	bool			hdr_digest;
 	bool			data_digest;
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 642c0fc0941b..6c8ca08f2972 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -219,7 +219,7 @@ static inline bool nvme_tcp_tls_configured(struct nvme_ctrl *ctrl)
 	if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
 		return 0;
 
-	return ctrl->opts->tls;
+	return ctrl->opts->tls || ctrl->opts->concat;
 }
 
 static inline struct blk_mq_tags *nvme_tcp_tagset(struct nvme_tcp_queue *queue)
@@ -1932,7 +1932,7 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
 	if (nvme_tcp_tls_configured(ctrl)) {
 		if (ctrl->opts->tls_key)
 			pskid = key_serial(ctrl->opts->tls_key);
-		else {
+		else if (ctrl->opts->tls) {
 			pskid = nvme_tls_psk_default(ctrl->opts->keyring,
 						      ctrl->opts->host->nqn,
 						      ctrl->opts->subsysnqn);
@@ -2615,6 +2615,9 @@ static int nvme_tcp_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
 
 	len = nvmf_get_address(ctrl, buf, size);
 
+	if (ctrl->state != NVME_CTRL_LIVE)
+		return len;
+
 	mutex_lock(&queue->queue_lock);
 
 	if (!test_bit(NVME_TCP_Q_LIVE, &queue->flags))
@@ -2800,7 +2803,7 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
 			  NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
 			  NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
 			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
-			  NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
+			  NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
 	.create_ctrl	= nvme_tcp_create_ctrl,
 };
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 462c21e0e417..2342a0240b6f 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1671,6 +1671,13 @@ enum {
 	NVME_AUTH_DHGROUP_INVALID	= 0xff,
 };
 
+enum {
+	NVME_AUTH_SECP_NOSC		= 0x00,
+	NVME_AUTH_SECP_SC		= 0x01,
+	NVME_AUTH_SECP_NEWTLSPSK	= 0x02,
+	NVME_AUTH_SECP_REPLACETLSPSK	= 0x03,
+};
+
 union nvmf_auth_protocol {
 	struct nvmf_auth_dhchap_protocol_descriptor dhchap;
 };
-- 
2.35.3

[PATCH 11/13] nvme-tcp: combine reset and recovery

[hare]

From: Hannes Reinecke <hare@suse.de>

Call 'connect_work' from reset to reduce duplicate code.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/tcp.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 6c8ca08f2972..a46ab8370007 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2329,14 +2329,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
 		return;
 	}
 
-	if (nvme_tcp_setup_ctrl(ctrl, false))
-		goto out_fail;
-
-	return;
-
-out_fail:
-	++ctrl->nr_reconnects;
-	nvme_tcp_reconnect_or_remove(ctrl);
+	queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work, 0);
 }
 
 static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
-- 
2.35.3

[PATCH 12/13] nvme-tcp: reset after recovery for secure concatenation

[hare]

From: Hannes Reinecke <hare@suse.de>

With TP8018 a new key will be generated from the DH-HMAC-CHAP
protocol after reset or recovery, but we need to start over
to establish a new TLS connection with the new keys.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/tcp.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index a46ab8370007..2e504fd9f616 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1834,6 +1834,18 @@ static void nvme_tcp_stop_queue(struct nvme_ctrl *nctrl, int qid)
 	if (test_and_clear_bit(NVME_TCP_Q_LIVE, &queue->flags))
 		__nvme_tcp_stop_queue(queue);
 	mutex_unlock(&queue->queue_lock);
+	/*
+	 * If the TLS key has not been set
+	 * but the queue PSK serial is valid
+	 * we are in reset, and should invalidate
+	 * the PSK serial for this queue to ensure
+	 * TLS won't be started after reset.
+	 */
+	if (nvme_tcp_tls_enabled(queue) &&
+	    nctrl->opts->concat && !nctrl->opts->tls_key) {
+		dev_dbg(nctrl->device, "queue %d clear TLS PSK\n", qid);
+		queue->tls_key = 0;
+	}
 }
 
 static void nvme_tcp_setup_sock_ops(struct nvme_tcp_queue *queue)
@@ -2263,6 +2275,21 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
 	if (nvme_tcp_setup_ctrl(ctrl, false))
 		goto requeue;
 
+	/*
+	 * Secure concatenation works in two steps;
+	 * the first connection is not encrypted, and
+	 * authentication generates the new TLS PSK.
+	 * Then the connection needs to be reset,
+	 * the TLS needs to be started with the generated
+	 * TLS PSK.
+	 */
+	if (ctrl->opts->concat && ctrl->opts->tls_key &&
+	    !nvme_tcp_tls_enabled(&tcp_ctrl->queues[0])) {
+		dev_info(ctrl->device, "Reset to enable TLS with generated PSK\n");
+		nvme_reset_ctrl(ctrl);
+		return;
+	}
+
 	dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
 			ctrl->nr_reconnects);
 
@@ -2284,6 +2311,13 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
+	if (ctrl->opts->concat && ctrl->opts->tls_key) {
+		dev_dbg(ctrl->device, "Wipe generated TLS PSK %0x8\n",
+			key_serial(ctrl->opts->tls_key));
+		key_revoke(ctrl->opts->tls_key);
+		key_put(ctrl->opts->tls_key);
+		ctrl->opts->tls_key = NULL;
+	}
 	nvme_tcp_teardown_io_queues(ctrl, false);
 	/* unquiesce to fail fast pending requests */
 	nvme_unquiesce_io_queues(ctrl);
@@ -2320,6 +2354,14 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
 		container_of(work, struct nvme_ctrl, reset_work);
 
 	nvme_stop_ctrl(ctrl);
+	if (ctrl->opts->concat && ctrl->opts->tls_key &&
+	    to_tcp_ctrl(ctrl)->queues[0].tls_key) {
+		dev_dbg(ctrl->device, "Wipe generated TLS PSK %0x8\n",
+			key_serial(ctrl->opts->tls_key));
+		key_revoke(ctrl->opts->tls_key);
+		key_put(ctrl->opts->tls_key);
+		ctrl->opts->tls_key = NULL;
+	}
 	nvme_tcp_teardown_ctrl(ctrl, false);
 
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING)) {
-- 
2.35.3

[PATCH 13/13] nvmet-tcp: support secure channel concatenation

[hare]

From: Hannes Reinecke <hare@suse.de>

Evaluate the SC_C flag during DH-CHAP-HMAC negotiation and insert
the generated PSK once negotiation has finished.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/auth.c             | 73 +++++++++++++++++++++++++-
 drivers/nvme/target/fabrics-cmd-auth.c | 46 ++++++++++++++--
 drivers/nvme/target/fabrics-cmd.c      | 30 ++++++++---
 drivers/nvme/target/nvmet.h            | 30 ++++++++---
 drivers/nvme/target/tcp.c              | 27 ++++++++++
 5 files changed, 186 insertions(+), 20 deletions(-)

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 3ddbc3880cac..538c935f023b 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -15,6 +15,7 @@
 #include <linux/ctype.h>
 #include <linux/random.h>
 #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
 #include <asm/unaligned.h>
 
 #include "nvmet.h"
@@ -124,7 +125,7 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 	return ret;
 }
 
-int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
+int nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 {
 	int ret = 0;
 	struct nvmet_host_link *p;
@@ -151,6 +152,11 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 		goto out_unlock;
 	}
 
+	if (nvmet_queue_is_tls(req->sq)) {
+		pr_debug("host %s tls enabled\n", ctrl->hostnqn);
+		goto out_unlock;
+	}
+
 	ret = nvmet_setup_dhgroup(ctrl, host->dhchap_dhgroup_id);
 	if (ret < 0)
 		pr_warn("Failed to setup DH group");
@@ -222,6 +228,9 @@ int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 void nvmet_auth_sq_free(struct nvmet_sq *sq)
 {
 	cancel_delayed_work(&sq->auth_expired_work);
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	sq->tls_key = 0;
+#endif
 	kfree(sq->dhchap_c1);
 	sq->dhchap_c1 = NULL;
 	kfree(sq->dhchap_c2);
@@ -250,6 +259,13 @@ void nvmet_destroy_auth(struct nvmet_ctrl *ctrl)
 		nvme_auth_free_key(ctrl->ctrl_key);
 		ctrl->ctrl_key = NULL;
 	}
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	if (ctrl->tls_key) {
+		key_revoke(ctrl->tls_key);
+		key_put(ctrl->tls_key);
+		ctrl->tls_key = NULL;
+	}
+#endif
 }
 
 bool nvmet_check_auth_status(struct nvmet_req *req)
@@ -529,3 +545,58 @@ int nvmet_auth_ctrl_sesskey(struct nvmet_req *req,
 
 	return ret;
 }
+
+void nvmet_auth_insert_psk(struct nvmet_sq *sq)
+{
+	int hash_len = nvme_auth_hmac_hash_len(sq->ctrl->shash_id);
+	u8 *psk, *digest, *tls_psk;
+	size_t psk_len;
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	struct key *tls_key = NULL;
+#endif
+
+	psk = nvme_auth_generate_psk(sq->ctrl->shash_id,
+				     sq->dhchap_skey,
+				     sq->dhchap_skey_len,
+				     sq->dhchap_c1, sq->dhchap_c2,
+				     hash_len, &psk_len);
+	if (IS_ERR(psk)) {
+		pr_warn("%s: ctrl %d qid %d failed to generate PSK, error %ld\n",
+			__func__, sq->ctrl->cntlid, sq->qid, PTR_ERR(psk));
+		return;
+	}
+	digest = nvme_auth_generate_digest(sq->ctrl->shash_id, psk, psk_len,
+					   sq->ctrl->subsysnqn,
+					   sq->ctrl->hostnqn);
+	if (IS_ERR(digest)) {
+		pr_warn("%s: ctrl %d qid %d failed to generate digest, error %ld\n",
+			__func__, sq->ctrl->cntlid, sq->qid, PTR_ERR(digest));
+		goto out_free_psk;
+	}
+	tls_psk = nvme_auth_derive_tls_psk(sq->ctrl->shash_id, psk, psk_len, digest);
+	if (IS_ERR(tls_psk)) {
+		pr_warn("%s: ctrl %d qid %d failed to derive TLS PSK, error %ld\n",
+			__func__, sq->ctrl->cntlid, sq->qid, PTR_ERR(tls_psk));
+		goto out_free_digest;
+	}
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	tls_key = nvme_tls_psk_refresh(NULL, sq->ctrl->hostnqn, sq->ctrl->subsysnqn,
+				       sq->ctrl->shash_id, true, tls_psk, psk_len, digest);
+	if (IS_ERR(tls_key)) {
+		pr_warn("%s: ctrl %d qid %d failed to refresh key, error %ld\n",
+			__func__, sq->ctrl->cntlid, sq->qid, PTR_ERR(tls_key));
+		tls_key = NULL;
+		kfree_sensitive(tls_psk);
+	}
+
+	if (sq->ctrl->tls_key)
+		key_revoke(sq->ctrl->tls_key);
+	key_put(sq->ctrl->tls_key);
+	sq->ctrl->tls_key = tls_key;
+#endif
+
+out_free_digest:
+	kfree_sensitive(digest);
+out_free_psk:
+	kfree_sensitive(psk);
+}
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index eb7785be0ca7..fe7f4b3adb77 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -43,8 +43,26 @@ static u16 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
 		 data->auth_protocol[0].dhchap.halen,
 		 data->auth_protocol[0].dhchap.dhlen);
 	req->sq->dhchap_tid = le16_to_cpu(data->t_id);
-	if (data->sc_c)
-		return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+	if (data->sc_c != NVME_AUTH_SECP_NOSC) {
+		if (!IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
+			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+		/* Secure concatenation can only be enabled on the admin queue */
+		if (req->sq->qid)
+			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+		switch (data->sc_c) {
+		case NVME_AUTH_SECP_NEWTLSPSK:
+			if (nvmet_queue_is_tls(req->sq))
+				return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+			break;
+		case NVME_AUTH_SECP_REPLACETLSPSK:
+			if (!nvmet_queue_is_tls(req->sq))
+				return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+			break;
+		default:
+			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+		}
+		ctrl->concat = true;
+	}
 
 	if (data->napd != 1)
 		return NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE;
@@ -103,6 +121,13 @@ static u16 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
 			 nvme_auth_dhgroup_name(fallback_dhgid));
 		ctrl->dh_gid = fallback_dhgid;
 	}
+	if (ctrl->dh_gid == NVME_AUTH_DHGROUP_NULL &&
+	    ctrl->concat) {
+		pr_debug("%s: ctrl %d qid %d: NULL DH group invalid "
+			 "for secure channel concatenation\n", __func__,
+			 ctrl->cntlid, req->sq->qid);
+		return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+	}
 	pr_debug("%s: ctrl %d qid %d: selected DH group %s (%d)\n",
 		 __func__, ctrl->cntlid, req->sq->qid,
 		 nvme_auth_dhgroup_name(ctrl->dh_gid), ctrl->dh_gid);
@@ -154,6 +179,12 @@ static u16 nvmet_auth_reply(struct nvmet_req *req, void *d)
 	kfree(response);
 	pr_debug("%s: ctrl %d qid %d host authenticated\n",
 		 __func__, ctrl->cntlid, req->sq->qid);
+	if (!data->cvalid && ctrl->concat) {
+		pr_debug("%s: ctrl %d qid %d invalid challenge\n",
+			 __func__, ctrl->cntlid, req->sq->qid);
+		return NVME_AUTH_DHCHAP_FAILURE_FAILED;
+	}
+	req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
 	if (data->cvalid) {
 		req->sq->dhchap_c2 = kmemdup(data->rval + data->hl, data->hl,
 					     GFP_KERNEL);
@@ -163,11 +194,14 @@ static u16 nvmet_auth_reply(struct nvmet_req *req, void *d)
 		pr_debug("%s: ctrl %d qid %d challenge %*ph\n",
 			 __func__, ctrl->cntlid, req->sq->qid, data->hl,
 			 req->sq->dhchap_c2);
-	} else {
+	}
+	if (req->sq->dhchap_s2 == 0) {
+		if (ctrl->concat)
+			nvmet_auth_insert_psk(req->sq);
 		req->sq->authenticated = true;
+		kfree(req->sq->dhchap_c2);
 		req->sq->dhchap_c2 = NULL;
 	}
-	req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
 
 	return 0;
 }
@@ -240,7 +274,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 			pr_debug("%s: ctrl %d qid %d reset negotiation\n", __func__,
 				 ctrl->cntlid, req->sq->qid);
 			if (!req->sq->qid) {
-				if (nvmet_setup_auth(ctrl) < 0) {
+				if (nvmet_setup_auth(ctrl, req) < 0) {
 					status = NVME_SC_INTERNAL;
 					pr_err("ctrl %d qid 0 failed to setup"
 					       "re-authentication",
@@ -296,6 +330,8 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 		}
 		goto done_kfree;
 	case NVME_AUTH_DHCHAP_MESSAGE_SUCCESS2:
+		if (ctrl->concat)
+			nvmet_auth_insert_psk(req->sq);
 		req->sq->authenticated = true;
 		pr_debug("%s: ctrl %d qid %d ctrl authenticated\n",
 			 __func__, ctrl->cntlid, req->sq->qid);
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index d8da840a1c0e..d5a9e47ac14c 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -198,10 +198,23 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 	return ret;
 }
 
-static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl)
+static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 {
+	bool needs_auth = nvmet_has_auth(ctrl, req);
+
+	/*
+	 * For secure concatenation the DH-HMAC-CHAP negotiation
+	 * should only run on the admin queue; the I/O queues should
+	 * be using TLS with the generated PSK.
+	 */
+	if (ctrl->concat && req->sq->qid)
+		needs_auth = false;
+
+	pr_debug("%s: ctrl %d qid %d should %sauthenticate\n",
+		 __func__, ctrl->cntlid, req->sq->qid,
+		 needs_auth ? "" : "not ");
 	return (u32)ctrl->cntlid |
-		(nvmet_has_auth(ctrl) ? NVME_CONNECT_AUTHREQ_ATR : 0);
+		(needs_auth ? NVME_CONNECT_AUTHREQ_ATR : 0);
 }
 
 static void nvmet_execute_admin_connect(struct nvmet_req *req)
@@ -255,7 +268,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 
 	uuid_copy(&ctrl->hostid, &d->hostid);
 
-	ret = nvmet_setup_auth(ctrl);
+	ret = nvmet_setup_auth(ctrl, req);
 	if (ret < 0) {
 		pr_err("Failed to setup authentication, error %d\n", ret);
 		nvmet_ctrl_put(ctrl);
@@ -272,12 +285,13 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 		goto out;
 	}
 
-	pr_info("creating %s controller %d for subsystem %s for NQN %s%s%s.\n",
+	pr_info("creating %s controller %d for subsystem %s for NQN %s%s%s%s.\n",
 		nvmet_is_disc_subsys(ctrl->subsys) ? "discovery" : "nvm",
 		ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn,
-		ctrl->pi_support ? " T10-PI is enabled" : "",
-		nvmet_has_auth(ctrl) ? " with DH-HMAC-CHAP" : "");
-	req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl));
+		ctrl->pi_support ? ", T10-PI" : "",
+		nvmet_has_auth(ctrl, req) ? ", DH-HMAC-CHAP" : "",
+		nvmet_queue_is_tls(req->sq) ? ", TLS" : "");
+	req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl, req));
 out:
 	kfree(d);
 complete:
@@ -336,7 +350,7 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 		goto out_ctrl_put;
 
 	pr_debug("adding queue %d to ctrl %d.\n", qid, ctrl->cntlid);
-	req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl));
+	req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl, req));
 out:
 	kfree(d);
 complete:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6c8acebe1a1a..0b06ab3b6644 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -121,6 +121,9 @@ struct nvmet_sq {
 	u32			dhchap_s2;
 	u8			*dhchap_skey;
 	int			dhchap_skey_len;
+#endif
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	key_serial_t		tls_key;
 #endif
 	struct completion	free_done;
 	struct completion	confirm_done;
@@ -234,6 +237,7 @@ struct nvmet_ctrl {
 	u64			err_counter;
 	struct nvme_error_slot	slots[NVMET_ERROR_LOG_SLOTS];
 	bool			pi_support;
+	bool			concat;
 #ifdef CONFIG_NVME_TARGET_AUTH
 	struct nvme_dhchap_key	*host_key;
 	struct nvme_dhchap_key	*ctrl_key;
@@ -243,6 +247,9 @@ struct nvmet_ctrl {
 	u8			*dh_key;
 	size_t			dh_keysize;
 #endif
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	struct key		*tls_key;
+#endif
 };
 
 struct nvmet_subsys {
@@ -705,13 +712,21 @@ static inline void nvmet_req_bio_put(struct nvmet_req *req, struct bio *bio)
 		bio_put(bio);
 }
 
+#ifdef NVME_TARGET_TCP_TLS
+static inline bool nvmet_queue_is_tls(strucct nvmet_sq *sq)
+{
+	return sq->tls_key;
+}
+#else
+static inline bool nvmet_queue_is_tls(struct nvmet_sq *sq) { return false; }
+#endif
 #ifdef CONFIG_NVME_TARGET_AUTH
 void nvmet_execute_auth_send(struct nvmet_req *req);
 void nvmet_execute_auth_receive(struct nvmet_req *req);
 int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
 		       bool set_ctrl);
 int nvmet_auth_set_host_hash(struct nvmet_host *host, const char *hash);
-int nvmet_setup_auth(struct nvmet_ctrl *ctrl);
+int nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req);
 void nvmet_auth_sq_init(struct nvmet_sq *sq);
 void nvmet_destroy_auth(struct nvmet_ctrl *ctrl);
 void nvmet_auth_sq_free(struct nvmet_sq *sq);
@@ -721,16 +736,18 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
 			 unsigned int hash_len);
 int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
 			 unsigned int hash_len);
-static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl)
+static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 {
-	return ctrl->host_key != NULL;
+	return ctrl->host_key != NULL && !nvmet_queue_is_tls(req->sq);
 }
 int nvmet_auth_ctrl_exponential(struct nvmet_req *req,
 				u8 *buf, int buf_size);
 int nvmet_auth_ctrl_sesskey(struct nvmet_req *req,
 			    u8 *buf, int buf_size);
+void nvmet_auth_insert_psk(struct nvmet_sq *sq);
 #else
-static inline int nvmet_setup_auth(struct nvmet_ctrl *ctrl)
+static inline int nvmet_setup_auth(struct nvmet_ctrl *ctrl,
+				   struct nvmet_req *req)
 {
 	return 0;
 }
@@ -743,11 +760,12 @@ static inline bool nvmet_check_auth_status(struct nvmet_req *req)
 {
 	return true;
 }
-static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl)
+static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl,
+				  struct nvmet_req *req)
 {
 	return false;
 }
 static inline const char *nvmet_dhchap_dhgroup_name(u8 dhgid) { return NULL; }
+static inline void nvmet_auth_insert_psk(struct nvmet_sq *sq) {};
 #endif
-
 #endif /* _NVMET_H */
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 3d606ab01506..671c477f042c 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1812,6 +1812,33 @@ static void nvmet_tcp_tls_handshake_done(void *data, int status,
 	spin_unlock_bh(&queue->state_lock);
 
 	cancel_delayed_work_sync(&queue->tls_handshake_tmo_work);
+
+	if (!status) {
+		struct key *tls_key = key_lookup(peerid);
+
+		if (IS_ERR(tls_key)) {
+			pr_warn("%s: queue %d failed to lookup key %x\n",
+				__func__, queue->idx, peerid);
+			spin_lock_bh(&queue->state_lock);
+			queue->state = NVMET_TCP_Q_FAILED;
+			spin_unlock_bh(&queue->state_lock);
+			status = PTR_ERR(tls_key);
+		} else if (test_bit(KEY_FLAG_REVOKED, &tls_key->flags) ||
+			   test_bit(KEY_FLAG_INVALIDATED, &tls_key->flags)) {
+			pr_warn("%s: queue %d key %08x invalid\n",
+				__func__, queue->idx, peerid);
+			key_put(tls_key);
+			spin_lock_bh(&queue->state_lock);
+			queue->state = NVMET_TCP_Q_FAILED;
+			spin_unlock_bh(&queue->state_lock);
+			status = -EKEYREVOKED;
+		} else {
+			pr_debug("%s: queue %d using TLS PSK %x\n",
+				 __func__, queue->idx, peerid);
+			queue->nvme_sq.tls_key = peerid;
+			key_put(tls_key);
+		}
+	}
 	if (status)
 		nvmet_tcp_schedule_release_queue(queue);
 	else
-- 
2.35.3

Re: [PATCHv2 00/13] nvme: implement secure concatenation

[Hannes Reinecke]

On 1/27/24 17:30, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Hi all,
> 
> here's my attempt to implement secure concatenation for NVMe-of TCP
> as outlined in TP8018.
> Secure concatenation means that a TLS PSK is generated from the key
> material negotiated by the DH-HMAC-CHAP protocol, and the TLS PSK
> is then used for a subsequent TLS connection.
> The difference between the original definition of secure concatenation
> and the method outlined in TP8018 is that with TP8018 the connection
> is reset after DH-HMAC-CHAP negotiation, and a new connection is setup
> with the generated TLS PSK.
> 
> To implement that I have decided on resetting the connection from the
> nvme-tcp driver after the initial connection has been set up.
> Another way would have been to offload the connection reset to userspace,
> and let nvme-cli reset the connection. But that would be a modification
> to the userspace interface, and hence I didn't go that way.
> The drawback with this approach is that we'll create all I/O queues
> before resetting for TLS, even though these queues should never be used.
> But fixing that requires a larger rewrite of the TCP driver to unify the
> setup and reconnect paths. So keep it that way for now.
> 
> As usual, comments and reviews are welcome.
> 
> Changes to the original submission:
> - Sanitize TLS key handling
> - Fixup modconfig compilation
> 
Ping?

Anyone interested in giving feedback?

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), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich

Re: [PATCH 02/13] nvme: add nvme_auth_generate_psk()

[Sagi Grimberg]

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

Re: [PATCH 03/13] nvme: add nvme_auth_generate_digest()

[Sagi Grimberg]

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

Re: [PATCH 06/13] nvme-keyring: restrict match length for version '1' identifiers

[Sagi Grimberg]

On 27/01/2024 11:30, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> TP8018 changed the TLS PSK identifiers to append a PSK hash value,
> so to lookup any version '1' identifiers we need to restrict the
> match length to exclude the PSK hash value (which we don't have
> when looking up keys).
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
> index c16f9496643f..51b99b34e100 100644
> --- a/drivers/nvme/common/keyring.c
> +++ b/drivers/nvme/common/keyring.c
> @@ -44,6 +44,27 @@ static bool nvme_tls_psk_match(const struct key *key,
>   		return false;
>   	}
>   	match_id = match_data->raw_data;
> +	if (memcmp(match_id, "NVMe1", 5)) {
> +		char *e = (char *)match_id;
> +		size_t offset = 0;
> +		int n = 0;
> +
> +		while (*e != ' ' && offset < match_len) {
> +			if (*e == ' ') {
> +				n++;
> +				if (n == 3)
> +					break;
> +			}
> +			e++;
> +			offset++;
> +		}
> +		if (n != 3) {
> +			pr_debug("%s: error parsing '%s'\n",
> +				 __func__, match_id);
> +			return false;
> +		}
> +		match_len = offset;
> +	}

My eyes hurt... I am failing to understand what this even does...

You have a concrete example?

Re: [PATCH 07/13] nvme-tcp: check for invalidated or revoked key

[Sagi Grimberg]

On 27/01/2024 11:30, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> key_lookup() will always return a key, even if that key is revoked
> or invalidated. So check for invalid keys before continuing.

Is this a bug fix? it should go probably either as a standalone patch
or as the first in this set (if it is only relevant with this patchset).

Re: [PATCH 08/13] nvme-fabrics: authentication errors are not retryable

[Sagi Grimberg]

On 27/01/2024 11:30, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> Whenever authentication failed the error is non-retryable as the
> authentication solely depends on the passed parameters, which do
> not change during retry.

Does this directly relate to secure concatenation? because if it isn't 
we should
drop it from here.

Re: [PATCH 09/13] nvme-tcp: sanitize TLS key handling

[Sagi Grimberg]

On 27/01/2024 11:30, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> There is a difference between TLS configured (ie the user has
> provisioned/requested a key) and TLS enabled (ie the connection
> is encrypted with TLS).

When would the latter happen without the former?

> So to differentiate between those two states store the provisioned
> key in opts->tls_key (as we're using the same TLS key for all queues)
> and the key serial of the key negotiated by the TLS handshake
> in queue->tls_key.

Does nvmet generate a different key for each queue?

Re: [PATCH 11/13] nvme-tcp: combine reset and recovery

[Sagi Grimberg]

On 27/01/2024 11:30, hare@kernel.org wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> Call 'connect_work' from reset to reduce duplicate code.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/tcp.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 6c8ca08f2972..a46ab8370007 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2329,14 +2329,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
>   		return;
>   	}
>   
> -	if (nvme_tcp_setup_ctrl(ctrl, false))
> -		goto out_fail;
> -
> -	return;
> -
> -out_fail:
> -	++ctrl->nr_reconnects;
> -	nvme_tcp_reconnect_or_remove(ctrl);
> +	queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work, 0);
>   }

This breaks nvme_reset_ctrl_sync

>   
>   static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)

Re: [PATCH 06/13] nvme-keyring: restrict match length for version '1' identifiers

[Hannes Reinecke]

On 3/7/24 11:49, Sagi Grimberg wrote:
> 
> 
> On 27/01/2024 11:30, hare@kernel.org wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> TP8018 changed the TLS PSK identifiers to append a PSK hash value,
>> so to lookup any version '1' identifiers we need to restrict the
>> match length to exclude the PSK hash value (which we don't have
>> when looking up keys).
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/nvme/common/keyring.c 
>> b/drivers/nvme/common/keyring.c
>> index c16f9496643f..51b99b34e100 100644
>> --- a/drivers/nvme/common/keyring.c
>> +++ b/drivers/nvme/common/keyring.c
>> @@ -44,6 +44,27 @@ static bool nvme_tls_psk_match(const struct key *key,
>>           return false;
>>       }
>>       match_id = match_data->raw_data;
>> +    if (memcmp(match_id, "NVMe1", 5)) {
>> +        char *e = (char *)match_id;
>> +        size_t offset = 0;
>> +        int n = 0;
>> +
>> +        while (*e != ' ' && offset < match_len) {
>> +            if (*e == ' ') {
>> +                n++;
>> +                if (n == 3)
>> +                    break;
>> +            }
>> +            e++;
>> +            offset++;
>> +        }
>> +        if (n != 3) {
>> +            pr_debug("%s: error parsing '%s'\n",
>> +                 __func__, match_id);
>> +            return false;
>> +        }
>> +        match_len = offset;
>> +    }
> 
> My eyes hurt... I am failing to understand what this even does...
> 
> You have a concrete example?

Yeah, it's ugly. What I'm trying to do here is to lookup TLS PSK 
identities which are either in the original format:

NVMe0[RG]0[12] <hostnqn> <subsysnqn>

_or_ the TP8018 TLS PSK format:

NVMe1[RG]0[12] <hostnqn> <subsysnqn> <digest>

given that I only have <hostnqn> and <subsysnqn> as lookup keys.
So for the memcmp() I'll have to exclude the ' <digest>' bit, which
happens to the after the third blank space in the identity.
I'll have a slightly better version queued, but parsing remains ugly.

Cheers,

Hannes

Re: [PATCH 07/13] nvme-tcp: check for invalidated or revoked key

[Hannes Reinecke]

On 3/7/24 11:51, Sagi Grimberg wrote:
> 
> 
> On 27/01/2024 11:30, hare@kernel.org wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> key_lookup() will always return a key, even if that key is revoked
>> or invalidated. So check for invalid keys before continuing.
> 
> Is this a bug fix? it should go probably either as a standalone patch
> or as the first in this set (if it is only relevant with this patchset).

Possibly. But I've created them via code review, so I'm not sure how 
relevant it really is.

Cheers,

Hannes

Re: [PATCH 08/13] nvme-fabrics: authentication errors are not retryable

[Hannes Reinecke]

On 3/7/24 11:52, Sagi Grimberg wrote:
> 
> 
> On 27/01/2024 11:30, hare@kernel.org wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Whenever authentication failed the error is non-retryable as the
>> authentication solely depends on the passed parameters, which do
>> not change during retry.
> 
> Does this directly relate to secure concatenation? because if it isn't 
> we should drop it from here.

No, it does not. Meanwhile it went to the other patch series for 
authentication errors, and will be dropped from the next iteration.

Cheers,

Hannes

Re: [PATCH 09/13] nvme-tcp: sanitize TLS key handling

[Hannes Reinecke]

On 3/7/24 12:03, Sagi Grimberg wrote:
> 
> 
> On 27/01/2024 11:30, hare@kernel.org wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> There is a difference between TLS configured (ie the user has
>> provisioned/requested a key) and TLS enabled (ie the connection
>> is encrypted with TLS).
> 
> When would the latter happen without the former?
> 
No. Difference is that 'TLS configured' is a configuration setting
(ie the admin has specified --tls or --tls_key), and 'TLS enabled'
is the result once the queue has been established.

>> So to differentiate between those two states store the provisioned
>> key in opts->tls_key (as we're using the same TLS key for all queues)
>> and the key serial of the key negotiated by the TLS handshake
>> in queue->tls_key.
> 
> Does nvmet generate a different key for each queue?

No.

Cheers,

Hannes

Re: [PATCH 11/13] nvme-tcp: combine reset and recovery

[Hannes Reinecke]

On 3/7/24 12:08, Sagi Grimberg wrote:
> 
> 
> On 27/01/2024 11:30, hare@kernel.org wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Call 'connect_work' from reset to reduce duplicate code.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/tcp.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 6c8ca08f2972..a46ab8370007 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -2329,14 +2329,7 @@ static void nvme_reset_ctrl_work(struct 
>> work_struct *work)
>>           return;
>>       }
>> -    if (nvme_tcp_setup_ctrl(ctrl, false))
>> -        goto out_fail;
>> -
>> -    return;
>> -
>> -out_fail:
>> -    ++ctrl->nr_reconnects;
>> -    nvme_tcp_reconnect_or_remove(ctrl);
>> +    queue_delayed_work(nvme_wq, &to_tcp_ctrl(ctrl)->connect_work, 0);
>>   }
> 
> This breaks nvme_reset_ctrl_sync
> 
>>   static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
> 
Okay, let's drop it. I've got another patchset anyway reshuffling the
tcp init sequence.

Cheers,

Hannes

Re: [PATCH 06/13] nvme-keyring: restrict match length for version '1' identifiers

[Sagi Grimberg]

On 07/03/2024 13:35, Hannes Reinecke wrote:
> On 3/7/24 11:49, Sagi Grimberg wrote:
>>
>>
>> On 27/01/2024 11:30, hare@kernel.org wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> TP8018 changed the TLS PSK identifiers to append a PSK hash value,
>>> so to lookup any version '1' identifiers we need to restrict the
>>> match length to exclude the PSK hash value (which we don't have
>>> when looking up keys).
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
>>>   1 file changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/nvme/common/keyring.c 
>>> b/drivers/nvme/common/keyring.c
>>> index c16f9496643f..51b99b34e100 100644
>>> --- a/drivers/nvme/common/keyring.c
>>> +++ b/drivers/nvme/common/keyring.c
>>> @@ -44,6 +44,27 @@ static bool nvme_tls_psk_match(const struct key 
>>> *key,
>>>           return false;
>>>       }
>>>       match_id = match_data->raw_data;
>>> +    if (memcmp(match_id, "NVMe1", 5)) {
>>> +        char *e = (char *)match_id;
>>> +        size_t offset = 0;
>>> +        int n = 0;
>>> +
>>> +        while (*e != ' ' && offset < match_len) {
>>> +            if (*e == ' ') {
>>> +                n++;
>>> +                if (n == 3)
>>> +                    break;
>>> +            }
>>> +            e++;
>>> +            offset++;
>>> +        }
>>> +        if (n != 3) {
>>> +            pr_debug("%s: error parsing '%s'\n",
>>> +                 __func__, match_id);
>>> +            return false;
>>> +        }
>>> +        match_len = offset;
>>> +    }
>>
>> My eyes hurt... I am failing to understand what this even does...
>>
>> You have a concrete example?
>
> Yeah, it's ugly. What I'm trying to do here is to lookup TLS PSK 
> identities which are either in the original format:
>
> NVMe0[RG]0[12] <hostnqn> <subsysnqn>
>
> _or_ the TP8018 TLS PSK format:
>
> NVMe1[RG]0[12] <hostnqn> <subsysnqn> <digest>
>
> given that I only have <hostnqn> and <subsysnqn> as lookup keys.
> So for the memcmp() I'll have to exclude the ' <digest>' bit, which
> happens to the after the third blank space in the identity.
> I'll have a slightly better version queued, but parsing remains ugly.

Shouldn't strtok/strsep simplify what you are trying to do here?

Re: [PATCH 06/13] nvme-keyring: restrict match length for version '1' identifiers

[Hannes Reinecke]

On 3/7/24 13:08, Sagi Grimberg wrote:
> 
> 
> On 07/03/2024 13:35, Hannes Reinecke wrote:
>> On 3/7/24 11:49, Sagi Grimberg wrote:
>>>
>>>
>>> On 27/01/2024 11:30, hare@kernel.org wrote:
>>>> From: Hannes Reinecke <hare@suse.de>
>>>>
>>>> TP8018 changed the TLS PSK identifiers to append a PSK hash value,
>>>> so to lookup any version '1' identifiers we need to restrict the
>>>> match length to exclude the PSK hash value (which we don't have
>>>> when looking up keys).
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>>   drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
>>>>   1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/nvme/common/keyring.c 
>>>> b/drivers/nvme/common/keyring.c
>>>> index c16f9496643f..51b99b34e100 100644
>>>> --- a/drivers/nvme/common/keyring.c
>>>> +++ b/drivers/nvme/common/keyring.c
>>>> @@ -44,6 +44,27 @@ static bool nvme_tls_psk_match(const struct key 
>>>> *key,
>>>>           return false;
>>>>       }
>>>>       match_id = match_data->raw_data;
>>>> +    if (memcmp(match_id, "NVMe1", 5)) {
>>>> +        char *e = (char *)match_id;
>>>> +        size_t offset = 0;
>>>> +        int n = 0;
>>>> +
>>>> +        while (*e != ' ' && offset < match_len) {
>>>> +            if (*e == ' ') {
>>>> +                n++;
>>>> +                if (n == 3)
>>>> +                    break;
>>>> +            }
>>>> +            e++;
>>>> +            offset++;
>>>> +        }
>>>> +        if (n != 3) {
>>>> +            pr_debug("%s: error parsing '%s'\n",
>>>> +                 __func__, match_id);
>>>> +            return false;
>>>> +        }
>>>> +        match_len = offset;
>>>> +    }
>>>
>>> My eyes hurt... I am failing to understand what this even does...
>>>
>>> You have a concrete example?
>>
>> Yeah, it's ugly. What I'm trying to do here is to lookup TLS PSK 
>> identities which are either in the original format:
>>
>> NVMe0[RG]0[12] <hostnqn> <subsysnqn>
>>
>> _or_ the TP8018 TLS PSK format:
>>
>> NVMe1[RG]0[12] <hostnqn> <subsysnqn> <digest>
>>
>> given that I only have <hostnqn> and <subsysnqn> as lookup keys.
>> So for the memcmp() I'll have to exclude the ' <digest>' bit, which
>> happens to the after the third blank space in the identity.
>> I'll have a slightly better version queued, but parsing remains ugly.
> 
> Shouldn't strtok/strsep simplify what you are trying to do here?

Yeah. I'll be reworking that.

Cheers,

Hannes