Skip to contentSkip to conversation

nvme: implement secure concatenation

PATCH Superseded [email protected] 3 participants 29 messages Jan 27, 2024 – Mar 7, 2024

Subsystems (5)

v2 of 15 Superseded Latest is v15 →

lore
Share & tools
lore
#0 lore raw

v2 cover nvme: implement secure concatenation

From: Hannes Reinecke <[email protected]>

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
Signature · 1 line
2.35.3
#1 lore raw

PATCH v2 1/13 crypto,fs: Separate out hkdf_extract() and hkdf_expand()

From: Hannes Reinecke <[email protected]>

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

---
 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
crypto/Makefile +1 -0
index 5ac6876f935a..282aefa1d74e 100644
@@ -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
crypto/hkdf.c new file +111 -0

111 lines not on this page · raw message

fs/crypto/hkdf.c +9 -59

96 lines not on this page · raw message

include/crypto/hkdf.h new file +18 -0
new file mode 100644
index 000000000000..bf06c080d7ed
@@ -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
Signature · 1 line
2.35.3
#2 lore raw

PATCH v2 2/13 nvme: add nvme_auth_generate_psk()

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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

---
 drivers/nvme/common/auth.c | 76 ++++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |  2 +
 2 files changed, 78 insertions(+)
drivers/nvme/common/auth.c +76 -0
index a23ab5c968b9..9ee459a9400e 100644
@@ -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");
include/linux/nvme-auth.h +2 -0
index c1d0bc5d9624..31dc1db2d4d6 100644
@@ -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 */
Signature · 1 line
2.35.3
#4 lore raw

PATCH v2 3/13 nvme: add nvme_auth_generate_digest()

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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

---
 drivers/nvme/common/auth.c | 105 +++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |   2 +
 2 files changed, 107 insertions(+)
drivers/nvme/common/auth.c +105 -0
index 9ee459a9400e..87e095fe3852 100644
@@ -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");
include/linux/nvme-auth.h +2 -0
index 31dc1db2d4d6..2cbb9249a8b3 100644
@@ -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 */
Signature · 1 line
2.35.3
#6 lore raw

PATCH v2 4/13 nvme: add nvme_auth_derive_tls_psk()

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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

---
 drivers/nvme/common/auth.c | 71 ++++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |  1 +
 2 files changed, 72 insertions(+)
drivers/nvme/common/auth.c +71 -0
index 87e095fe3852..20eb809929e2 100644
@@ -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");
include/linux/nvme-auth.h +1 -0
index 2cbb9249a8b3..335236fb2b73 100644
@@ -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 */
Signature · 1 line
2.35.3
#7 lore raw

PATCH v2 5/13 nvme-keyring: add nvme_tls_psk_refresh()

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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

---
 drivers/nvme/common/keyring.c | 50 +++++++++++++++++++++++++++++++++++
 include/linux/nvme-keyring.h  |  7 +++++
 2 files changed, 57 insertions(+)
drivers/nvme/common/keyring.c +50 -0
index a5c0431c101c..c16f9496643f 100644
@@ -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
  *
include/linux/nvme-keyring.h +7 -0
index e10333d78dbb..d2b8a0783ad7 100644
@@ -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)
 {
Signature · 1 line
2.35.3
#8 lore raw

PATCH v2 6/13 nvme-keyring: restrict match length for version '1' identifiers

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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).

---
 drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
drivers/nvme/common/keyring.c +21 -0
index c16f9496643f..51b99b34e100 100644
@@ -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);
Signature · 1 line
2.35.3
maintainer
#9 lore raw

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

Quoted 31 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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).

---
  drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)
drivers/nvme/common/keyring.c +11 -0
index c16f9496643f..51b99b34e100 100644
@@ -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;
+			}

drivers/nvme/common/keyring.c static bool nvme_tls_psk_match(const struct key *key, · line 67

Quoted 10 lines
drivers/nvme/common/keyring.c +10 -0
+			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?

maintainer
#10 lore raw

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

Quoted 47 lines · Sagi Grimberg
Quoted 31 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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).

