Skip to contentSkip to conversation

nvme: implement secure concatenation

PATCH Superseded Hannes Reinecke 4 participants 27 messages Jan 22, 2025 – Feb 4, 2025

Subsystems (5)

v14 of 15 Superseded Latest is v15 →

lore
Share & tools
lore
#0 lore raw

v14 cover nvme: implement secure concatenation

Hi all,

here's my attempt to implement secure concatenation for NVMe-of TCP as outlined in TP8018.
The original (v5) patchset had been split in two, the first part of which has already been merged with nvme-6.11, and this is the second part which actually implements secure concatenation.

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 Sagi came up with the idea to directly reset the admin queue once the DH-CHAP negotiation has completed; that way it will be transparent to the upper layers and we don't have to worry about exposing queues which should not be used.

A blktest submission is in
https://github.com/osandov/blktests/pull/158
in case anyone want to run their own tests.

As usual, comments and reviews are welcome.

Patchset can be found at git.kernel.org:/pub/scm/linux/kernel/git/hare/nvme.git branch secure-concat.v14

Changes to v13:
- Fixup yet another kbuild robot failure
- Rework to apply on nvme-6.14

Changes to v12:
- Fixup kbuild robot failures
- Add missing return value

Changes to v11:
- Include reviews from Sagi

Changes to v10:
- Include reviews from Eric Biggers
- Drop test vectors for SHA1
- Add test vectors for SHA384 and SHA512
- Include reviews from Mark O'Donovan

Changes to v9:
- Include reviews from Eric Biggers
- Fixup secure concatenation after reset
- Rebased to nvme-6.12

Changes to v8:
- Include reviews from Eric Biggers
- Make hkdf a proper module
- Add testcases for hkdf

Changes to v7:
- Add patch to display nvme target TLS status in debugfs
- Include reviews from Sagi

Changes to v6:
- Rebase to nvme-6.11

Changes to v5:
- Include reviews from Sagi
- Split patchset in two parts

Changes to v4:
- Rework reset admin queue functionality based on an idea from Sagi (thanks!)
- kbuild robot fixes
- Fixup dhchap negotiation with non-empty C2 value

Changes to v3:
- Include reviews from Sagi
- Do not start I/O queues after DH-HMAC-CHAP negotiation
- Use bool to indicate TLS has been enabled on a queue
- Add 'tls_keyring' sysfs attribute
- Add 'tls_configured_key' sysfs attribute

Changes to v2:
- Fixup reset after dhchap negotiation
- Disable namespace scanning on I/O queues after
dhchap negotiation
- Reworked TLS key handling (again)

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

*** BLURB HERE ***

Hannes Reinecke (10):
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: always include <linux/key.h>
nvme-tcp: request secure channel concatenation
nvme-fabrics: reset admin connection for secure concatenation
nvmet-tcp: support secure channel concatenation
nvmet: add tls_concat and tls_key debugfs entries

 crypto/Kconfig                         |   6 +
 crypto/Makefile                        |   1 +
 crypto/hkdf.c                          | 573 +++++++++++++++++++++++++
 drivers/nvme/common/Kconfig            |   1 +
 drivers/nvme/common/auth.c             | 348 +++++++++++++++
 drivers/nvme/common/keyring.c          |  65 ++-
 drivers/nvme/host/Kconfig              |   2 +-
 drivers/nvme/host/auth.c               | 113 ++++-
 drivers/nvme/host/fabrics.c            |  34 +-
 drivers/nvme/host/fabrics.h            |   3 +
 drivers/nvme/host/nvme.h               |   2 +
 drivers/nvme/host/sysfs.c              |   4 +-
 drivers/nvme/host/tcp.c                |  68 ++-
 drivers/nvme/target/auth.c             |  72 +++-
 drivers/nvme/target/core.c             |   7 +-
 drivers/nvme/target/debugfs.c          |  27 ++
 drivers/nvme/target/fabrics-cmd-auth.c |  49 ++-
 drivers/nvme/target/fabrics-cmd.c      |  25 +-
 drivers/nvme/target/nvmet.h            |  38 +-
 drivers/nvme/target/tcp.c              |  24 +-
 fs/crypto/Kconfig                      |   1 +
 fs/crypto/hkdf.c                       |  85 +---
 include/crypto/hkdf.h                  |  20 +
 include/linux/nvme-auth.h              |   7 +
 include/linux/nvme-keyring.h           |  12 +-
 include/linux/nvme.h                   |   7 +
 26 files changed, 1484 insertions(+), 110 deletions(-)
 create mode 100644 crypto/hkdf.c
 create mode 100644 include/crypto/hkdf.h
Signature · 1 line
2.35.3
#1 lore raw

PATCH v14 1/10 crypto,fs: Separate out hkdf_extract() and hkdf_expand()

Separate out the HKDF functions into a separate module to to make them available to other callers.
And add a testsuite to the module with test vectors from RFC 5869 (and additional vectors for SHA384 and SHA512) to ensure the integrity of the algorithm.

---
 crypto/Kconfig        |   6 +
 crypto/Makefile       |   1 +
 crypto/hkdf.c         | 573 ++++++++++++++++++++++++++++++++++++++++++
 fs/crypto/Kconfig     |   1 +
 fs/crypto/hkdf.c      |  85 ++-----
 include/crypto/hkdf.h |  20 ++
 6 files changed, 616 insertions(+), 70 deletions(-)
 create mode 100644 crypto/hkdf.c
 create mode 100644 include/crypto/hkdf.h
crypto/Kconfig +6 -0
index 6b0bfbccac08..d7c244c7ab2f 100644
@@ -141,6 +141,12 @@ config CRYPTO_ACOMP
 	select CRYPTO_ALGAPI
 	select CRYPTO_ACOMP2
 
+config CRYPTO_HKDF
+	tristate
+	select CRYPTO_SHA256 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+	select CRYPTO_SHA512 if !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+	select CRYPTO_HASH2
+
 config CRYPTO_MANAGER
 	tristate "Cryptographic algorithm manager"
 	select CRYPTO_MANAGER2
crypto/Makefile +1 -0
index 77abca715445..05780c16a3e4 100644
@@ -34,6 +34,7 @@ obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
 obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
 obj-$(CONFIG_CRYPTO_SIG2) += sig.o
 obj-$(CONFIG_CRYPTO_KPP2) += kpp.o
+obj-$(CONFIG_CRYPTO_HKDF) += hkdf.o
 
 dh_generic-y := dh.o
 dh_generic-y += dh_helper.o
crypto/hkdf.c new file +573 -0

573 lines not on this page · raw message

fs/crypto/Kconfig +1 -0
index 5aff5934baa1..0ea56f588a4a 100644
@@ -3,6 +3,7 @@ config FS_ENCRYPTION
 	bool "FS Encryption (Per-file encryption)"
 	select CRYPTO
 	select CRYPTO_HASH
+	select CRYPTO_HKDF
 	select CRYPTO_SKCIPHER
 	select CRYPTO_LIB_SHA256
 	select KEYS
fs/crypto/hkdf.c +15 -70

124 lines not on this page · raw message

include/crypto/hkdf.h new file +20 -0
new file mode 100644
index 000000000000..6a9678f508f5
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * 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
+
+#include <crypto/hash.h>
+
+int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
+		 unsigned int ikmlen, const u8 *salt, unsigned int saltlen,
+		 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 v14 2/10 nvme: add nvme_auth_generate_psk()

In reply to Hannes Reinecke

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

---
 drivers/nvme/common/auth.c | 98 ++++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |  3 ++
 2 files changed, 101 insertions(+)
drivers/nvme/common/auth.c +98 -0
index 9b7126e1a19d..a63777190597 100644
@@ -11,6 +11,7 @@
 #include <linux/unaligned.h>
 #include <crypto/hash.h>
 #include <crypto/dh.h>
+#include <crypto/hkdf.h>
 #include <linux/nvme.h>
 #include <linux/nvme-auth.h>
 
@@ -471,5 +472,102 @@ int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key)
 }
 EXPORT_SYMBOL_GPL(nvme_auth_generate_key);
 
+/**
+ * nvme_auth_generate_psk - Generate a PSK for TLS
+ * @hmac_id: Hash function identifier
+ * @skey: Session key
+ * @skey_len: Length of @skey
+ * @c1: Value of challenge C1
+ * @c2: Value of challenge C2
+ * @hash_len: Hash length of the hash algorithm
+ * @ret_psk: Pointer too the resulting generated PSK
+ * @ret_len: length of @ret_psk
+ *
+ * Generate a PSK for TLS as specified in NVMe base specification, section 8.13.5.9:
+ *    Generated PSK for TLS
+ *
+ * The generated PSK for TLS shall be computed applying the HMAC function using the
+ * hash function H( ) selected by the HashID parameter in the DH-HMAC-CHAP_Challenge
+ * message with the session key KS as key to the concatenation of the two challenges
+ * C1 and C2 (i.e., generated PSK = HMAC(KS, C1 || C2)).
+ *
+ * Returns 0 on success with a valid generated PSK pointer in @ret_psk and the length
+ * of @ret_psk in @ret_len, or a negative error number otherwise.
+ */
+int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+		u8 *c1, u8 *c2, size_t hash_len, u8 **ret_psk,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 -EINVAL;
+	}
+
+	hmac_name = nvme_auth_hmac_name(hmac_id);
+	if (!hmac_name) {
+		pr_warn("%s: invalid hash algorithm %d\n",
+			__func__, hmac_id);
+		return -EINVAL;
+	}
+
+	tfm = crypto_alloc_shash(hmac_name, 0, 0);
+	if (IS_ERR(tfm))
+		return PTR_ERR(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_psk = psk;
+		*ret_len = psk_len;
+	}
+
+out_free_shash:
+	kfree_sensitive(shash);
+out_free_psk:
+	if (ret)
+		kfree_sensitive(psk);
+out_free_tfm:
+	crypto_free_shash(tfm);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_generate_psk);
+
 MODULE_DESCRIPTION("NVMe Authentication framework");
 MODULE_LICENSE("GPL v2");
include/linux/nvme-auth.h +3 -0
index c1d0bc5d9624..b13884b04dfd 100644
@@ -40,5 +40,8 @@ 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);
+int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+			   u8 *c1, u8 *c2, size_t hash_len,
+			   u8 **ret_psk, size_t *ret_len);
 
 #endif /* _NVME_AUTH_H */
Signature · 1 line
2.35.3
#3 lore raw

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

Quoted 1 line

+ * Generate a PSK for TLS as specified in NVMe base specification, section 8.13.5.9:

Please fix your comment to stay in 80 characters. The line wrapping here makes the text almost unreadable.

drivers/nvme/common/auth.c int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key) · line 498

Quoted 2 lines
drivers/nvme/common/auth.c +2 -0
+int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+		u8 *c1, u8 *c2, size_t hash_len, u8 **ret_psk,size_t *ret_len)

Missing whitespace before the last size_t.

drivers/nvme/common/auth.c int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key) · line 509

Quoted 11 lines
drivers/nvme/common/auth.c +11 -0
+{
+	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 -EINVAL;
+	}

No a very useful error message. Given that this is a violation of the API contract either a WARN_ON_ONCE or just letting the crypto algorithm fail should be fine.

Quoted 8 lines
drivers/nvme/common/auth.c +8 -0
+	tfm = crypto_alloc_shash(hmac_name, 0, 0);
+	if (IS_ERR(tfm))
+		return PTR_ERR(tfm);
+
+	psk_len = crypto_shash_digestsize(tfm);
+	psk = kzalloc(psk_len, GFP_KERNEL);
+	if (!psk) {
+		ret = -ENOMEM;

drivers/nvme/common/auth.c int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key) · line 535

Quoted 10 lines
drivers/nvme/common/auth.c +10 -0
+		goto out_free_tfm;
+	}
+
+	shash = kmalloc(sizeof(struct shash_desc) +
+			crypto_shash_descsize(tfm),
+			GFP_KERNEL);
+	if (!shash) {
+		ret = -ENOMEM;
+		goto out_free_psk;
+	}

Is there a good reason this can't use SHASH_DESC_ON_STACK()?

maintainer
#4 lore raw

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

Quoted 10 lines · Christoph Hellwig
Quoted 1 line

+ * Generate a PSK for TLS as specified in NVMe base specification, section 8.13.5.9:

Please fix your comment to stay in 80 characters. The line wrapping here makes the text almost unreadable.

drivers/nvme/common/auth.c int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key) · line 498

Quoted 2 lines
drivers/nvme/common/auth.c +2 -0
+int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+		u8 *c1, u8 *c2, size_t hash_len, u8 **ret_psk,size_t *ret_len)

Missing whitespace before the last size_t.

Quoted 16 lines

drivers/nvme/common/auth.c int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key) · line 509

Quoted 11 lines
drivers/nvme/common/auth.c +11 -0
+{
+	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 -EINVAL;
+	}

No a very useful error message. Given that this is a violation of the API contract either a WARN_ON_ONCE or just letting the crypto algorithm fail should be fine.

Okay, will just let it fail.

Quoted 12 lines
Quoted 12 lines
drivers/nvme/common/auth.c +12 -0
+	tfm = crypto_alloc_shash(hmac_name, 0, 0);
+	if (IS_ERR(tfm))
+		return PTR_ERR(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) +
Quoted 9 lines

drivers/nvme/common/auth.c int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key) · line 535

Quoted 6 lines
drivers/nvme/common/auth.c +6 -0
+			crypto_shash_descsize(tfm),
+			GFP_KERNEL);
+	if (!shash) {
+		ret = -ENOMEM;
+		goto out_free_psk;
+	}

Is there a good reason this can't use SHASH_DESC_ON_STACK()?

I'll check.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#5 lore raw

PATCH v14 3/10 nvme: add nvme_auth_generate_digest()

In reply to Hannes Reinecke

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

---
 drivers/nvme/common/auth.c | 139 +++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h  |   2 +
 2 files changed, 141 insertions(+)
drivers/nvme/common/auth.c +139 -0
index a63777190597..616465bc8e79 100644
@@ -569,5 +569,144 @@ int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 }
 EXPORT_SYMBOL_GPL(nvme_auth_generate_psk);
 
+/**
+ * nvme_auth_generate_digest - Generate TLS PSK digest
+ * @hmac_id: Hash function identifier
+ * @psk: Generated input PSK
+ * @psk_len: Lenght of @psk
+ * @subsysnqn: NQN of the subsystem
+ * @hostnqn: NQN of the host
+ * @ret_digest: Pointer to the returned digest
+ *
+ * Generate a TLS PSK digest as specified in TP8018 Section 3.6.1.3:
+ *   TLS PSK and PSK identity Derivation
+ *
+ * The PSK digest shall be computed by encoding in Base64 (refer to RFC 4648)
+ * the result of the application of the HMAC function using the hash function
+ * specified in item 4 above (ie the hash function of the cipher suite associated
+ * with the PSK identity) with the PSK as HMAC key to the concatenation of:
+ * - the NQN of the host (i.e., NQNh) not including the null terminator;
+ * - a space character;
+ * - the NQN of the NVM subsystem (i.e., NQNc) not including the null terminator;
+ * - a space character; and
+ * - the seventeen ASCII characters "NVMe-over-Fabrics"
+ * (i.e., <PSK digest> = Base64(HMAC(PSK, NQNh || " " || NQNc || " " || "NVMe-over-Fabrics"))).
+ * The length of the PSK digest depends on the hash function used to compute
+ * it as follows:
+ * - If the SHA-256 hash function is used, the resulting PSK digest is 44 characters long; or
+ * - If the SHA-384 hash function is used, the resulting PSK digest is 64 characters long.
+ *
+ * Returns 0 on success with a valid digest pointer in @ret_digest, or a negative
+ * error number on failure.
+ */
+int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+		char *subsysnqn, char *hostnqn, u8 **ret_digest)
+{
+	struct crypto_shash *tfm;
+	struct shash_desc *shash;
+	u8 *digest, *enc;
+	const char *hmac_name;
+	size_t digest_len, hmac_len;
+	int ret;
+
+	if (WARN_ON(!subsysnqn || !hostnqn))
+		return -EINVAL;
+
+	hmac_name = nvme_auth_hmac_name(hmac_id);
+	if (!hmac_name) {
+		pr_warn("%s: invalid hash algorithm %d\n",
+			__func__, hmac_id);
+		return -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 -EINVAL;
+	}
+
+	enc = kzalloc(hmac_len + 1, GFP_KERNEL);
+	if (!enc)
+		return -ENOMEM;
+
+	tfm = crypto_alloc_shash(hmac_name, 0, 0);
+	if (IS_ERR(tfm)) {
+		ret = PTR_ERR(tfm);
+		goto out_free_enc;
+	}
+
+	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, enc);
+	if (ret < hmac_len)
+		ret = -ENOKEY;
+	else {
+	    *ret_digest = enc;
+	    ret = 0;
+	}
+
+out_free_shash:
+	kfree_sensitive(shash);
+out_free_digest:
+	kfree_sensitive(digest);
+out_free_tfm:
+	crypto_free_shash(tfm);
+out_free_enc:
+	if (ret)
+		kfree_sensitive(enc);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_generate_digest);
+
 MODULE_DESCRIPTION("NVMe Authentication framework");
 MODULE_LICENSE("GPL v2");