---
  drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++

  1 file changed, 21 insertions(+)
drivers/nvme/common/keyring.c +11 -0
index c16f9496643f..51b99b34e100 100644
@@ -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;
+            }

drivers/nvme/common/keyring.c static bool nvme_tls_psk_match(const struct key *key, · line 67

Quoted 10 lines
drivers/nvme/common/keyring.c +10 -0
+            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...

Quoted 1 line

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

maintainer
#11 lore raw

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

Quoted 55 lines · Hannes Reinecke
Quoted 48 lines · Sagi Grimberg
Quoted 32 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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).

---
  drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++

  1 file changed, 21 insertions(+)
drivers/nvme/common/keyring.c +11 -0
index c16f9496643f..51b99b34e100 100644
@@ -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;
+            }

drivers/nvme/common/keyring.c static bool nvme_tls_psk_match(const struct key · line 68

Quoted 10 lines
drivers/nvme/common/keyring.c +10 -0
+            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...

Quoted 1 line

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>

Quoted 8 lines

_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?

maintainer
#12 lore raw

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

Quoted 60 lines · Sagi Grimberg
Quoted 57 lines · Hannes Reinecke
Quoted 48 lines · Sagi Grimberg
Quoted 32 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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).

---
  drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++

  1 file changed, 21 insertions(+)
drivers/nvme/common/keyring.c +11 -0
index c16f9496643f..51b99b34e100 100644
@@ -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;
+            }

drivers/nvme/common/keyring.c static bool nvme_tls_psk_match(const struct key · line 68

Quoted 10 lines
drivers/nvme/common/keyring.c +10 -0
+            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...

Quoted 1 line

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:

Quoted 8 lines
Quoted 6 lines

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

#13 lore raw

PATCH v2 7/13 nvme-tcp: check for invalidated or revoked key

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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

---
 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(-)
drivers/nvme/host/fabrics.c +6 -1
index aa88606a44c4..a7da088331dc 100644
@@ -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;
 }
drivers/nvme/host/sysfs.c +7 -2
index 754e91111042..1076b5b59b35 100644
@@ -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
drivers/nvme/host/tcp.c +7 -1
index c160b1a64ec0..65d9a817e752 100644
@@ -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;
Signature · 1 line
2.35.3
maintainer
#14 lore raw

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

Quoted 4 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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).

maintainer
#15 lore raw

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

Quoted 10 lines · Sagi Grimberg
Quoted 4 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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

#16 lore raw

PATCH v2 8/13 nvme-fabrics: authentication errors are not retryable

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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

---
 drivers/nvme/host/fabrics.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/nvme/host/fabrics.c +4 -4
index a7da088331dc..c6feb6e1e05f 100644
@@ -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)
Signature · 1 line
2.35.3
maintainer
#17 lore raw

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

Quoted 5 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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.

maintainer
#18 lore raw

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

Quoted 10 lines · Sagi Grimberg
Quoted 5 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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

#19 lore raw

PATCH v2 9/13 nvme-tcp: sanitize TLS key handling

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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.

---
 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(-)
drivers/nvme/host/core.c +0 -1
index 50818dbcfa1a..376efb310eb2 100644
@@ -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);
drivers/nvme/host/nvme.h +0 -1
index 6092cc361837..396e6e72b6a3 100644
@@ -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;
drivers/nvme/host/sysfs.c +1 -1
index 1076b5b59b35..dd18a118d053 100644
@@ -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;
drivers/nvme/host/tcp.c +24 -16
index 65d9a817e752..642c0fc0941b 100644
@@ -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;
 	}
Signature · 1 line
2.35.3
maintainer
#20 lore raw

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

Quoted 5 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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?

Quoted 4 lines

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?

maintainer
#21 lore raw

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

Quoted 11 lines · Sagi Grimberg
Quoted 5 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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.

Quoted 6 lines
Quoted 4 lines

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

#22 lore raw

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

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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.

---
 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(-)