include/linux/nvme-auth.h +2 -0
index b13884b04dfd..998f06bf10fd 100644
@@ -43,5 +43,7 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
 int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 			   u8 *c1, u8 *c2, size_t hash_len,
 			   u8 **ret_psk, size_t *ret_len);
+int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+		char *subsysnqn, char *hostnqn, u8 **ret_digest);
 
 #endif /* _NVME_AUTH_H */
Signature · 1 line
2.35.3
#6 lore raw

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

Quoted 1 line

+ * @psk_len: Lenght of @psk

Spelling: Lenght -> Length.

Quoted 5 lines

+ * specified in item 4 above (ie the hash function of the cipher suite associated
+ * with the PSK identity) with the PSK as HMAC key to the concatenation of:
+ * - the NQN of the host (i.e., NQNh) not including the null terminator;
+ * - a space character;
+ * - the NQN of the NVM subsystem (i.e., NQNc) not including the null terminator;

Same issue with overly long comment lines as in the last patch. (I won't bring this up another time if it shows up in the next patches)

drivers/nvme/common/auth.c int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len, · line 654

Quoted 3 lines
drivers/nvme/common/auth.c +3 -0
+	shash = kmalloc(sizeof(struct shash_desc) +
+			crypto_shash_descsize(tfm),
+			GFP_KERNEL);

SHASH_DESC_ON_STACK?

drivers/nvme/common/auth.c int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len, · line 695

Quoted 7 lines
drivers/nvme/common/auth.c +7 -0
+	ret = base64_encode(digest, digest_len, enc);
+	if (ret < hmac_len)
+		ret = -ENOKEY;
+	else {
+	    *ret_digest = enc;
+	    ret = 0;
+	}

Messed up formatting here. But this would normally be a:

	if (ret < hmac_len) {
		ret = -ENOKEY;
		goto out_free_shash;
	}

	*ret_digest = enc;
	ret = 0;
#7 lore raw

PATCH v14 4/10 nvme: add nvme_auth_derive_tls_psk()

In reply to Hannes Reinecke

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

---
 drivers/nvme/common/Kconfig |   1 +
 drivers/nvme/common/auth.c  | 111 ++++++++++++++++++++++++++++++++++++
 include/linux/nvme-auth.h   |   2 +
 3 files changed, 114 insertions(+)
drivers/nvme/common/Kconfig +1 -0
index 244432e0b73d..da963e4f3f1f 100644
@@ -12,3 +12,4 @@ config NVME_AUTH
 	select CRYPTO_SHA512
 	select CRYPTO_DH
 	select CRYPTO_DH_RFC7919_GROUPS
+	select CRYPTO_HKDF
drivers/nvme/common/auth.c +111 -0

122 lines not on this page · raw message

include/linux/nvme-auth.h +2 -0
index 998f06bf10fd..60e069a6757f 100644
@@ -45,5 +45,7 @@ int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 			   u8 **ret_psk, size_t *ret_len);
 int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
 		char *subsysnqn, char *hostnqn, u8 **ret_digest);
+int nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len,
+		u8 *psk_digest, u8 **ret_psk);
 
 #endif /* _NVME_AUTH_H */
Signature · 1 line
2.35.3
#8 lore raw

Re: [PATCH 04/10] nvme: add nvme_auth_derive_tls_psk()

drivers/nvme/common/auth.c int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len, · line 788

Quoted 1 line
drivers/nvme/common/auth.c +1 -0
+	info_len = strlen(psk_digest) + strlen(psk_prefix) + 5;

Where does the magic 5 come from?

drivers/nvme/common/auth.c int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len, · line 797

Quoted 10 lines
drivers/nvme/common/auth.c +9 -0
+	info = kzalloc(info_len, GFP_KERNEL);
+	if (!info) {
+		ret = -ENOMEM;
+		goto out_free_prk;
+	}
+
+	put_unaligned_be16(psk_len, info);
+	memcpy(info + 2, psk_prefix, strlen(psk_prefix));
+	sprintf(info + 2 + strlen(psk_prefix), "%02d %s", hmac_id, psk_digest);

And this feels like we'd want a basic struct for this, even if it just is the len field and a flexible array. Which will also help the compiler to to do bounds checking using __counted_by.

maintainer
#9 lore raw

Re: [PATCH 04/10] nvme: add nvme_auth_derive_tls_psk()

Quoted 4 lines · Christoph Hellwig

drivers/nvme/common/auth.c int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len, · line 788

Quoted 1 line
drivers/nvme/common/auth.c +1 -0
+	info_len = strlen(psk_digest) + strlen(psk_prefix) + 5;

Where does the magic 5 come from?

Two bytes for the HASH ID, two bytes for the length field from HKDF-Expand-Label, and one byte for the space separator.

Quoted 15 lines

drivers/nvme/common/auth.c int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len, · line 797

Quoted 10 lines
drivers/nvme/common/auth.c +9 -0
+	info = kzalloc(info_len, GFP_KERNEL);
+	if (!info) {
+		ret = -ENOMEM;
+		goto out_free_prk;
+	}
+
+	put_unaligned_be16(psk_len, info);
+	memcpy(info + 2, psk_prefix, strlen(psk_prefix));
+	sprintf(info + 2 + strlen(psk_prefix), "%02d %s", hmac_id, psk_digest);

And this feels like we'd want a basic struct for this, even if it just is the len field and a flexible array. Which will also help the compiler to to do bounds checking using __counted_by.

It would, if the were counting the full 'info' string.
But the 'length' field is just the length of the string 'tls13 ' + 'nvme-tls-psk' (cf the definition of HKDF-Expand-Label in RFC 8446). So having a _counted_by thingie would only count the first part
of the full 'info' string.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#10 lore raw

Re: [PATCH 04/10] nvme: add nvme_auth_derive_tls_psk()

Quoted 7 lines · Hannes Reinecke
Quoted 4 lines · Christoph Hellwig

drivers/nvme/common/auth.c int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len, · line 788

Quoted 1 line
drivers/nvme/common/auth.c +1 -0
+	info_len = strlen(psk_digest) + strlen(psk_prefix) + 5;

Where does the magic 5 come from?

Two bytes for the HASH ID, two bytes for the length field from HKDF-Expand-Label, and one byte for the space separator.

Well, that's information that needs to go here. If at all possible using a struct, but otherwise at least using a documented constant.

#11 lore raw

PATCH v14 5/10 nvme-keyring: add nvme_tls_psk_refresh()

In reply to Hannes Reinecke

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

---
 drivers/nvme/common/keyring.c | 64 +++++++++++++++++++++++++++++++++++
 include/linux/nvme-keyring.h  | 10 +++++-
 2 files changed, 73 insertions(+), 1 deletion(-)
drivers/nvme/common/keyring.c +64 -0
index ed5167f942d8..8cb253fcd586 100644
@@ -124,6 +124,70 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
 	return key_ref_to_ptr(keyref);
 }
 
+/**
+ * nvme_tls_psk_refresh - Refresh TLS PSK
+ * @keyring: Keyring holding the TLS PSK
+ * @hostnqn: Host NQN to use
+ * @subnqn: Subsystem NQN to use
+ * @hmac_id: Hash function identifier
+ * @data: TLS PSK key material
+ * @data_len: Length of @data
+ * @digest: TLS PSK digest
+ *
+ * Refresh a generated version 1 TLS PSK with the identity generated
+ * from @hmac_id, @hostnqn, @subnqn, and @digest in the keyring given
+ * by @keyring.
+ *
+ * Returns the updated key success or an error pointer otherwise.
+ */
+struct key *nvme_tls_psk_refresh(struct key *keyring,
+		const char *hostnqn, const char *subnqn, u8 hmac_id,
+		u8 *data, size_t data_len, const 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;
+	key_ref_t keyref;
+	key_serial_t keyring_id;
+	struct key *key;
+
+	if (!hostnqn || !subnqn || !data || !data_len)
+		return ERR_PTR(-EINVAL);
+
+	identity = kasprintf(GFP_KERNEL, "NVMe1G%02d %s %s %s",
+		 hmac_id, hostnqn, subnqn, digest);
+	if (!identity)
+		return ERR_PTR(-ENOMEM);
+
+	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 +9 -1
index 19d2b256180f..f7bbcbeb4b13 100644
@@ -8,13 +8,21 @@
 
 #if IS_ENABLED(CONFIG_NVME_KEYRING)
 
+struct key *nvme_tls_psk_refresh(struct key *keyring,
+		const char *hostnqn, const char *subnqn, u8 hmac_id,
+		u8 *data, size_t data_len, const char *digest);
 key_serial_t nvme_tls_psk_default(struct key *keyring,
 		const char *hostnqn, const char *subnqn);
 
 key_serial_t nvme_keyring_id(void);
 struct key *nvme_tls_key_lookup(key_serial_t key_id);
 #else
-
+static inline struct key *nvme_tls_psk_refresh(struct key *keyring,
+		const char *hostnqn, char *subnqn, u8 hmac_id,
+		u8 *data, size_t data_len, const char *digest)
+{
+	return ERR_PTR(-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
#12 lore raw

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

include/linux/nvme-keyring.h line 24

Quoted 5 lines · Hannes Reinecke
include/linux/nvme-keyring.h +5 -0
+static inline struct key *nvme_tls_psk_refresh(struct key *keyring,
+		const char *hostnqn, char *subnqn, u8 hmac_id,
+		u8 *data, size_t data_len, const char *digest)
+{
+	return ERR_PTR(-ENOTSUPP);

Is this supposed to be -EOPNOTSUPP?

Also the later added callers treat this an an error. Should this stub be reachable at all?

maintainer
#13 lore raw

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

Quoted 9 lines · Christoph Hellwig

include/linux/nvme-keyring.h line 24

Quoted 5 lines · Hannes Reinecke
include/linux/nvme-keyring.h +5 -0
+static inline struct key *nvme_tls_psk_refresh(struct key *keyring,
+		const char *hostnqn, char *subnqn, u8 hmac_id,
+		u8 *data, size_t data_len, const char *digest)
+{
+	return ERR_PTR(-ENOTSUPP);

Is this supposed to be -EOPNOTSUPP?

Possibly.

Quoted 3 lines

Also the later added callers treat this an an error. Should this stub be reachable at all?

Yes, and no.
The function is called unconditionally from drivers/nvme/host/tcp.c, so we need to provide the stub when nvme-keyring is not available. I could make it conditional depending on CONFIG_NVME_KEYRING, but then Sagi wanted to reduce the usage of conditional compilations. I don't mind.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#14 lore raw

PATCH v14 6/10 nvme: always include <linux/key.h>

In reply to Hannes Reinecke

To avoid build errors when NVME_KEYRING is not set.

---
 drivers/nvme/common/keyring.c | 1 -
 drivers/nvme/host/tcp.c       | 1 -
 drivers/nvme/target/tcp.c     | 1 -
 include/linux/nvme-keyring.h  | 2 ++
 4 files changed, 2 insertions(+), 3 deletions(-)
drivers/nvme/common/keyring.c +0 -1
index 8cb253fcd586..32d16c53133b 100644
@@ -5,7 +5,6 @@
 
 #include <linux/module.h>
 #include <linux/seq_file.h>
-#include <linux/key.h>
 #include <linux/key-type.h>
 #include <keys/user-type.h>
 #include <linux/nvme.h>
drivers/nvme/host/tcp.c +0 -1
index dc5bbca58c6d..68769449d595 100644
@@ -8,7 +8,6 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/err.h>
-#include <linux/key.h>
 #include <linux/nvme-tcp.h>
 #include <linux/nvme-keyring.h>
 #include <net/sock.h>
drivers/nvme/target/tcp.c +0 -1
index 7c51c2a8c109..fa59a7996efa 100644
@@ -8,7 +8,6 @@
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/err.h>
-#include <linux/key.h>
 #include <linux/nvme-tcp.h>
 #include <linux/nvme-keyring.h>
 #include <net/sock.h>
include/linux/nvme-keyring.h +2 -0
index f7bbcbeb4b13..ab8971afa973 100644
@@ -6,6 +6,8 @@
 #ifndef _NVME_KEYRING_H
 #define _NVME_KEYRING_H
 
+#include <linux/key.h>
+
 #if IS_ENABLED(CONFIG_NVME_KEYRING)
 
 struct key *nvme_tls_psk_refresh(struct key *keyring,
Signature · 1 line
2.35.3
#15 lore raw

Re: [PATCH 06/10] nvme: always include <linux/key.h>

Quoted 1 line · Hannes Reinecke

To avoid build errors when NVME_KEYRING is not set.

So do the previous patches created these errors? Or following ones? This commit message doesn't seem particularly useful. I'd expected it to be folded into the patch that would create the build failure.

maintainer
#16 lore raw

Re: [PATCH 06/10] nvme: always include <linux/key.h>

Quoted 8 lines · Christoph Hellwig
Quoted 1 line · Hannes Reinecke

To avoid build errors when NVME_KEYRING is not set.

So do the previous patches created these errors? Or following ones? This commit message doesn't seem particularly useful. I'd expected it to be folded into the patch that would create the build failure.

Okay.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#17 lore raw

PATCH v14 7/10 nvme-tcp: request secure channel concatenation

In reply to Hannes Reinecke

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 after reset.

---
 drivers/nvme/host/Kconfig   |   2 +-
 drivers/nvme/host/auth.c    | 113 +++++++++++++++++++++++++++++++++++-
 drivers/nvme/host/fabrics.c |  34 ++++++++++-
 drivers/nvme/host/fabrics.h |   3 +
 drivers/nvme/host/nvme.h    |   2 +
 drivers/nvme/host/sysfs.c   |   4 +-
 drivers/nvme/host/tcp.c     |  57 ++++++++++++++++--
 include/linux/nvme.h        |   7 +++
 8 files changed, 207 insertions(+), 15 deletions(-)
drivers/nvme/host/Kconfig +1 -1
index 486afe598184..10e453b2436e 100644
@@ -109,7 +109,7 @@ config NVME_HOST_AUTH
 	bool "NVMe over Fabrics In-Band Authentication in host side"
 	depends on NVME_CORE
 	select NVME_AUTH
-	select NVME_KEYRING if NVME_TCP_TLS
+	select NVME_KEYRING
 	help
 	  This provides support for NVMe over Fabrics In-Band Authentication in
 	  host side.
drivers/nvme/host/auth.c +110 -3

156 lines not on this page · raw message

drivers/nvme/host/fabrics.c +31 -3
index 432efcbf9e2f..93e9041b9657 100644
@@ -472,8 +472,9 @@ 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");
 			ret = -EOPNOTSUPP;
@@ -550,7 +551,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
 		/* Secure concatenation is not implemented */
 		if (result & NVME_CONNECT_AUTHREQ_ASCR) {
 			dev_warn(ctrl->device,
-				 "qid 0: secure concatenation is not supported\n");
+				 "qid %d: secure concatenation is not supported\n", qid);
 			ret = -EOPNOTSUPP;
 			goto out_free_data;
 		}
@@ -706,6 +707,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			}
 };
@@ -735,6 +737,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)
@@ -1053,6 +1056,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);
@@ -1079,6 +1090,23 @@ 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) {
+		if (opts->tls) {
+			pr_err("Secure concatenation over TLS is not supported\n");
+			ret = -EINVAL;
+			goto out;
+		}
+		if (opts->tls_key) {
+			pr_err("Cannot specify a TLS key for secure concatenation\n");
+			ret = -EINVAL;
+			goto out;
+		}
+		if (!opts->dhchap_secret) {
+			pr_err("Need to enable DH-CHAP for secure concatenation\n");
+			ret = -EINVAL;
+			goto out;
+		}
+	}
 
 	opts->host = nvmf_host_add(hostnqn, &hostid);
 	if (IS_ERR(opts->host)) {
drivers/nvme/host/fabrics.h +3 -0
index 21d75dc4a3a0..9cf5b020adba 100644
@@ -66,6 +66,7 @@ enum {
 	NVMF_OPT_TLS		= 1 << 25,
 	NVMF_OPT_KEYRING	= 1 << 26,
 	NVMF_OPT_TLS_KEY	= 1 << 27,
+	NVMF_OPT_CONCAT		= 1 << 28,
 };
 
 /**
@@ -101,6 +102,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)
@@ -130,6 +132,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/nvme.h +2 -0
index 2c76afd00390..2860f75d53d6 100644
@@ -1142,6 +1142,7 @@ void nvme_auth_stop(struct nvme_ctrl *ctrl);
 int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid);
 int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid);
 void nvme_auth_free(struct nvme_ctrl *ctrl);
+void nvme_auth_revoke_tls_key(struct nvme_ctrl *ctrl);
 #else
 static inline int nvme_auth_init_ctrl(struct nvme_ctrl *ctrl)
 {
@@ -1164,6 +1165,7 @@ static inline int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
 	return -EPROTONOSUPPORT;
 }
 static inline void nvme_auth_free(struct nvme_ctrl *ctrl) {};
+static inline void nvme_auth_revoke_tls_key(struct nvme_ctrl *ctrl) {};
 #endif
 
 u32 nvme_command_effects(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
drivers/nvme/host/sysfs.c +2 -2
index b68a9e5f1ea3..efb35eef1915 100644
@@ -780,10 +780,10 @@ static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,
 		return 0;
 
 	if (a == &dev_attr_tls_key.attr &&
-	    !ctrl->opts->tls)
+	    !ctrl->opts->tls && !ctrl->opts->concat)
 		return 0;
 	if (a == &dev_attr_tls_configured_key.attr &&
-	    !ctrl->opts->tls_key)
+	    (!ctrl->opts->tls_key || ctrl->opts->concat))
 		return 0;
 	if (a == &dev_attr_tls_keyring.attr &&
 	    !ctrl->opts->keyring)
drivers/nvme/host/tcp.c +51 -6

99 lines not on this page · raw message

include/linux/nvme.h +7 -0
index fe3b60818fdc..bfb5688363b0 100644
@@ -1746,6 +1746,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
#18 lore raw

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

Quoted 3 lines · Hannes Reinecke

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 after reset.

That's a very sparse commit message. What is the point of doing this? What is the spec reference for the implementation? What is the user interface? Why does this now always select NVME_KEYRING?

drivers/nvme/host/auth.c static int nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl, · line 140

Quoted 6 lines
drivers/nvme/host/auth.c +6 -0
+	if (!ctrl->opts->concat || chap->qid != 0)
+		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;

Took me a while to unwind this. Why not make this a little easier as:

	if (ctrl->opts->concat && chap->qid == 0) {
		if (ctrl->opts->tls_key)
			data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
		else
			data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
	} else {
		data->sc_c = NVME_AUTH_SECP_NOSC;
	}

?

drivers/nvme/host/auth.c static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap) · line 745

Quoted 1 line
drivers/nvme/host/auth.c +1 -0
+	ret = nvme_auth_derive_tls_psk(chap->hash_id, psk, psk_len, digest, &tls_psk);

Overly long line.

drivers/nvme/host/auth.c static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap) · line 753

Quoted 1 line
drivers/nvme/host/auth.c +1 -0
+	tls_key = nvme_tls_psk_refresh(ctrl->opts->keyring, ctrl->opts->host->nqn,

Same. Not going to mention more of that, but please fix it up.

drivers/nvme/host/auth.c static void nvme_queue_auth_work(struct work_struct *work) · line 931

Quoted 2 lines
drivers/nvme/host/auth.c +2 -0
+		if (ctrl->opts->concat &&
+		    (ret = nvme_auth_secure_concat(ctrl, chap))) {
		if (ctrl->opts->concat) {
			ret = nvme_auth_secure_concat(ctrl, chap);
			if (ret) {
				...

Review

Quoted 4 lines
drivers/nvme/host/auth.c +2 -0
+	/*
+	 * Only run authentication on the admin queue for

* secure concatenation

(quoted diff) +1 -0
+	 */

The two lines of comment can be folded in one, and it's missing a period at the end of the sentence.

drivers/nvme/host/tcp.c static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl) · line 2025