drivers/nvme/host/auth.c +104 -4
index 72c0525c75f5..35a460cfb4bf 100644
@@ -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);
drivers/nvme/host/fabrics.c +28 -3
index c6feb6e1e05f..8140d8166c3e 100644
@@ -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;
 
drivers/nvme/host/fabrics.h +3 -0
index fbaee5a7be19..b6b731935dbf 100644
@@ -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;
drivers/nvme/host/tcp.c +6 -3
index 642c0fc0941b..6c8ca08f2972 100644
@@ -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,
 };
 
include/linux/nvme.h +7 -0
index 462c21e0e417..2342a0240b6f 100644
@@ -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;
 };
Signature · 1 line
2.35.3
#23 lore raw

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

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

Call 'connect_work' from reset to reduce duplicate code.

---
 drivers/nvme/host/tcp.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)
drivers/nvme/host/tcp.c +1 -8
index 6c8ca08f2972..a46ab8370007 100644
@@ -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)
Signature · 1 line
2.35.3
maintainer
#24 lore raw

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

Quoted 17 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

Call 'connect_work' from reset to reduce duplicate code.

---
  drivers/nvme/host/tcp.c | 9 +--------
  1 file changed, 1 insertion(+), 8 deletions(-)
drivers/nvme/host/tcp.c +0 -0
index 6c8ca08f2972..a46ab8370007 100644
@@ -2329,14 +2329,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
  		return;
  	}
 

drivers/nvme/host/tcp.c static void nvme_reset_ctrl_work(struct work_struct *work) · line 2333

Quoted 10 lines
drivers/nvme/host/tcp.c +1 -8
-	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

Quoted 2 lines
drivers/nvme/host/tcp.c +0 -0
  static void nvme_tcp_stop_ctrl(struct nvme_ctrl *ctrl)
maintainer
#25 lore raw

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

Quoted 24 lines · Sagi Grimberg
Quoted 21 lines · On 27/01/2024 11:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

Call 'connect_work' from reset to reduce duplicate code.

---
  drivers/nvme/host/tcp.c | 9 +--------

  1 file changed, 1 insertion(+), 8 deletions(-)
drivers/nvme/host/tcp.c +0 -4
index 6c8ca08f2972..a46ab8370007 100644
@@ -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;
Quoted 11 lines

drivers/nvme/host/tcp.c static void nvme_reset_ctrl_work(struct work_struct *work) · line 2333

Quoted 6 lines
drivers/nvme/host/tcp.c +1 -4
-
-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

Quoted 1 line
drivers/nvme/host/tcp.c +0 -0
   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

#26 lore raw

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

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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.

---
 drivers/nvme/host/tcp.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)
drivers/nvme/host/tcp.c +42 -0
index a46ab8370007..2e504fd9f616 100644
@@ -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)) {
Signature · 1 line
2.35.3
#27 lore raw

PATCH v2 13/13 nvmet-tcp: support secure channel concatenation

In reply to [email protected]

From: Hannes Reinecke <[email protected]>

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

---
 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(-)
drivers/nvme/target/auth.c +72 -1
index 3ddbc3880cac..538c935f023b 100644
@@ -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);
+}
drivers/nvme/target/fabrics-cmd-auth.c +41 -5

85 lines not on this page · raw message

drivers/nvme/target/fabrics-cmd.c +22 -8
index d8da840a1c0e..d5a9e47ac14c 100644
@@ -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:
drivers/nvme/target/nvmet.h +24 -6
index 6c8acebe1a1a..0b06ab3b6644 100644
@@ -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 */
drivers/nvme/target/tcp.c +27 -0
index 3d606ab01506..671c477f042c 100644
@@ -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
Signature · 1 line
2.35.3
maintainer
#28 lore raw
In reply to [email protected]
Quoted 24 lines · On 1/27/24 17:30, [email protected] wrote:

From: Hannes Reinecke <[email protected]>

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.

Quoted 6 lines

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

Signature · 5 lines
Dr. Hannes Reinecke                Kernel Storage Architect
[email protected]                              +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