Quoted 4 lines
drivers/nvme/host/tcp.c +4 -0
+			/*
+			 * The generated PSK is stored in the
+			 * fabric options
+			 */

Missing period and not using up the line as well.

Quoted 12 lines

+/*
+ * The TLS key is set by secure concatenation after negotiation
+ * has been completed on the admin queue. We need to revoke the
+ * key when:
+ * - concatenation is enabled
+ * (otherwise it's a static key set by the user)
+ * and
+ * - the generated key is present in ctrl->tls_key
+ * (otherwise there's nothing to revoke)
+ * and
+ * - a valid PSK key ID has been set in ctrl->tls_pskid
+ * (otherwise TLS negotiation has not run).

Quoted 8 lines

+ *
+ * We cannot always revoke the key, as we're calling
+ * nvme_tcp_alloc_admin_queue() twice during secure
+ * concatenation, once on a 'normal' connection to run
+ * the DH-HMAC-CHAP negotiation (which generates the key,
+ * so it _must not_ be set), and once after the negotiation
+ * (which uses the key, so it _must_ be set).
+ */

Another overly dense fomatting. Are you trying to make up the extra width used in the first patches? :)

include/linux/nvme.h enum { · line 1754

Quoted 12 lines
include/linux/nvme.h +6 -0
@@ -1746,6 +1746,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,
+};

Comments please to explain what fields this applies to. Also we usuall try to split protocol definition additions into separate patches.

maintainer
#19 lore raw

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

Quoted 9 lines · Christoph Hellwig
Quoted 3 lines · Hannes Reinecke

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 after reset.

That's a very sparse commit message. What is the point of doing this? What is the spec reference for the implementation? What is the user interface? Why does this now always select NVME_KEYRING?

Okay, I'll add a spec reference and improve the description. And NVME_KEYRING is selected due to a kbuild failure for some odd combination of build-in vs modular in host and target.

Quoted 18 lines

drivers/nvme/host/auth.c static int nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl, · line 140

Quoted 6 lines
drivers/nvme/host/auth.c +6 -0
+	if (!ctrl->opts->concat || chap->qid != 0)
+		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;

Took me a while to unwind this. Why not make this a little easier as:

	if (ctrl->opts->concat && chap->qid == 0) {
		if (ctrl->opts->tls_key)
			data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
		else
			data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
	} else {
		data->sc_c = NVME_AUTH_SECP_NOSC;
	}
Quoted 2 lines

?

Okay.

[ .. ]

Quoted 6 lines
Quoted 6 lines
include/linux/nvme.h +0 -0
@@ -1746,6 +1746,13 @@ enum {
  	NVME_AUTH_DHGROUP_INVALID	= 0xff,
  };
 
Quoted 11 lines

include/linux/nvme.h enum { · line 1754

Quoted 6 lines
include/linux/nvme.h +6 -0
+enum {
+	NVME_AUTH_SECP_NOSC		= 0x00,
+	NVME_AUTH_SECP_SC		= 0x01,
+	NVME_AUTH_SECP_NEWTLSPSK	= 0x02,
+	NVME_AUTH_SECP_REPLACETLSPSK	= 0x03,
+};

Comments please to explain what fields this applies to. Also we usuall try to split protocol definition additions into separate patches.

Sure, I can split it into a separate patch.

And promise to fixup of formatting :-)

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
maintainer
#20 lore raw

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

In reply to Christoph Hellwig
Quoted 27 lines · Christoph Hellwig
Quoted 3 lines · Hannes Reinecke

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 after reset.

That's a very sparse commit message. What is the point of doing this? What is the spec reference for the implementation? What is the user interface? Why does this now always select NVME_KEYRING?

drivers/nvme/host/auth.c static int nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl, · line 140

Quoted 6 lines
drivers/nvme/host/auth.c +6 -0
+	if (!ctrl->opts->concat || chap->qid != 0)
+		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;

Took me a while to unwind this. Why not make this a little easier as:

	if (ctrl->opts->concat && chap->qid == 0) {
		if (ctrl->opts->tls_key)
			data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
		else
			data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
	} else {
		data->sc_c = NVME_AUTH_SECP_NOSC;
	}
Quoted 2 lines

?

Ok.

Quoted 8 lines

drivers/nvme/host/auth.c static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap) · line 745

Quoted 1 line
drivers/nvme/host/auth.c +1 -0
+	ret = nvme_auth_derive_tls_psk(chap->hash_id, psk, psk_len, digest, &tls_psk);

Overly long line.

drivers/nvme/host/auth.c static void nvme_auth_free_dhchap(struct nvme_dhchap_queue_context *chap) · line 753

Quoted 1 line
drivers/nvme/host/auth.c +1 -0
+	tls_key = nvme_tls_psk_refresh(ctrl->opts->keyring, ctrl->opts->host->nqn,

Same. Not going to mention more of that, but please fix it up.

Will do.

Quoted 8 lines

drivers/nvme/host/auth.c static void nvme_queue_auth_work(struct work_struct *work) · line 931

Quoted 2 lines
drivers/nvme/host/auth.c +2 -0
+		if (ctrl->opts->concat &&
+		    (ret = nvme_auth_secure_concat(ctrl, chap))) {
		if (ctrl->opts->concat) {
			ret = nvme_auth_secure_concat(ctrl, chap);
			if (ret) {
				...
Quoted 8 lines

Review

Quoted 4 lines
drivers/nvme/host/auth.c +2 -0
+	/*
+	 * Only run authentication on the admin queue for

* secure concatenation

(quoted diff) +1 -0
+	 */

The two lines of comment can be folded in one, and it's missing a period at the end of the sentence.

Will be fixing it up.

Quoted 7 lines

drivers/nvme/host/tcp.c static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl) · line 2025

Quoted 4 lines
drivers/nvme/host/tcp.c +4 -0
+			/*
+			 * The generated PSK is stored in the
+			 * fabric options
+			 */

Missing period and not using up the line as well.

Ditto.

Quoted 21 lines
Quoted 12 lines

+/*
+ * The TLS key is set by secure concatenation after negotiation
+ * has been completed on the admin queue. We need to revoke the
+ * key when:
+ * - concatenation is enabled
+ * (otherwise it's a static key set by the user)
+ * and
+ * - the generated key is present in ctrl->tls_key
+ * (otherwise there's nothing to revoke)
+ * and
+ * - a valid PSK key ID has been set in ctrl->tls_pskid
+ * (otherwise TLS negotiation has not run).

Quoted 8 lines

+ *
+ * We cannot always revoke the key, as we're calling
+ * nvme_tcp_alloc_admin_queue() twice during secure
+ * concatenation, once on a 'normal' connection to run
+ * the DH-HMAC-CHAP negotiation (which generates the key,
+ * so it _must not_ be set), and once after the negotiation
+ * (which uses the key, so it _must_ be set).
+ */

Quoted 3 lines

Another overly dense fomatting. Are you trying to make up the extra width used in the first patches? :)

Possibly. Will be fixing it up.

Quoted 6 lines
Quoted 6 lines
include/linux/nvme.h +0 -0
@@ -1746,6 +1746,13 @@ enum {
  	NVME_AUTH_DHGROUP_INVALID	= 0xff,
  };
 
Quoted 12 lines

include/linux/nvme.h enum { · line 1754

Quoted 6 lines
include/linux/nvme.h +6 -0
+enum {
+	NVME_AUTH_SECP_NOSC		= 0x00,
+	NVME_AUTH_SECP_SC		= 0x01,
+	NVME_AUTH_SECP_NEWTLSPSK	= 0x02,
+	NVME_AUTH_SECP_REPLACETLSPSK	= 0x03,
+};

Comments please to explain what fields this applies to. Also we usuall try to split protocol definition additions into separate patches.

Ok.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#21 lore raw

PATCH v14 8/10 nvme-fabrics: reset admin connection for secure concatenation

In reply to Hannes Reinecke

When secure concatenation is requested the connection needs to be reset to enable TLS encryption on the new cnnection. That implies that the original connection used for the DH-CHAP negotiation really shouldn't be used, and we should reset as soon as the DH-CHAP negotiation has succeeded on the admin queue.

Based on an idea from Sagi.

---
 drivers/nvme/host/tcp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)
drivers/nvme/host/tcp.c +10 -0
index 66899774805e..e9ff6babc540 100644
@@ -2293,6 +2293,16 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 	if (ret)
 		return ret;
 
+	if (ctrl->opts && ctrl->opts->concat && !ctrl->tls_pskid) {
+		/* See comments for nvme_tcp_key_revoke_needed() */
+		dev_dbg(ctrl->device, "restart admin queue for secure concatenation\n");
+		nvme_stop_keep_alive(ctrl);
+		nvme_tcp_teardown_admin_queue(ctrl, false);
+		ret = nvme_tcp_configure_admin_queue(ctrl, false);
+		if (ret)
+			return ret;
+	}
+
 	if (ctrl->icdoff) {
 		ret = -EOPNOTSUPP;
 		dev_err(ctrl->device, "icdoff is not supported!\n");
Signature · 1 line
2.35.3
#22 lore raw

PATCH v14 9/10 nvmet-tcp: support secure channel concatenation

In reply to Hannes Reinecke

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

---
 drivers/nvme/target/auth.c             | 72 +++++++++++++++++++++++++-
 drivers/nvme/target/core.c             |  7 +--
 drivers/nvme/target/fabrics-cmd-auth.c | 49 +++++++++++++++---
 drivers/nvme/target/fabrics-cmd.c      | 25 +++++++--
 drivers/nvme/target/nvmet.h            | 38 ++++++++++++--
 drivers/nvme/target/tcp.c              | 23 +++++++-
 6 files changed, 193 insertions(+), 21 deletions(-)
drivers/nvme/target/auth.c +71 -1
index b47d675232d2..0b0645ac5df4 100644
@@ -15,6 +15,7 @@
 #include <linux/ctype.h>
 #include <linux/random.h>
 #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
 #include <linux/unaligned.h>
 
 #include "nvmet.h"
@@ -139,7 +140,7 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
 	return ret;
 }
 
-u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl)
+u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq)
 {
 	int ret = 0;
 	struct nvmet_host_link *p;
@@ -165,6 +166,11 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl)
 		goto out_unlock;
 	}
 
+	if (nvmet_queue_tls_keyid(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");
@@ -233,6 +239,9 @@ u8 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);
@@ -261,6 +270,12 @@ 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_put(ctrl->tls_key);
+		ctrl->tls_key = NULL;
+	}
+#endif
 }
 
 bool nvmet_check_auth_status(struct nvmet_req *req)
@@ -542,3 +557,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;
+	int ret;
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	struct key *tls_key = NULL;
+#endif
+
+	ret = nvme_auth_generate_psk(sq->ctrl->shash_id,
+				     sq->dhchap_skey,
+				     sq->dhchap_skey_len,
+				     sq->dhchap_c1, sq->dhchap_c2,
+				     hash_len, &psk, &psk_len);
+	if (ret) {
+		pr_warn("%s: ctrl %d qid %d failed to generate PSK, error %d\n",
+			__func__, sq->ctrl->cntlid, sq->qid, ret);
+		return;
+	}
+	ret = nvme_auth_generate_digest(sq->ctrl->shash_id, psk, psk_len,
+					sq->ctrl->subsysnqn,
+					sq->ctrl->hostnqn, &digest);
+	if (ret) {
+		pr_warn("%s: ctrl %d qid %d failed to generate digest, error %d\n",
+			__func__, sq->ctrl->cntlid, sq->qid, ret);
+		goto out_free_psk;
+	}
+	ret = nvme_auth_derive_tls_psk(sq->ctrl->shash_id, psk, psk_len,
+				       digest, &tls_psk);
+	if (ret) {
+		pr_warn("%s: ctrl %d qid %d failed to derive TLS PSK, error %d\n",
+			__func__, sq->ctrl->cntlid, sq->qid, ret);
+		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, 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_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/core.c +4 -3
index 43c9888eea90..bc17ec4a783a 100644
@@ -1631,7 +1631,7 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 	if (args->hostid)
 		uuid_copy(&ctrl->hostid, args->hostid);
 
-	dhchap_status = nvmet_setup_auth(ctrl);
+	dhchap_status = nvmet_setup_auth(ctrl, args->sq);
 	if (dhchap_status) {
 		pr_err("Failed to setup authentication, dhchap status %u\n",
 		       dhchap_status);
@@ -1646,11 +1646,12 @@ struct nvmet_ctrl *nvmet_alloc_ctrl(struct nvmet_alloc_ctrl_args *args)
 
 	args->status = NVME_SC_SUCCESS;
 
-	pr_info("Created %s controller %d for subsystem %s for NQN %s%s%s.\n",
+	pr_info("Created %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" : "");
+		nvmet_has_auth(ctrl, args->sq) ? " with DH-HMAC-CHAP" : "",
+		nvmet_queue_tls_keyid(args->sq) ? ", TLS" : "");
 
 	return ctrl;
 
drivers/nvme/target/fabrics-cmd-auth.c +43 -6

87 lines not on this page · raw message

drivers/nvme/target/fabrics-cmd.c +21 -4
index a7ff05b3be29..9c01a4b6e543 100644
@@ -234,10 +234,26 @@ 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_sq *sq)
 {
+	bool needs_auth = nvmet_has_auth(ctrl, sq);
+	key_serial_t keyid = nvmet_queue_tls_keyid(sq);
+
+	/* Do not authenticate I/O queues for secure concatenation */
+	if (ctrl->concat && sq->qid)
+		needs_auth = false;
+
+	if (keyid)
+		pr_debug("%s: ctrl %d qid %d should %sauthenticate, tls psk %08x\n",
+			 __func__, ctrl->cntlid, sq->qid,
+			 needs_auth ? "" : "not ", keyid);
+	else
+		pr_debug("%s: ctrl %d qid %d should %sauthenticate%s\n",
+			 __func__, ctrl->cntlid, sq->qid,
+			 needs_auth ? "" : "not ",
+			 ctrl->concat ? ", secure concatenation" : "");
 	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)
@@ -247,6 +263,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	struct nvmet_ctrl *ctrl = NULL;
 	struct nvmet_alloc_ctrl_args args = {
 		.port = req->port,
+		.sq = req->sq,
 		.ops = req->ops,
 		.p2p_client = req->p2p_client,
 		.kato = le32_to_cpu(c->kato),
@@ -299,7 +316,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 		goto out;
 	}
 
-	args.result = cpu_to_le32(nvmet_connect_result(ctrl));
+	args.result = cpu_to_le32(nvmet_connect_result(ctrl, req->sq));
 out:
 	kfree(d);
 complete:
@@ -357,7 +374,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->sq));
 out:
 	kfree(d);
 complete:
drivers/nvme/target/nvmet.h +33 -5
index f4df458df9db..7401d59f26df 100644
@@ -157,6 +157,9 @@ struct nvmet_sq {
 	u32			dhchap_s2;
 	u8			*dhchap_skey;
 	int			dhchap_skey_len;
+#endif
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	struct key		*tls_key;
 #endif
 	struct completion	free_done;
 	struct completion	confirm_done;
@@ -282,6 +285,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;
@@ -290,6 +294,9 @@ struct nvmet_ctrl {
 	u8			dh_gid;
 	u8			*dh_key;
 	size_t			dh_keysize;
+#endif
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	struct key		*tls_key;
 #endif
 	struct nvmet_pr_log_mgr pr_log_mgr;
 };
@@ -576,6 +583,7 @@ void nvmet_update_cc(struct nvmet_ctrl *ctrl, u32 new);
 
 struct nvmet_alloc_ctrl_args {
 	struct nvmet_port	*port;
+	struct nvmet_sq		*sq;
 	char			*subsysnqn;
 	char			*hostnqn;
 	uuid_t			*hostid;
@@ -845,6 +853,22 @@ static inline void nvmet_req_bio_put(struct nvmet_req *req, struct bio *bio)
 		bio_put(bio);
 }
 
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static inline key_serial_t nvmet_queue_tls_keyid(struct nvmet_sq *sq)
+{
+	return sq->tls_key ? key_serial(sq->tls_key) : 0;
+}
+static inline void nvmet_sq_put_tls_key(struct nvmet_sq *sq)
+{
+	if (sq->tls_key) {
+		key_put(sq->tls_key);
+		sq->tls_key = NULL;
+	}
+}
+#else
+static inline key_serial_t nvmet_queue_tls_keyid(struct nvmet_sq *sq) { return 0; }
+static inline void nvmet_sq_put_tls_key(struct nvmet_sq *sq) {}
+#endif
 #ifdef CONFIG_NVME_TARGET_AUTH
 u32 nvmet_auth_send_data_len(struct nvmet_req *req);
 void nvmet_execute_auth_send(struct nvmet_req *req);
@@ -853,7 +877,7 @@ 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);
-u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl);
+u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_sq *sq);
 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);
@@ -863,16 +887,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_sq *sq)
 {
-	return ctrl->host_key != NULL;
+	return ctrl->host_key != NULL && !nvmet_queue_tls_keyid(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 u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl)
+static inline u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl,
+				  struct nvmet_sq *sq)
 {
 	return 0;
 }
@@ -885,11 +911,13 @@ 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_sq *sq)
 {
 	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
 
 int nvmet_pr_init_ns(struct nvmet_ns *ns);
drivers/nvme/target/tcp.c +21 -2
index fa59a7996efa..6d8d150a75c6 100644
@@ -1072,10 +1072,11 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
 
 	if (unlikely(!nvmet_req_init(req, &queue->nvme_cq,
 			&queue->nvme_sq, &nvmet_tcp_ops))) {
-		pr_err("failed cmd %p id %d opcode %d, data_len: %d\n",
+		pr_err("failed cmd %p id %d opcode %d, data_len: %d, status: %04x\n",
 			req->cmd, req->cmd->common.command_id,
 			req->cmd->common.opcode,
-			le32_to_cpu(req->cmd->common.dptr.sgl.length));
+		       le32_to_cpu(req->cmd->common.dptr.sgl.length),
+		       le16_to_cpu(req->cqe->status));
 
 		nvmet_tcp_handle_req_failure(queue, queue->cmd, req);
 		return 0;
@@ -1601,6 +1602,7 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
 	/* stop accepting incoming data */
 	queue->rcv_state = NVMET_TCP_RECV_ERR;
 
+	nvmet_sq_put_tls_key(&queue->nvme_sq);
 	nvmet_tcp_uninit_data_in_cmds(queue);
 	nvmet_sq_destroy(&queue->nvme_sq);
 	cancel_work_sync(&queue->io_work);
@@ -1806,6 +1808,23 @@ 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 = nvme_tls_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 {
+			pr_debug("%s: queue %d using TLS PSK %x\n",
+				 __func__, queue->idx, peerid);
+			queue->nvme_sq.tls_key = tls_key;
+		}
+	}
 	if (status)
 		nvmet_tcp_schedule_release_queue(queue);
 	else
Signature · 1 line
2.35.3
#23 lore raw

Re: [PATCH 09/10] nvmet-tcp: support secure channel concatenation

Quoted 2 lines · Hannes Reinecke

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

Same as for the host side, please write a much more detailed commit log.

drivers/nvme/target/fabrics-cmd-auth.c static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d) · line 125

Quoted 2 lines
drivers/nvme/target/fabrics-cmd-auth.c +2 -0
+	if (ctrl->dh_gid == NVME_AUTH_DHGROUP_NULL &&
+	    ctrl->concat) {

This conditional easily fits onto a single line.

drivers/nvme/target/fabrics-cmd.c static void nvmet_execute_admin_connect(struct nvmet_req *req) · line 266

Quoted 5 lines
drivers/nvme/target/fabrics-cmd.c +1 -0
@@ -247,6 +263,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	struct nvmet_ctrl *ctrl = NULL;
 	struct nvmet_alloc_ctrl_args args = {
 		.port = req->port,
+		.sq = req->sq,

So this now needs to pass a sq in alloc_ctrl_args? That needs proper explanation and really should be in a prep patch. Also please Cc Damien to make sure this doesn't break the nvme PCIe endpoint code.

Quoted 11 lines
drivers/nvme/target/tcp.c +6 -0
 	cancel_work_sync(&queue->io_work);
@@ -1806,6 +1808,23 @@ 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 = nvme_tls_key_lookup(peerid);
+
+		if (IS_ERR(tls_key)) {
+			pr_warn("%s: queue %d failed to lookup key %x\n",

drivers/nvme/target/tcp.c static void nvmet_tcp_tls_handshake_done(void *data, int status, · line 1826

Quoted 10 lines
drivers/nvme/target/tcp.c +10 -0
+				__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 {
+			pr_debug("%s: queue %d using TLS PSK %x\n",
+				 __func__, queue->idx, peerid);
+			queue->nvme_sq.tls_key = tls_key;
+		}

This is almost begging for a separate helper..

maintainer
#24 lore raw

Re: [PATCH 09/10] nvmet-tcp: support secure channel concatenation

Quoted 6 lines · Christoph Hellwig
Quoted 2 lines · Hannes Reinecke

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

Same as for the host side, please write a much more detailed commit log.

Ok.

Quoted 5 lines

drivers/nvme/target/fabrics-cmd-auth.c static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d) · line 125

Quoted 2 lines
drivers/nvme/target/fabrics-cmd-auth.c +2 -0
+	if (ctrl->dh_gid == NVME_AUTH_DHGROUP_NULL &&
+	    ctrl->concat) {

This conditional easily fits onto a single line.

Ok.

Quoted 10 lines

drivers/nvme/target/fabrics-cmd.c static void nvmet_execute_admin_connect(struct nvmet_req *req) · line 266

Quoted 5 lines
drivers/nvme/target/fabrics-cmd.c +1 -0
@@ -247,6 +263,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
  	struct nvmet_ctrl *ctrl = NULL;
  	struct nvmet_alloc_ctrl_args args = {
  		.port = req->port,
+		.sq = req->sq,

So this now needs to pass a sq in alloc_ctrl_args? That needs proper explanation and really should be in a prep patch. Also please Cc Damien to make sure this doesn't break the nvme PCIe endpoint code.

Sure, can do.

Quoted 15 lines
Quoted 15 lines
drivers/nvme/target/tcp.c +10 -0
  	cancel_work_sync(&queue->io_work);
@@ -1806,6 +1808,23 @@ 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 = nvme_tls_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);
Quoted 10 lines

drivers/nvme/target/tcp.c static void nvmet_tcp_tls_handshake_done(void *data, int status, · line 1826

Quoted 6 lines
drivers/nvme/target/tcp.c +6 -0
+			status = PTR_ERR(tls_key);
+		} else {
+			pr_debug("%s: queue %d using TLS PSK %x\n",
+				 __func__, queue->idx, peerid);
+			queue->nvme_sq.tls_key = tls_key;
+		}

This is almost begging for a separate helper..

Ok, will do.

Cheers,

Hannes

Signature · 4 lines
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
#25 lore raw

PATCH v14 10/10 nvmet: add tls_concat and tls_key debugfs entries

In reply to Hannes Reinecke

Add debugfs entries to display the 'concat' and 'tls_key' controller attributes.

---
 drivers/nvme/target/debugfs.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/nvme/target/debugfs.c +27 -0
index 220c7391fc19..e4300eb95101 100644
@@ -132,6 +132,27 @@ static int nvmet_ctrl_host_traddr_show(struct seq_file *m, void *p)
 }
 NVMET_DEBUGFS_ATTR(nvmet_ctrl_host_traddr);
 
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static int nvmet_ctrl_tls_key_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = m->private;
+	key_serial_t keyid = nvmet_queue_tls_keyid(ctrl->sqs[0]);
+
+	seq_printf(m, "%08x\n", keyid);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_tls_key);
+
+static int nvmet_ctrl_tls_concat_show(struct seq_file *m, void *p)
+{
+	struct nvmet_ctrl *ctrl = m->private;
+
+	seq_printf(m, "%d\n", ctrl->concat);
+	return 0;
+}
+NVMET_DEBUGFS_ATTR(nvmet_ctrl_tls_concat);
+#endif
+
 int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
 {
 	char name[32];
@@ -157,6 +178,12 @@ int nvmet_debugfs_ctrl_setup(struct nvmet_ctrl *ctrl)
 			    &nvmet_ctrl_state_fops);
 	debugfs_create_file("host_traddr", S_IRUSR, ctrl->debugfs_dir, ctrl,
 			    &nvmet_ctrl_host_traddr_fops);
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	debugfs_create_file("tls_concat", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_tls_concat_fops);
+	debugfs_create_file("tls_key", S_IRUSR, ctrl->debugfs_dir, ctrl,
+			    &nvmet_ctrl_tls_key_fops);
+#endif
 	return 0;
 }
 
Signature · 1 line
2.35.3
maintainer
#26 lore raw
In reply to Hannes Reinecke
Quoted 21 lines · Hannes Reinecke

Hi all,

here's my attempt to implement secure concatenation for NVMe-of TCP as outlined in TP8018.
The original (v5) patchset had been split in two, the first part of which has already been merged with nvme-6.11, and this is the second part which actually implements secure concatenation.

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 Sagi came up with the idea to directly reset the admin queue once the DH-CHAP negotiation has completed; that way it will be transparent to the upper layers and we don't have to worry about exposing queues which should not be used.

Quoted 5 lines

A blktest submission is in
https://github.com/osandov/blktests/pull/158
in case anyone want to run their own tests.

As usual, comments and reviews are welcome.

Looks good. already reviewed.