[PATCHv3 00/17] nvme: implement secure concatenation

[PATCHv3 00/17] nvme: implement secure concatenation

[Hannes Reinecke]

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

Hannes Reinecke (17):
  nvme-keyring: restrict match length for version '1' identifiers
  nvme-tcp: check for invalidated or revoked key
  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-tcp: sanitize TLS key handling
  nvme-tcp: do not quiesce admin queue in nvme_tcp_teardown_io_queues()
  nvme: add a newline to the 'tls_key' sysfs attribute
  nvme-tcp: request secure channel concatenation
  nvme-fabrics: reset connection for secure concatenation
  nvme-tcp: reset after recovery for secure concatenation
  nvmet-auth: allow to clear DH-HMAC-CHAP keys
  nvme-target: do not check authentication status for admin commands
    twice
  nvme-target: do not check authentication status for I/O commands twice
  nvmet-tcp: support secure channel concatenation

 crypto/Makefile                        |   1 +
 crypto/hkdf.c                          | 112 +++++++++++
 drivers/nvme/common/auth.c             | 252 +++++++++++++++++++++++++
 drivers/nvme/common/keyring.c          |  84 ++++++++-
 drivers/nvme/host/auth.c               | 108 ++++++++++-
 drivers/nvme/host/core.c               |   9 +-
 drivers/nvme/host/fabrics.c            |  38 +++-
 drivers/nvme/host/fabrics.h            |   3 +
 drivers/nvme/host/sysfs.c              |  11 +-
 drivers/nvme/host/tcp.c                | 132 +++++++++++--
 drivers/nvme/target/admin-cmd.c        |   3 +-
 drivers/nvme/target/auth.c             |  82 +++++++-
 drivers/nvme/target/core.c             |   3 -
 drivers/nvme/target/fabrics-cmd-auth.c |  46 ++++-
 drivers/nvme/target/fabrics-cmd.c      |  29 ++-
 drivers/nvme/target/nvmet.h            |  30 ++-
 drivers/nvme/target/tcp.c              |  34 +++-
 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 +
 22 files changed, 958 insertions(+), 124 deletions(-)
 create mode 100644 crypto/hkdf.c
 create mode 100644 include/crypto/hkdf.h

-- 
2.35.3

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

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index 6f7e7a8fa5ae..2beac89b2246 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -36,14 +36,12 @@ static bool nvme_tls_psk_match(const struct key *key,
 		pr_debug("%s: no key description\n", __func__);
 		return false;
 	}
-	match_len = strlen(key->description);
-	pr_debug("%s: id %s len %zd\n", __func__, key->description, match_len);
-
 	if (!match_data->raw_data) {
 		pr_debug("%s: no match data\n", __func__);
 		return false;
 	}
 	match_id = match_data->raw_data;
+	match_len = strlen(match_id);
 	pr_debug("%s: match '%s' '%s' len %zd\n",
 		 __func__, match_id, key->description, match_len);
 	return !memcmp(key->description, match_id, match_len);
@@ -71,7 +69,7 @@ static struct key_type nvme_tls_psk_key_type = {
 
 static struct key *nvme_tls_psk_lookup(struct key *keyring,
 		const char *hostnqn, const char *subnqn,
-		int hmac, bool generated)
+		u8 hmac, u8 psk_ver, bool generated)
 {
 	char *identity;
 	size_t identity_len = (NVMF_NQN_SIZE) * 2 + 11;
@@ -79,11 +77,11 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
 	key_serial_t keyring_id;
 
 	identity = kzalloc(identity_len, GFP_KERNEL);
-	if (!identity)
+	if (WARN_ON(!identity))
 		return ERR_PTR(-ENOMEM);
 
-	snprintf(identity, identity_len, "NVMe0%c%02d %s %s",
-		 generated ? 'G' : 'R', hmac, hostnqn, subnqn);
+	snprintf(identity, identity_len, "NVMe%u%c%02u %s %s",
+		 psk_ver, generated ? 'G' : 'R', hmac, hostnqn, subnqn);
 
 	if (!keyring)
 		keyring = nvme_keyring;
@@ -109,19 +107,38 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
  *
  * 'Retained' PSKs (ie 'generated == false')
  * should be preferred to 'generated' PSKs,
+ * PSKs with hash (psk_ver 1) should be
+ * preferred to PSKs without (psk_ver 0),
  * and SHA-384 should be preferred to SHA-256.
  */
 static struct nvme_tls_psk_priority_list {
 	bool generated;
+	u8 psk_ver;
 	enum nvme_tcp_tls_cipher cipher;
 } nvme_tls_psk_prio[] = {
 	{ .generated = false,
+	  .psk_ver = 1,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
+	{ .generated = false,
+	  .psk_ver = 1,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
+	{ .generated = false,
+	  .psk_ver = 0,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
 	{ .generated = false,
+	  .psk_ver = 0,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
+	{ .generated = true,
+	  .psk_ver = 1,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
+	{ .generated = true,
+	  .psk_ver = 1,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
 	{ .generated = true,
+	  .psk_ver = 0,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
 	{ .generated = true,
+	  .psk_ver = 0,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
 };
 
@@ -137,10 +154,11 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 
 	for (prio = 0; prio < ARRAY_SIZE(nvme_tls_psk_prio); prio++) {
 		bool generated = nvme_tls_psk_prio[prio].generated;
+		u8 ver = nvme_tls_psk_prio[prio].psk_ver;
 		enum nvme_tcp_tls_cipher cipher = nvme_tls_psk_prio[prio].cipher;
 
 		tls_key = nvme_tls_psk_lookup(keyring, hostnqn, subnqn,
-					      cipher, generated);
+					      cipher, ver, generated);
 		if (!IS_ERR(tls_key)) {
 			tls_key_id = tls_key->serial;
 			key_put(tls_key);
-- 
2.35.3

[PATCH 02/17] nvme-tcp: check for invalidated or revoked key

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

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

Re: [PATCH 02/17] nvme-tcp: check for invalidated or revoked key

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> key_lookup() will always return a key, even if that key is revoked
> or invalidated. So check for invalid keys before continuing.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/fabrics.c | 7 ++++++-
>   drivers/nvme/host/sysfs.c   | 9 +++++++--
>   drivers/nvme/host/tcp.c     | 8 +++++++-
>   3 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 0141c0a6942f..75aa69457353 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -639,7 +639,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;

Looks like it will be useful to have a nvme_key_lookup() that
prints an error and returns a semantic error for revoked/invalidated key
and would be used in the call-sites ?

Re: [PATCH 02/17] nvme-tcp: check for invalidated or revoked key

[Hannes Reinecke]

On 4/7/24 22:51, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> key_lookup() will always return a key, even if that key is revoked
>> or invalidated. So check for invalid keys before continuing.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/fabrics.c | 7 ++++++-
>>   drivers/nvme/host/sysfs.c   | 9 +++++++--
>>   drivers/nvme/host/tcp.c     | 8 +++++++-
>>   3 files changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 0141c0a6942f..75aa69457353 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -639,7 +639,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;
> 
> Looks like it will be useful to have a nvme_key_lookup() that
> prints an error and returns a semantic error for revoked/invalidated key
> and would be used in the call-sites ?

Good idea. Will be updating the patch.

Cheers,

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

[PATCH 03/17] crypto,fs: Separate out hkdf_extract() and hkdf_expand()

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

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

[PATCH 04/17] nvme: add nvme_auth_generate_psk()

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index a3455f1d67fa..7a4b6589351d 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -11,6 +11,7 @@
 #include <asm/unaligned.h>
 #include <crypto/hash.h>
 #include <crypto/dh.h>
+#include <crypto/hkdf.h>
 #include <linux/nvme.h>
 #include <linux/nvme-auth.h>
 
@@ -471,5 +472,80 @@ 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_DESCRIPTION("NVMe Authentication framework");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index c1d0bc5d9624..31dc1db2d4d6 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -40,5 +40,7 @@ int nvme_auth_gen_pubkey(struct crypto_kpp *dh_tfm,
 int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
 				u8 *ctrl_key, size_t ctrl_key_len,
 				u8 *sess_key, size_t sess_key_len);
+u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
 
 #endif /* _NVME_AUTH_H */
-- 
2.35.3

Re: [PATCH 04/17] nvme: add nvme_auth_generate_psk()

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> Add a function to generate a NVMe PSK from the shared credentials
> negotiated by DH-HMAC-CHAP.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/common/auth.c | 76 ++++++++++++++++++++++++++++++++++++++
>   include/linux/nvme-auth.h  |  2 +
>   2 files changed, 78 insertions(+)
>
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index a3455f1d67fa..7a4b6589351d 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -11,6 +11,7 @@
>   #include <asm/unaligned.h>
>   #include <crypto/hash.h>
>   #include <crypto/dh.h>
> +#include <crypto/hkdf.h>
>   #include <linux/nvme.h>
>   #include <linux/nvme-auth.h>
>   
> @@ -471,5 +472,80 @@ 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)

It seems it would be simpler to have this psk and psk_len as out params and
return a normal status instead?

> +{
> +	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_DESCRIPTION("NVMe Authentication framework");
>   MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
> index c1d0bc5d9624..31dc1db2d4d6 100644
> --- a/include/linux/nvme-auth.h
> +++ b/include/linux/nvme-auth.h
> @@ -40,5 +40,7 @@ int nvme_auth_gen_pubkey(struct crypto_kpp *dh_tfm,
>   int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
>   				u8 *ctrl_key, size_t ctrl_key_len,
>   				u8 *sess_key, size_t sess_key_len);
> +u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
> +			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
>   
>   #endif /* _NVME_AUTH_H */

Re: [PATCH 04/17] nvme: add nvme_auth_generate_psk()

[Hannes Reinecke]

On 4/7/24 22:59, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Add a function to generate a NVMe PSK from the shared credentials
>> negotiated by DH-HMAC-CHAP.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>>   drivers/nvme/common/auth.c | 76 ++++++++++++++++++++++++++++++++++++++
>>   include/linux/nvme-auth.h  |  2 +
>>   2 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
>> index a3455f1d67fa..7a4b6589351d 100644
>> --- a/drivers/nvme/common/auth.c
>> +++ b/drivers/nvme/common/auth.c
>> @@ -11,6 +11,7 @@
>>   #include <asm/unaligned.h>
>>   #include <crypto/hash.h>
>>   #include <crypto/dh.h>
>> +#include <crypto/hkdf.h>
>>   #include <linux/nvme.h>
>>   #include <linux/nvme-auth.h>
>> @@ -471,5 +472,80 @@ 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)
> 
> It seems it would be simpler to have this psk and psk_len as out params and
> return a normal status instead?
> 
Tried to shorten the number of arguments; it's long enough already.
But sure, can do.

Cheers,

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

[PATCH 05/17] nvme: add nvme_auth_generate_digest()

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 7a4b6589351d..f56f08a4461e 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -547,5 +547,110 @@ 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_DESCRIPTION("NVMe Authentication framework");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index 31dc1db2d4d6..2cbb9249a8b3 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -42,5 +42,7 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
 				u8 *sess_key, size_t sess_key_len);
 u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
+u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+			      char *subsysnqn, char *hostnqn);
 
 #endif /* _NVME_AUTH_H */
-- 
2.35.3

Re: [PATCH 05/17] nvme: add nvme_auth_generate_digest()

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> Add a function to calculate the PSK digest as specified in TP8018.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> ---
>   drivers/nvme/common/auth.c | 105 +++++++++++++++++++++++++++++++++++++
>   include/linux/nvme-auth.h  |   2 +
>   2 files changed, 107 insertions(+)
>
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index 7a4b6589351d..f56f08a4461e 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -547,5 +547,110 @@ 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)

Same comment here.

> +{
> +	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_DESCRIPTION("NVMe Authentication framework");
>   MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
> index 31dc1db2d4d6..2cbb9249a8b3 100644
> --- a/include/linux/nvme-auth.h
> +++ b/include/linux/nvme-auth.h
> @@ -42,5 +42,7 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
>   				u8 *sess_key, size_t sess_key_len);
>   u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
>   			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
> +u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
> +			      char *subsysnqn, char *hostnqn);
>   
>   #endif /* _NVME_AUTH_H */

[PATCH 06/17] nvme: add nvme_auth_derive_tls_psk()

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index f56f08a4461e..90d525afc240 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -652,5 +652,76 @@ 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_DESCRIPTION("NVMe Authentication framework");
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index 2cbb9249a8b3..335236fb2b73 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -44,5 +44,6 @@ u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
 			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
 u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
 			      char *subsysnqn, char *hostnqn);
+u8 *nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len, u8 *psk_digest);
 
 #endif /* _NVME_AUTH_H */
-- 
2.35.3

Re: [PATCH 06/17] nvme: add nvme_auth_derive_tls_psk()

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> Add a function to derive the TLS PSK as specified TP8018.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/common/auth.c | 71 ++++++++++++++++++++++++++++++++++++++
>   include/linux/nvme-auth.h  |  1 +
>   2 files changed, 72 insertions(+)
>
> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
> index f56f08a4461e..90d525afc240 100644
> --- a/drivers/nvme/common/auth.c
> +++ b/drivers/nvme/common/auth.c
> @@ -652,5 +652,76 @@ 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)
> +{

Same comment here.
Can you please add a description of the derivation process in a function 
comment?
It is not trivial for me to follow.

> +	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);

What does prk stand for?

> +	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_DESCRIPTION("NVMe Authentication framework");
>   MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
> index 2cbb9249a8b3..335236fb2b73 100644
> --- a/include/linux/nvme-auth.h
> +++ b/include/linux/nvme-auth.h
> @@ -44,5 +44,6 @@ u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
>   			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
>   u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
>   			      char *subsysnqn, char *hostnqn);
> +u8 *nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len, u8 *psk_digest);
>   
>   #endif /* _NVME_AUTH_H */

Re: [PATCH 06/17] nvme: add nvme_auth_derive_tls_psk()

[Hannes Reinecke]

On 4/7/24 23:04, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Add a function to derive the TLS PSK as specified TP8018.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/common/auth.c | 71 ++++++++++++++++++++++++++++++++++++++
>>   include/linux/nvme-auth.h  |  1 +
>>   2 files changed, 72 insertions(+)
>>
>> diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
>> index f56f08a4461e..90d525afc240 100644
>> --- a/drivers/nvme/common/auth.c
>> +++ b/drivers/nvme/common/auth.c
>> @@ -652,5 +652,76 @@ 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)
>> +{
> 
> Same comment here.
> Can you please add a description of the derivation process in a function 
> comment?
> It is not trivial for me to follow.
> 
Sure, will do.

>> +    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);
> 
> What does prk stand for?
> 
That's from the spec. It'll be referred to in the function
description.

Cheers,

Hannes

[PATCH 07/17] nvme-keyring: add nvme_tls_psk_refresh()

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

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

[PATCH 08/17] nvme-tcp: sanitize TLS key handling

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

There is a difference between TLS configured (ie the user has
provisioned/requested a key) and TLS enabled (ie the connection
is encrypted with TLS). This becomes important for secure concatenation,
where the initial authentication is run unencrypted (ie with
TLS configured, but not enabled), and then the queue is reset to
run over TLS (ie TLS configured _and_ enabled).
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 only the key serial of the key negotiated by the TLS handshake
in queue->tls_pskid.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c  |  1 -
 drivers/nvme/host/sysfs.c |  2 +-
 drivers/nvme/host/tcp.c   | 54 +++++++++++++++++++++++++++++----------
 3 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 2baf5786a92f..9b601655f423 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4567,7 +4567,6 @@ static void nvme_free_ctrl(struct device *dev)
 
 	if (!subsys || ctrl->instance != subsys->instance)
 		ida_free(&nvme_instance_ida, ctrl->instance);
-	key_put(ctrl->tls_key);
 	nvme_free_cels(ctrl);
 	nvme_mpath_uninit(ctrl);
 	nvme_auth_stop(ctrl);
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index ec581608f16c..1f9e57fbfee3 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -671,7 +671,7 @@ static ssize_t tls_key_show(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-	struct key *key = ctrl->tls_key;
+	struct key *key = ctrl->opts->tls_key;
 
 	if (!key)
 		return 0;
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 4a58886e1354..7018dc0dd026 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -163,6 +163,7 @@ struct nvme_tcp_queue {
 	__le32			recv_ddgst;
 	struct completion       tls_complete;
 	int                     tls_err;
+	key_serial_t		tls_pskid;
 	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_pskid != 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,11 @@ 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_pskid = key_serial(tls_key);
+		if (qid == 0)
+			ctrl->ctrl.tls_key = tls_key;
+		else
+			key_put(tls_key);
 		queue->tls_err = 0;
 	}
 
@@ -1762,7 +1775,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;
@@ -1823,6 +1836,12 @@ static void nvme_tcp_stop_queue(struct nvme_ctrl *nctrl, int qid)
 	mutex_lock(&queue->queue_lock);
 	if (test_and_clear_bit(NVME_TCP_Q_LIVE, &queue->flags))
 		__nvme_tcp_stop_queue(queue);
+	/* Stopping the queue will disable TLS, so clear the PSK ID */
+	if (queue->tls_pskid) {
+		dev_dbg(nctrl->device, "queue %d clear TLS PSK %08x\n",
+			qid, queue->tls_pskid);
+		queue->tls_pskid = 0;
+	}
 	mutex_unlock(&queue->queue_lock);
 }
 
@@ -1919,16 +1938,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 +1969,17 @@ 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 = ctrl->tls_key ? key_serial(ctrl->tls_key) : 0;
 	int i, ret;
 
-	if (nvme_tcp_tls(ctrl) && !ctrl->tls_key) {
+	if (nvme_tcp_tls_configured(ctrl) && !pskid) {
 		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;
 	}
@@ -2138,6 +2160,12 @@ static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
 	if (remove)
 		nvme_unquiesce_admin_queue(ctrl);
 	nvme_tcp_destroy_admin_queue(ctrl, remove);
+	if (ctrl->tls_key) {
+		dev_dbg(ctrl->device, "Wipe negotiated TLS_PSK %08x\n",
+			key_serial(ctrl->tls_key));
+		key_put(ctrl->tls_key);
+		ctrl->tls_key = NULL;
+	}
 }
 
 static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
-- 
2.35.3

Re: [PATCH 08/17] nvme-tcp: sanitize TLS key handling

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> There is a difference between TLS configured (ie the user has
> provisioned/requested a key) and TLS enabled (ie the connection
> is encrypted with TLS). This becomes important for secure concatenation,
> where the initial authentication is run unencrypted (ie with
> TLS configured, but not enabled), and then the queue is reset to
> run over TLS (ie TLS configured _and_ enabled).

1. configured/enabled confuse (me at least) in this context.
2. What does "queue is reset" mean?

> 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 only the key serial of the key negotiated by the TLS handshake
> in queue->tls_pskid.

I have some recollection of asking about it, but can you explain why is
a tls_pskid now stored per-queue? What do we lose if tls_pskid moves from
nvme_tcp_queue to nvme_tcp_ctrl ?

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/core.c  |  1 -
>   drivers/nvme/host/sysfs.c |  2 +-
>   drivers/nvme/host/tcp.c   | 54 +++++++++++++++++++++++++++++----------
>   3 files changed, 42 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2baf5786a92f..9b601655f423 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4567,7 +4567,6 @@ static void nvme_free_ctrl(struct device *dev)
>   
>   	if (!subsys || ctrl->instance != subsys->instance)
>   		ida_free(&nvme_instance_ida, ctrl->instance);
> -	key_put(ctrl->tls_key);
>   	nvme_free_cels(ctrl);
>   	nvme_mpath_uninit(ctrl);
>   	nvme_auth_stop(ctrl);
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index ec581608f16c..1f9e57fbfee3 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -671,7 +671,7 @@ static ssize_t tls_key_show(struct device *dev,
>   			    struct device_attribute *attr, char *buf)
>   {
>   	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> -	struct key *key = ctrl->tls_key;
> +	struct key *key = ctrl->opts->tls_key;
>   
>   	if (!key)
>   		return 0;
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 4a58886e1354..7018dc0dd026 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -163,6 +163,7 @@ struct nvme_tcp_queue {
>   	__le32			recv_ddgst;
>   	struct completion       tls_complete;
>   	int                     tls_err;
> +	key_serial_t		tls_pskid;
>   	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_pskid != 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,11 @@ 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_pskid = key_serial(tls_key);
> +		if (qid == 0)
> +			ctrl->ctrl.tls_key = tls_key;
> +		else
> +			key_put(tls_key);
>   		queue->tls_err = 0;
>   	}
>   
> @@ -1762,7 +1775,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;
> @@ -1823,6 +1836,12 @@ static void nvme_tcp_stop_queue(struct nvme_ctrl *nctrl, int qid)
>   	mutex_lock(&queue->queue_lock);
>   	if (test_and_clear_bit(NVME_TCP_Q_LIVE, &queue->flags))
>   		__nvme_tcp_stop_queue(queue);
> +	/* Stopping the queue will disable TLS, so clear the PSK ID */
> +	if (queue->tls_pskid) {
> +		dev_dbg(nctrl->device, "queue %d clear TLS PSK %08x\n",
> +			qid, queue->tls_pskid);
> +		queue->tls_pskid = 0;
> +	}
>   	mutex_unlock(&queue->queue_lock);
>   }
>   
> @@ -1919,16 +1938,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 +1969,17 @@ 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 = ctrl->tls_key ? key_serial(ctrl->tls_key) : 0;
>   	int i, ret;
>   
> -	if (nvme_tcp_tls(ctrl) && !ctrl->tls_key) {
> +	if (nvme_tcp_tls_configured(ctrl) && !pskid) {
>   		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;
>   	}
> @@ -2138,6 +2160,12 @@ static void nvme_tcp_teardown_admin_queue(struct nvme_ctrl *ctrl,
>   	if (remove)
>   		nvme_unquiesce_admin_queue(ctrl);
>   	nvme_tcp_destroy_admin_queue(ctrl, remove);
> +	if (ctrl->tls_key) {
> +		dev_dbg(ctrl->device, "Wipe negotiated TLS_PSK %08x\n",
> +			key_serial(ctrl->tls_key));
> +		key_put(ctrl->tls_key);
> +		ctrl->tls_key = NULL;
> +	}
>   }
>   
>   static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,

Re: [PATCH 08/17] nvme-tcp: sanitize TLS key handling

[Hannes Reinecke]

On 4/7/24 23:15, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> There is a difference between TLS configured (ie the user has
>> provisioned/requested a key) and TLS enabled (ie the connection
>> is encrypted with TLS). This becomes important for secure concatenation,
>> where the initial authentication is run unencrypted (ie with
>> TLS configured, but not enabled), and then the queue is reset to
>> run over TLS (ie TLS configured _and_ enabled).
> 
> 1. configured/enabled confuse (me at least) in this context.
> 2. What does "queue is reset" mean?
> 
>> 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 only the key serial of the key negotiated by the TLS handshake
>> in queue->tls_pskid.
> 
> I have some recollection of asking about it, but can you explain why is
> a tls_pskid now stored per-queue? What do we lose if tls_pskid moves from
> nvme_tcp_queue to nvme_tcp_ctrl ?
> 
We need to have a distinction between 'the queue is TLS enabled' (ie TLS
handshake should be started on that queue) and 'the queue is tls 
encrypted' (ie TLS handshake has completed on that queue).
The former indeed can be derived from controller settings, and for the
latter we use the 'tls_pskid' setting.
Additionally it's the value for the tls_key sysfs attribute.
I could have used a per-queue flag, but using the tls_pskid was just
too easy here :-)

Cheers,

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

Re: [PATCH 08/17] nvme-tcp: sanitize TLS key handling

[Sagi Grimberg]

On 08/04/2024 9:48, Hannes Reinecke wrote:
> On 4/7/24 23:15, Sagi Grimberg wrote:
>>
>>
>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> There is a difference between TLS configured (ie the user has
>>> provisioned/requested a key) and TLS enabled (ie the connection
>>> is encrypted with TLS). This becomes important for secure 
>>> concatenation,
>>> where the initial authentication is run unencrypted (ie with
>>> TLS configured, but not enabled), and then the queue is reset to
>>> run over TLS (ie TLS configured _and_ enabled).
>>
>> 1. configured/enabled confuse (me at least) in this context.
>> 2. What does "queue is reset" mean?

Can you answer this?

>>
>>> 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 only the key serial of the key negotiated by the TLS handshake
>>> in queue->tls_pskid.
>>
>> I have some recollection of asking about it, but can you explain why is
>> a tls_pskid now stored per-queue? What do we lose if tls_pskid moves 
>> from
>> nvme_tcp_queue to nvme_tcp_ctrl ?
>>
> We need to have a distinction between 'the queue is TLS enabled' (ie TLS
> handshake should be started on that queue) and 'the queue is tls 
> encrypted' (ie TLS handshake has completed on that queue).
> The former indeed can be derived from controller settings, and for the
> latter we use the 'tls_pskid' setting.
> Additionally it's the value for the tls_key sysfs attribute.
> I could have used a per-queue flag, but using the tls_pskid was just
> too easy here :-)

I think a flag would be clearer, because a per-queue pskid suggest that 
it is
unique per queue while you concur that it isn't.

Re: [PATCH 08/17] nvme-tcp: sanitize TLS key handling

[Hannes Reinecke]

On 4/8/24 23:07, Sagi Grimberg wrote:
> 
> 
> On 08/04/2024 9:48, Hannes Reinecke wrote:
>> On 4/7/24 23:15, Sagi Grimberg wrote:
>>>
>>>
>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>> From: Hannes Reinecke <hare@suse.de>
>>>>
>>>> There is a difference between TLS configured (ie the user has
>>>> provisioned/requested a key) and TLS enabled (ie the connection
>>>> is encrypted with TLS). This becomes important for secure 
>>>> concatenation,
>>>> where the initial authentication is run unencrypted (ie with
>>>> TLS configured, but not enabled), and then the queue is reset to
>>>> run over TLS (ie TLS configured _and_ enabled).
>>>
>>> 1. configured/enabled confuse (me at least) in this context.
>>> 2. What does "queue is reset" mean?
> 
> Can you answer this?
> 
Sorry, I meant 'controller reset'.

>>>
>>>> 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 only the key serial of the key negotiated by the TLS handshake
>>>> in queue->tls_pskid.
>>>
>>> I have some recollection of asking about it, but can you explain why is
>>> a tls_pskid now stored per-queue? What do we lose if tls_pskid moves 
>>> from
>>> nvme_tcp_queue to nvme_tcp_ctrl ?
>>>
>> We need to have a distinction between 'the queue is TLS enabled' (ie TLS
>> handshake should be started on that queue) and 'the queue is tls 
>> encrypted' (ie TLS handshake has completed on that queue).
>> The former indeed can be derived from controller settings, and for the
>> latter we use the 'tls_pskid' setting.
>> Additionally it's the value for the tls_key sysfs attribute.
>> I could have used a per-queue flag, but using the tls_pskid was just
>> too easy here :-)
> 
> I think a flag would be clearer, because a per-queue pskid suggest that 
> it is unique per queue while you concur that it isn't.

Ok. Let's see how this works out.

Cheerc,

Hannes

[PATCH 09/17] nvme-tcp: do not quiesce admin queue in nvme_tcp_teardown_io_queues()

[Hannes Reinecke]

nvme_tcp_teardown_io_queues() is for I/O queues; the admin queue
should be left untouched here.

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

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 7018dc0dd026..66675b2dc197 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2173,7 +2173,6 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
 {
 	if (ctrl->queue_count <= 1)
 		return;
-	nvme_quiesce_admin_queue(ctrl);
 	nvme_quiesce_io_queues(ctrl);
 	nvme_sync_io_queues(ctrl);
 	nvme_tcp_stop_io_queues(ctrl);
-- 
2.35.3

Re: [PATCH 09/17] nvme-tcp: do not quiesce admin queue in nvme_tcp_teardown_io_queues()

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> nvme_tcp_teardown_io_queues() is for I/O queues; the admin queue
> should be left untouched here.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/tcp.c | 1 -
>   1 file changed, 1 deletion(-)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 7018dc0dd026..66675b2dc197 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2173,7 +2173,6 @@ static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
>   {
>   	if (ctrl->queue_count <= 1)
>   		return;
> -	nvme_quiesce_admin_queue(ctrl);
>   	nvme_quiesce_io_queues(ctrl);
>   	nvme_sync_io_queues(ctrl);
>   	nvme_tcp_stop_io_queues(ctrl);

1. can you explain why is this needed?
2. I have some vague recollection of this being added to address
something, but git blame leads to a patch that looks completely unrelated:
d4d61470ae48 ("nvme-tcp: serialize controller teardown sequences")

So I do not really object this (primarily because I cannot figure out why
this exists there in the first place).

Re: [PATCH 09/17] nvme-tcp: do not quiesce admin queue in nvme_tcp_teardown_io_queues()

[Hannes Reinecke]

On 4/7/24 23:24, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> nvme_tcp_teardown_io_queues() is for I/O queues; the admin queue
>> should be left untouched here.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/tcp.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 7018dc0dd026..66675b2dc197 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -2173,7 +2173,6 @@ static void nvme_tcp_teardown_io_queues(struct 
>> nvme_ctrl *ctrl,
>>   {
>>       if (ctrl->queue_count <= 1)
>>           return;
>> -    nvme_quiesce_admin_queue(ctrl);
>>       nvme_quiesce_io_queues(ctrl);
>>       nvme_sync_io_queues(ctrl);
>>       nvme_tcp_stop_io_queues(ctrl);
> 
> 1. can you explain why is this needed?
> 2. I have some vague recollection of this being added to address
> something, but git blame leads to a patch that looks completely unrelated:
> d4d61470ae48 ("nvme-tcp: serialize controller teardown sequences")
> 
> So I do not really object this (primarily because I cannot figure out why
> this exists there in the first place).
> 
You are right; this is unrelated. I'll leave it out for the next round.

Cheers,

Hannes

[PATCH 10/17] nvme: add a newline to the 'tls_key' sysfs attribute

[Hannes Reinecke]

Print a newline for easier userspace handling.

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

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 1f9e57fbfee3..07ec26de2d91 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -679,7 +679,7 @@ static ssize_t tls_key_show(struct device *dev,
 	    test_bit(KEY_FLAG_INVALIDATED, &key->flags))
 		return -EKEYREVOKED;
 
-	return sysfs_emit(buf, "%08x", key_serial(key));
+	return sysfs_emit(buf, "%08x\n", key_serial(key));
 }
 static DEVICE_ATTR_RO(tls_key);
 #endif
-- 
2.35.3

Re: [PATCH 10/17] nvme: add a newline to the 'tls_key' sysfs attribute

[Sagi Grimberg]

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

[PATCH 11/17] nvme-tcp: request secure channel concatenation

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/auth.c    | 108 ++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/fabrics.c |  25 ++++++++-
 drivers/nvme/host/fabrics.h |   3 +
 drivers/nvme/host/sysfs.c   |   2 +
 drivers/nvme/host/tcp.c     |  53 ++++++++++++++++--
 include/linux/nvme.h        |   7 +++
 6 files changed, 186 insertions(+), 12 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index a264b3ae078b..a4033a8f9607 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -12,6 +12,7 @@
 #include "nvme.h"
 #include "fabrics.h"
 #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
 
 #define CHAP_BUF_SIZE 4096
 static struct kmem_cache *nvme_chap_buf_cache;
@@ -131,7 +132,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 || 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;
 	data->napd = 1;
 	data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
 	data->auth_protocol[0].dhchap.halen = 3;
@@ -311,8 +317,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,
@@ -322,7 +329,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",
@@ -677,6 +687,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 =
@@ -831,10 +914,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)
@@ -912,6 +1006,12 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
 			 "qid 0: authentication failed\n");
 		return;
 	}
+	/*
+	* Only run authentication on the admin queue for
+	* secure concatenation
+	 */
+	if (ctrl->opts->concat)
+		return;
 
 	for (q = 1; q < ctrl->queue_count; q++) {
 		ret = nvme_auth_negotiate(ctrl, q);
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 75aa69457353..ae091e0e4ecf 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -463,8 +463,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 = NVME_SC_AUTH_REQUIRED;
@@ -682,6 +683,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			}
 };
@@ -711,6 +713,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)
@@ -1029,6 +1032,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);
@@ -1055,6 +1066,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)) {
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 06cc54851b1b..accfc7228366 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -73,6 +73,7 @@ enum {
 	NVMF_OPT_TLS		= 1 << 25,
 	NVMF_OPT_KEYRING	= 1 << 26,
 	NVMF_OPT_TLS_KEY	= 1 << 27,
+	NVMF_OPT_CONCAT		= 1 << 28,
 };
 
 /**
@@ -108,6 +109,7 @@ enum {
  * @keyring:    Keyring to use for key lookups
  * @tls_key:    TLS key for encrypted connections (TCP)
  * @tls:        Start TLS encrypted connections (TCP)
+ * @concat:     Enabled Secure channel concatenation (TCP)
  * @disable_sqflow: disable controller sq flow control
  * @hdr_digest: generate/verify header digest (TCP)
  * @data_digest: generate/verify data digest (TCP)
@@ -137,6 +139,7 @@ struct nvmf_ctrl_options {
 	struct key		*keyring;
 	struct key		*tls_key;
 	bool			tls;
+	bool			concat;
 	bool			disable_sqflow;
 	bool			hdr_digest;
 	bool			data_digest;
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 07ec26de2d91..edd96f501d8a 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -675,6 +675,8 @@ static ssize_t tls_key_show(struct device *dev,
 
 	if (!key)
 		return 0;
+	if (ctrl->opts->concat)
+		return 0;
 	if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
 	    test_bit(KEY_FLAG_INVALIDATED, &key->flags))
 		return -EKEYREVOKED;
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 66675b2dc197..94152ded123a 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -219,7 +219,7 @@ static inline bool nvme_tcp_tls_configured(struct nvme_ctrl *ctrl)
 	if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
 		return 0;
 
-	return ctrl->opts->tls;
+	return ctrl->opts->tls || ctrl->opts->concat;
 }
 
 static inline struct blk_mq_tags *nvme_tcp_tagset(struct nvme_tcp_queue *queue)
@@ -1941,7 +1941,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);
@@ -1973,12 +1973,28 @@ static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
 	key_serial_t pskid = ctrl->tls_key ? key_serial(ctrl->tls_key) : 0;
 	int i, ret;
 
-	if (nvme_tcp_tls_configured(ctrl) && !pskid) {
-		dev_err(ctrl->device, "no PSK negotiated\n");
-		return -ENOKEY;
+	if (nvme_tcp_tls_configured(ctrl)) {
+		if (ctrl->opts->concat) {
+			/*
+			 * The generated PSK is stored in the
+			 * fabric options
+			 */
+			if (!ctrl->opts->tls_key) {
+				dev_err(ctrl->device, "no PSK generated\n");
+				return -ENOKEY;
+			}
+			if (pskid && pskid != key_serial(ctrl->opts->tls_key)) {
+				dev_err(ctrl->device, "Stale PSK id %08x\n", pskid);
+				pskid = 0;
+			}
+		} else if (!pskid) {
+			dev_err(ctrl->device, "no PSK negotiated\n");
+			return -ENOKEY;
+		}
 	}
 
 	for (i = 1; i < ctrl->queue_count; i++) {
+		WARN_ON(nvme_tcp_tls_enabled(&tcp_ctrl->queues[i]));
 		ret = nvme_tcp_alloc_queue(ctrl, i, pskid);
 		if (ret)
 			goto out_free_queues;
@@ -2203,6 +2219,26 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
 	}
 }
 
+static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
+{
+	if (!ctrl->opts->concat)
+		return;
+	/* No key generated, nothing to do */
+	if (!ctrl->opts->tls_key)
+		return;
+	/*
+	 * Key generated, and TLS enabled:
+	 * Revoke the generated key.
+	 */
+	if (ctrl->tls_key) {
+		dev_dbg(ctrl->device, "Wipe generated TLS PSK %08x\n",
+			key_serial(ctrl->opts->tls_key));
+		key_revoke(ctrl->opts->tls_key);
+		key_put(ctrl->opts->tls_key);
+		ctrl->opts->tls_key = NULL;
+	}
+}
+
 static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 {
 	struct nvmf_ctrl_options *opts = ctrl->opts;
@@ -2304,6 +2340,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
 				struct nvme_tcp_ctrl, err_work);
 	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
 
+	nvme_tcp_revoke_generated_tls_key(ctrl);
 	nvme_stop_keep_alive(ctrl);
 	flush_work(&ctrl->async_event_work);
 	nvme_tcp_teardown_io_queues(ctrl, false);
@@ -2343,6 +2380,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
 	struct nvme_ctrl *ctrl =
 		container_of(work, struct nvme_ctrl, reset_work);
 
+	nvme_tcp_revoke_generated_tls_key(ctrl);
 	nvme_stop_ctrl(ctrl);
 	nvme_tcp_teardown_ctrl(ctrl, false);
 
@@ -2632,6 +2670,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))
@@ -2817,7 +2858,7 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
 			  NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
 			  NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
 			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
-			  NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
+			  NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
 	.create_ctrl	= nvme_tcp_create_ctrl,
 };
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index ce0c1143c7e4..b29310424ee1 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1663,6 +1663,13 @@ enum {
 	NVME_AUTH_DHGROUP_INVALID	= 0xff,
 };
 
+enum {
+	NVME_AUTH_SECP_NOSC		= 0x00,
+	NVME_AUTH_SECP_SC		= 0x01,
+	NVME_AUTH_SECP_NEWTLSPSK	= 0x02,
+	NVME_AUTH_SECP_REPLACETLSPSK	= 0x03,
+};
+
 union nvmf_auth_protocol {
 	struct nvmf_auth_dhchap_protocol_descriptor dhchap;
 };
-- 
2.35.3

Re: [PATCH 11/17] nvme-tcp: request secure channel concatenation

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> Add a fabrics option 'concat' to request secure channel concatenation.

maybe name it secconcat ? or secure_concat ?

> When secure channel concatenation is enabled a 'generated PSK' is inserted
> into the keyring such that it's available after reset.

Umm, I'm not sure what this means? Is this the revoke action? what if
the user passes a keyring? I'm not entirely sure what is the interface 
semantics here.

>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/auth.c    | 108 ++++++++++++++++++++++++++++++++++--
>   drivers/nvme/host/fabrics.c |  25 ++++++++-
>   drivers/nvme/host/fabrics.h |   3 +
>   drivers/nvme/host/sysfs.c   |   2 +
>   drivers/nvme/host/tcp.c     |  53 ++++++++++++++++--
>   include/linux/nvme.h        |   7 +++
>   6 files changed, 186 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index a264b3ae078b..a4033a8f9607 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -12,6 +12,7 @@
>   #include "nvme.h"
>   #include "fabrics.h"
>   #include <linux/nvme-auth.h>
> +#include <linux/nvme-keyring.h>
>   
>   #define CHAP_BUF_SIZE 4096
>   static struct kmem_cache *nvme_chap_buf_cache;
> @@ -131,7 +132,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 || 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;
>   	data->napd = 1;
>   	data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
>   	data->auth_protocol[0].dhchap.halen = 3;
> @@ -311,8 +317,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,
> @@ -322,7 +329,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",
> @@ -677,6 +687,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);
> +	}

Question, is this for the case where the user passes both tls_key and
concat? Is this something that we expect users to do? What is the
benefit?

> +	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 =
> @@ -831,10 +914,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)
> @@ -912,6 +1006,12 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
>   			 "qid 0: authentication failed\n");
>   		return;
>   	}
> +	/*
> +	* Only run authentication on the admin queue for
> +	* secure concatenation
> +	 */
> +	if (ctrl->opts->concat)
> +		return;
>   
>   	for (q = 1; q < ctrl->queue_count; q++) {
>   		ret = nvme_auth_negotiate(ctrl, q);
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 75aa69457353..ae091e0e4ecf 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -463,8 +463,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");

Perhaps the log msg should be adjusted as well.

>   			ret = NVME_SC_AUTH_REQUIRED;
> @@ -682,6 +683,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			}
>   };
> @@ -711,6 +713,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)
> @@ -1029,6 +1032,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;

Isn't there a dependency with a dhchap auth rquested? meaning what 
happens if the user
did not pass a dhchap_secret?

>   		default:
>   			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
>   				p);
> @@ -1055,6 +1066,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)) {
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 06cc54851b1b..accfc7228366 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -73,6 +73,7 @@ enum {
>   	NVMF_OPT_TLS		= 1 << 25,
>   	NVMF_OPT_KEYRING	= 1 << 26,
>   	NVMF_OPT_TLS_KEY	= 1 << 27,
> +	NVMF_OPT_CONCAT		= 1 << 28,
>   };
>   
>   /**
> @@ -108,6 +109,7 @@ enum {
>    * @keyring:    Keyring to use for key lookups
>    * @tls_key:    TLS key for encrypted connections (TCP)
>    * @tls:        Start TLS encrypted connections (TCP)
> + * @concat:     Enabled Secure channel concatenation (TCP)
>    * @disable_sqflow: disable controller sq flow control
>    * @hdr_digest: generate/verify header digest (TCP)
>    * @data_digest: generate/verify data digest (TCP)
> @@ -137,6 +139,7 @@ struct nvmf_ctrl_options {
>   	struct key		*keyring;
>   	struct key		*tls_key;
>   	bool			tls;
> +	bool			concat;
>   	bool			disable_sqflow;
>   	bool			hdr_digest;
>   	bool			data_digest;
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 07ec26de2d91..edd96f501d8a 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -675,6 +675,8 @@ static ssize_t tls_key_show(struct device *dev,
>   
>   	if (!key)
>   		return 0;
> +	if (ctrl->opts->concat)
> +		return 0;
>   	if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
>   	    test_bit(KEY_FLAG_INVALIDATED, &key->flags))
>   		return -EKEYREVOKED;
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 66675b2dc197..94152ded123a 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -219,7 +219,7 @@ static inline bool nvme_tcp_tls_configured(struct nvme_ctrl *ctrl)
>   	if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
>   		return 0;
>   
> -	return ctrl->opts->tls;
> +	return ctrl->opts->tls || ctrl->opts->concat;
>   }
>   
>   static inline struct blk_mq_tags *nvme_tcp_tagset(struct nvme_tcp_queue *queue)
> @@ -1941,7 +1941,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);
> @@ -1973,12 +1973,28 @@ static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
>   	key_serial_t pskid = ctrl->tls_key ? key_serial(ctrl->tls_key) : 0;
>   	int i, ret;
>   
> -	if (nvme_tcp_tls_configured(ctrl) && !pskid) {
> -		dev_err(ctrl->device, "no PSK negotiated\n");
> -		return -ENOKEY;
> +	if (nvme_tcp_tls_configured(ctrl)) {
> +		if (ctrl->opts->concat) {
> +			/*
> +			 * The generated PSK is stored in the
> +			 * fabric options
> +			 */
> +			if (!ctrl->opts->tls_key) {
> +				dev_err(ctrl->device, "no PSK generated\n");
> +				return -ENOKEY;
> +			}
> +			if (pskid && pskid != key_serial(ctrl->opts->tls_key)) {
> +				dev_err(ctrl->device, "Stale PSK id %08x\n", pskid);
> +				pskid = 0;
> +			}
> +		} else if (!pskid) {
> +			dev_err(ctrl->device, "no PSK negotiated\n");
> +			return -ENOKEY;
> +		}
>   	}
>   
>   	for (i = 1; i < ctrl->queue_count; i++) {
> +		WARN_ON(nvme_tcp_tls_enabled(&tcp_ctrl->queues[i]));
>   		ret = nvme_tcp_alloc_queue(ctrl, i, pskid);
>   		if (ret)
>   			goto out_free_queues;
> @@ -2203,6 +2219,26 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>   	}
>   }
>   
> +static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
> +{
> +	if (!ctrl->opts->concat)
> +		return;
> +	/* No key generated, nothing to do */
> +	if (!ctrl->opts->tls_key)
> +		return;
> +	/*
> +	 * Key generated, and TLS enabled:
> +	 * Revoke the generated key.
> +	 */
> +	if (ctrl->tls_key) {
> +		dev_dbg(ctrl->device, "Wipe generated TLS PSK %08x\n",
> +			key_serial(ctrl->opts->tls_key));
> +		key_revoke(ctrl->opts->tls_key);
> +		key_put(ctrl->opts->tls_key);
> +		ctrl->opts->tls_key = NULL;
> +	}
> +}
> +
>   static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>   {
>   	struct nvmf_ctrl_options *opts = ctrl->opts;
> @@ -2304,6 +2340,7 @@ static void nvme_tcp_error_recovery_work(struct work_struct *work)
>   				struct nvme_tcp_ctrl, err_work);
>   	struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
>   
> +	nvme_tcp_revoke_generated_tls_key(ctrl);
>   	nvme_stop_keep_alive(ctrl);
>   	flush_work(&ctrl->async_event_work);
>   	nvme_tcp_teardown_io_queues(ctrl, false);
> @@ -2343,6 +2380,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
>   	struct nvme_ctrl *ctrl =
>   		container_of(work, struct nvme_ctrl, reset_work);
>   
> +	nvme_tcp_revoke_generated_tls_key(ctrl);
>   	nvme_stop_ctrl(ctrl);
>   	nvme_tcp_teardown_ctrl(ctrl, false);
>   
> @@ -2632,6 +2670,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))
> @@ -2817,7 +2858,7 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
>   			  NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
>   			  NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
>   			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
> -			  NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
> +			  NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
>   	.create_ctrl	= nvme_tcp_create_ctrl,
>   };
>   
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index ce0c1143c7e4..b29310424ee1 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1663,6 +1663,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;
>   };

Re: [PATCH 11/17] nvme-tcp: request secure channel concatenation

[Hannes Reinecke]

On 4/7/24 23:41, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> Add a fabrics option 'concat' to request secure channel concatenation.
> 
> maybe name it secconcat ? or secure_concat ?
> 
>> When secure channel concatenation is enabled a 'generated PSK' is 
>> inserted
>> into the keyring such that it's available after reset.
> 
> Umm, I'm not sure what this means? Is this the revoke action? what if
> the user passes a keyring? I'm not entirely sure what is the interface 
> semantics here.
> 
No, this is not the 'revoke' action. This is the modification TP8018
brought in. For secure concatenation the initial connection is done
without encryption, DH-CHAP is run and generates the TLS PSK. Then
the connection is reset, and the new connection starts TLS with the
generated key.

>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/auth.c    | 108 ++++++++++++++++++++++++++++++++++--
>>   drivers/nvme/host/fabrics.c |  25 ++++++++-
>>   drivers/nvme/host/fabrics.h |   3 +
>>   drivers/nvme/host/sysfs.c   |   2 +
>>   drivers/nvme/host/tcp.c     |  53 ++++++++++++++++--
>>   include/linux/nvme.h        |   7 +++
>>   6 files changed, 186 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>> index a264b3ae078b..a4033a8f9607 100644
>> --- a/drivers/nvme/host/auth.c
>> +++ b/drivers/nvme/host/auth.c
>> @@ -12,6 +12,7 @@
>>   #include "nvme.h"
>>   #include "fabrics.h"
>>   #include <linux/nvme-auth.h>
>> +#include <linux/nvme-keyring.h>
>>   #define CHAP_BUF_SIZE 4096
>>   static struct kmem_cache *nvme_chap_buf_cache;
>> @@ -131,7 +132,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 || 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;
>>       data->napd = 1;
>>       data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
>>       data->auth_protocol[0].dhchap.halen = 3;
>> @@ -311,8 +317,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,
>> @@ -322,7 +329,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",
>> @@ -677,6 +687,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);
>> +    }
> 
> Question, is this for the case where the user passes both tls_key and
> concat? Is this something that we expect users to do? What is the
> benefit?
> 
>> +    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 =
>> @@ -831,10 +914,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)
>> @@ -912,6 +1006,12 @@ static void nvme_ctrl_auth_work(struct 
>> work_struct *work)
>>                "qid 0: authentication failed\n");
>>           return;
>>       }
>> +    /*
>> +    * Only run authentication on the admin queue for
>> +    * secure concatenation
>> +     */
>> +    if (ctrl->opts->concat)
>> +        return;
>>       for (q = 1; q < ctrl->queue_count; q++) {
>>           ret = nvme_auth_negotiate(ctrl, q);
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 75aa69457353..ae091e0e4ecf 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -463,8 +463,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");
> 
> Perhaps the log msg should be adjusted as well.
> 
>>               ret = NVME_SC_AUTH_REQUIRED;
>> @@ -682,6 +683,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            }
>>   };
>> @@ -711,6 +713,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)
>> @@ -1029,6 +1032,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;
> 
> Isn't there a dependency with a dhchap auth rquested? meaning what 
> happens if the user
> did not pass a dhchap_secret?
> 
Ah, yes, indeed. Need to check for that.

>>           default:
>>               pr_warn("unknown parameter or missing value '%s' in ctrl 
>> creation request\n",
>>                   p);
>> @@ -1055,6 +1066,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)) {
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index 06cc54851b1b..accfc7228366 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -73,6 +73,7 @@ enum {
>>       NVMF_OPT_TLS        = 1 << 25,
>>       NVMF_OPT_KEYRING    = 1 << 26,
>>       NVMF_OPT_TLS_KEY    = 1 << 27,
>> +    NVMF_OPT_CONCAT        = 1 << 28,
>>   };
>>   /**
>> @@ -108,6 +109,7 @@ enum {
>>    * @keyring:    Keyring to use for key lookups
>>    * @tls_key:    TLS key for encrypted connections (TCP)
>>    * @tls:        Start TLS encrypted connections (TCP)
>> + * @concat:     Enabled Secure channel concatenation (TCP)
>>    * @disable_sqflow: disable controller sq flow control
>>    * @hdr_digest: generate/verify header digest (TCP)
>>    * @data_digest: generate/verify data digest (TCP)
>> @@ -137,6 +139,7 @@ struct nvmf_ctrl_options {
>>       struct key        *keyring;
>>       struct key        *tls_key;
>>       bool            tls;
>> +    bool            concat;
>>       bool            disable_sqflow;
>>       bool            hdr_digest;
>>       bool            data_digest;
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 07ec26de2d91..edd96f501d8a 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -675,6 +675,8 @@ static ssize_t tls_key_show(struct device *dev,
>>       if (!key)
>>           return 0;
>> +    if (ctrl->opts->concat)
>> +        return 0;
>>       if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
>>           test_bit(KEY_FLAG_INVALIDATED, &key->flags))
>>           return -EKEYREVOKED;
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 66675b2dc197..94152ded123a 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -219,7 +219,7 @@ static inline bool nvme_tcp_tls_configured(struct 
>> nvme_ctrl *ctrl)
>>       if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
>>           return 0;
>> -    return ctrl->opts->tls;
>> +    return ctrl->opts->tls || ctrl->opts->concat;
>>   }
>>   static inline struct blk_mq_tags *nvme_tcp_tagset(struct 
>> nvme_tcp_queue *queue)
>> @@ -1941,7 +1941,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);
>> @@ -1973,12 +1973,28 @@ static int __nvme_tcp_alloc_io_queues(struct 
>> nvme_ctrl *ctrl)
>>       key_serial_t pskid = ctrl->tls_key ? key_serial(ctrl->tls_key) : 0;
>>       int i, ret;
>> -    if (nvme_tcp_tls_configured(ctrl) && !pskid) {
>> -        dev_err(ctrl->device, "no PSK negotiated\n");
>> -        return -ENOKEY;
>> +    if (nvme_tcp_tls_configured(ctrl)) {
>> +        if (ctrl->opts->concat) {
>> +            /*
>> +             * The generated PSK is stored in the
>> +             * fabric options
>> +             */
>> +            if (!ctrl->opts->tls_key) {
>> +                dev_err(ctrl->device, "no PSK generated\n");
>> +                return -ENOKEY;
>> +            }
>> +            if (pskid && pskid != key_serial(ctrl->opts->tls_key)) {
>> +                dev_err(ctrl->device, "Stale PSK id %08x\n", pskid);
>> +                pskid = 0;
>> +            }
>> +        } else if (!pskid) {
>> +            dev_err(ctrl->device, "no PSK negotiated\n");
>> +            return -ENOKEY;
>> +        }
>>       }
>>       for (i = 1; i < ctrl->queue_count; i++) {
>> +        WARN_ON(nvme_tcp_tls_enabled(&tcp_ctrl->queues[i]));
>>           ret = nvme_tcp_alloc_queue(ctrl, i, pskid);
>>           if (ret)
>>               goto out_free_queues;
>> @@ -2203,6 +2219,26 @@ static void nvme_tcp_reconnect_or_remove(struct 
>> nvme_ctrl *ctrl)
>>       }
>>   }
>> +static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
>> +{
>> +    if (!ctrl->opts->concat)
>> +        return;
>> +    /* No key generated, nothing to do */
>> +    if (!ctrl->opts->tls_key)
>> +        return;
>> +    /*
>> +     * Key generated, and TLS enabled:
>> +     * Revoke the generated key.
>> +     */
>> +    if (ctrl->tls_key) {
>> +        dev_dbg(ctrl->device, "Wipe generated TLS PSK %08x\n",
>> +            key_serial(ctrl->opts->tls_key));
>> +        key_revoke(ctrl->opts->tls_key);
>> +        key_put(ctrl->opts->tls_key);
>> +        ctrl->opts->tls_key = NULL;
>> +    }
>> +}
>> +
>>   static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>>   {
>>       struct nvmf_ctrl_options *opts = ctrl->opts;
>> @@ -2304,6 +2340,7 @@ static void nvme_tcp_error_recovery_work(struct 
>> work_struct *work)
>>                   struct nvme_tcp_ctrl, err_work);
>>       struct nvme_ctrl *ctrl = &tcp_ctrl->ctrl;
>> +    nvme_tcp_revoke_generated_tls_key(ctrl);
>>       nvme_stop_keep_alive(ctrl);
>>       flush_work(&ctrl->async_event_work);
>>       nvme_tcp_teardown_io_queues(ctrl, false);
>> @@ -2343,6 +2380,7 @@ static void nvme_reset_ctrl_work(struct 
>> work_struct *work)
>>       struct nvme_ctrl *ctrl =
>>           container_of(work, struct nvme_ctrl, reset_work);
>> +    nvme_tcp_revoke_generated_tls_key(ctrl);
>>       nvme_stop_ctrl(ctrl);
>>       nvme_tcp_teardown_ctrl(ctrl, false);
>> @@ -2632,6 +2670,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))
>> @@ -2817,7 +2858,7 @@ static struct nvmf_transport_ops 
>> nvme_tcp_transport = {
>>                 NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
>>                 NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
>>                 NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
>> -              NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
>> +              NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
>>       .create_ctrl    = nvme_tcp_create_ctrl,
>>   };
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index ce0c1143c7e4..b29310424ee1 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
>> @@ -1663,6 +1663,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;
>>   };
> 

Cheers,

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

Re: [PATCH 11/17] nvme-tcp: request secure channel concatenation

[Sagi Grimberg]

On 08/04/2024 8:32, Hannes Reinecke wrote:
> On 4/7/24 23:41, Sagi Grimberg wrote:
>>
>>
>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> Add a fabrics option 'concat' to request secure channel concatenation.
>>
>> maybe name it secconcat ? or secure_concat ?
>>
>>> When secure channel concatenation is enabled a 'generated PSK' is 
>>> inserted
>>> into the keyring such that it's available after reset.
>>
>> Umm, I'm not sure what this means? Is this the revoke action? what if
>> the user passes a keyring? I'm not entirely sure what is the 
>> interface semantics here.
>>
> No, this is not the 'revoke' action. This is the modification TP8018
> brought in. For secure concatenation the initial connection is done
> without encryption, DH-CHAP is run and generates the TLS PSK. Then
> the connection is reset, and the new connection starts TLS with the
> generated key.

Yes, I misread "it's available" and "it's not available". Sorry.

>
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/nvme/host/auth.c    | 108 
>>> ++++++++++++++++++++++++++++++++++--
>>>   drivers/nvme/host/fabrics.c |  25 ++++++++-
>>>   drivers/nvme/host/fabrics.h |   3 +
>>>   drivers/nvme/host/sysfs.c   |   2 +
>>>   drivers/nvme/host/tcp.c     |  53 ++++++++++++++++--
>>>   include/linux/nvme.h        |   7 +++
>>>   6 files changed, 186 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>>> index a264b3ae078b..a4033a8f9607 100644
>>> --- a/drivers/nvme/host/auth.c
>>> +++ b/drivers/nvme/host/auth.c
>>> @@ -12,6 +12,7 @@
>>>   #include "nvme.h"
>>>   #include "fabrics.h"
>>>   #include <linux/nvme-auth.h>
>>> +#include <linux/nvme-keyring.h>
>>>   #define CHAP_BUF_SIZE 4096
>>>   static struct kmem_cache *nvme_chap_buf_cache;
>>> @@ -131,7 +132,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 || 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;
>>>       data->napd = 1;
>>>       data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
>>>       data->auth_protocol[0].dhchap.halen = 3;
>>> @@ -311,8 +317,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,
>>> @@ -322,7 +329,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",
>>> @@ -677,6 +687,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);
>>> +    }
>>
>> Question, is this for the case where the user passes both tls_key and
>> concat? Is this something that we expect users to do? What is the
>> benefit?

Answer?

Re: [PATCH 11/17] nvme-tcp: request secure channel concatenation

[Hannes Reinecke]

On 4/8/24 23:09, Sagi Grimberg wrote:
> 
> 
> On 08/04/2024 8:32, Hannes Reinecke wrote:
>> On 4/7/24 23:41, Sagi Grimberg wrote:
>>>
>>>
>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>> From: Hannes Reinecke <hare@suse.de>
>>>>
>>>> Add a fabrics option 'concat' to request secure channel concatenation.
>>>
>>> maybe name it secconcat ? or secure_concat ?
>>>
>>>> When secure channel concatenation is enabled a 'generated PSK' is 
>>>> inserted
>>>> into the keyring such that it's available after reset.
>>>
>>> Umm, I'm not sure what this means? Is this the revoke action? what if
>>> the user passes a keyring? I'm not entirely sure what is the 
>>> interface semantics here.
>>>
>> No, this is not the 'revoke' action. This is the modification TP8018
>> brought in. For secure concatenation the initial connection is done
>> without encryption, DH-CHAP is run and generates the TLS PSK. Then
>> the connection is reset, and the new connection starts TLS with the
>> generated key.
> 
> Yes, I misread "it's available" and "it's not available". Sorry.
> 
>>
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>>   drivers/nvme/host/auth.c    | 108 
>>>> ++++++++++++++++++++++++++++++++++--
>>>>   drivers/nvme/host/fabrics.c |  25 ++++++++-
>>>>   drivers/nvme/host/fabrics.h |   3 +
>>>>   drivers/nvme/host/sysfs.c   |   2 +
>>>>   drivers/nvme/host/tcp.c     |  53 ++++++++++++++++--
>>>>   include/linux/nvme.h        |   7 +++
>>>>   6 files changed, 186 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>>>> index a264b3ae078b..a4033a8f9607 100644
>>>> --- a/drivers/nvme/host/auth.c
>>>> +++ b/drivers/nvme/host/auth.c
>>>> @@ -12,6 +12,7 @@
>>>>   #include "nvme.h"
>>>>   #include "fabrics.h"
>>>>   #include <linux/nvme-auth.h>
>>>> +#include <linux/nvme-keyring.h>
>>>>   #define CHAP_BUF_SIZE 4096
>>>>   static struct kmem_cache *nvme_chap_buf_cache;
>>>> @@ -131,7 +132,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 || 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;
>>>>       data->napd = 1;
>>>>       data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
>>>>       data->auth_protocol[0].dhchap.halen = 3;
>>>> @@ -311,8 +317,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,
>>>> @@ -322,7 +329,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",
>>>> @@ -677,6 +687,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);
>>>> +    }
>>>
>>> Question, is this for the case where the user passes both tls_key and
>>> concat? Is this something that we expect users to do? What is the
>>> benefit?
> 
> Answer?
> 
Once DH-HMAC-CHAP is run a new key is generated, and that key should be 
used for the subsequent TLS connection. So I need to find a location 
where to store the key, _and_ be sure that the key reference is kept 
intact during controller reset (which I need to do to establish the TLS 
connection).
So I store the generated key in 'opts->tls_key', as this structure is 
kept around throughout the lifetime of a connection.
I am aware that the controller structure has a 'tls_key' entry, too, but
this holds the key being negotiated from the TLS handshake, so we cannot
store it there.
For reference, the schedule is:
ctrl->opts->tls_key: key to be used for the TLS handshake, overriding
  the default selection scheme
ctrl->tls_key: negotiated key from TLS handshake on the admin channel.
  Defines the key to be used for the I/O channels; I/O channels do
  _not_ use the default selection scheme
queue->tls_pskid: serial number of the negotiated keys from the TLS
  handshake.

I do agree that we could use a flag instead of the per-queue tls pskid,
but then we'll lose the information _which_ key had been used for
the TLS handshake. Keys will be regenerated after each DH-CHAP
negotiation, and the spec allows for running DH-CHAP negotiation
on the encrypted connection.
(Not that it's currently implemented, but we might want to eventually).
In that cases we would need to know which key is currently active
on the TLS connection. That information is pretty much opaque as
the in-kernel crypto is using derived key information, and we cannot
get the original key information via any other means.
So I'd rather keep it.

Cheers,

Hannes

Re: [PATCH 11/17] nvme-tcp: request secure channel concatenation

[Sagi Grimberg]

On 09/04/2024 0:48, Hannes Reinecke wrote:
> On 4/8/24 23:09, Sagi Grimberg wrote:
>>
>>
>> On 08/04/2024 8:32, Hannes Reinecke wrote:
>>> On 4/7/24 23:41, Sagi Grimberg wrote:
>>>>
>>>>
>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>> From: Hannes Reinecke <hare@suse.de>
>>>>>
>>>>> Add a fabrics option 'concat' to request secure channel 
>>>>> concatenation.
>>>>
>>>> maybe name it secconcat ? or secure_concat ?
>>>>
>>>>> When secure channel concatenation is enabled a 'generated PSK' is 
>>>>> inserted
>>>>> into the keyring such that it's available after reset.
>>>>
>>>> Umm, I'm not sure what this means? Is this the revoke action? what if
>>>> the user passes a keyring? I'm not entirely sure what is the 
>>>> interface semantics here.
>>>>
>>> No, this is not the 'revoke' action. This is the modification TP8018
>>> brought in. For secure concatenation the initial connection is done
>>> without encryption, DH-CHAP is run and generates the TLS PSK. Then
>>> the connection is reset, and the new connection starts TLS with the
>>> generated key.
>>
>> Yes, I misread "it's available" and "it's not available". Sorry.
>>
>>>
>>>>>
>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>> ---
>>>>>   drivers/nvme/host/auth.c    | 108 
>>>>> ++++++++++++++++++++++++++++++++++--
>>>>>   drivers/nvme/host/fabrics.c |  25 ++++++++-
>>>>>   drivers/nvme/host/fabrics.h |   3 +
>>>>>   drivers/nvme/host/sysfs.c   |   2 +
>>>>>   drivers/nvme/host/tcp.c     |  53 ++++++++++++++++--
>>>>>   include/linux/nvme.h        |   7 +++
>>>>>   6 files changed, 186 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>>>>> index a264b3ae078b..a4033a8f9607 100644
>>>>> --- a/drivers/nvme/host/auth.c
>>>>> +++ b/drivers/nvme/host/auth.c
>>>>> @@ -12,6 +12,7 @@
>>>>>   #include "nvme.h"
>>>>>   #include "fabrics.h"
>>>>>   #include <linux/nvme-auth.h>
>>>>> +#include <linux/nvme-keyring.h>
>>>>>   #define CHAP_BUF_SIZE 4096
>>>>>   static struct kmem_cache *nvme_chap_buf_cache;
>>>>> @@ -131,7 +132,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 || 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;
>>>>>       data->napd = 1;
>>>>>       data->auth_protocol[0].dhchap.authid = 
>>>>> NVME_AUTH_DHCHAP_AUTH_ID;
>>>>>       data->auth_protocol[0].dhchap.halen = 3;
>>>>> @@ -311,8 +317,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,
>>>>> @@ -322,7 +329,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",
>>>>> @@ -677,6 +687,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);
>>>>> +    }
>>>>
>>>> Question, is this for the case where the user passes both tls_key and
>>>> concat? Is this something that we expect users to do? What is the
>>>> benefit?
>>
>> Answer?
>>
> Once DH-HMAC-CHAP is run a new key is generated, and that key should 
> be used for the subsequent TLS connection. So I need to find a 
> location where to store the key, _and_ be sure that the key reference 
> is kept intact during controller reset (which I need to do to 
> establish the TLS connection).
> So I store the generated key in 'opts->tls_key', as this structure is 
> kept around throughout the lifetime of a connection.

Why do you store a generated key in opts? That is supposed to hold what 
the user passed.

> I am aware that the controller structure has a 'tls_key' entry, too, but
> this holds the key being negotiated from the TLS handshake, so we cannot
> store it there.

I suggest to introduce a new tls_generated_key ctrl member. Lets not 
overload
opts->tls_key which is designed to hold the a user driven tls_key 
(unless I'm misunderstanding
what you are saying?).


> For reference, the schedule is:
> ctrl->opts->tls_key: key to be used for the TLS handshake, overriding
>  the default selection scheme

This is what the user passes in?

> ctrl->tls_key: negotiated key from TLS handshake on the admin channel.
>  Defines the key to be used for the I/O channels; I/O channels do
>  _not_ use the default selection scheme
> queue->tls_pskid: serial number of the negotiated keys from the TLS
>  handshake.

what do you mean by "negotiated keys"? plural?

> I do agree that we could use a flag instead of the per-queue tls pskid,
> but then we'll lose the information _which_ key had been used for
> the TLS handshake.

Isn't it stored in the ctrl?

> Keys will be regenerated after each DH-CHAP
> negotiation, and the spec allows for running DH-CHAP negotiation
> on the encrypted connection.
> (Not that it's currently implemented, but we might want to eventually).
> In that cases we would need to know which key is currently active
> on the TLS connection. That information is pretty much opaque as
> the in-kernel crypto is using derived key information, and we cannot
> get the original key information via any other means.
> So I'd rather keep it.

Can you give a concrete example of what info you are missing?
I'm not following...

[PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[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.
The current implementation does not allow to easily skip
connection attempts on the I/O queues, so we connect I/O
queues, but disable namespace scanning on these queues.
With that no I/O can be issued on these queues, so we
can tear them down quickly without having to wait for
quiescing etc.
Once that is done we can reset the controller directly
after the ->create_ctrl() callback.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c    | 8 +++++++-
 drivers/nvme/host/fabrics.c | 6 ++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9b601655f423..57b664d12863 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4513,6 +4513,8 @@ EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
 
 void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 {
+	bool start_scan = ctrl->queue_count > 1;
+
 	nvme_enable_aen(ctrl);
 
 	/*
@@ -4525,7 +4527,11 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 	    nvme_discovery_ctrl(ctrl))
 		nvme_change_uevent(ctrl, "NVME_EVENT=rediscover");
 
-	if (ctrl->queue_count > 1) {
+	/* Suppress namespace scanning during setting up secure concatenation */
+	if (ctrl->opts->concat && !ctrl->tls_key)
+		start_scan = false;
+
+	if (start_scan) {
 		nvme_queue_scan(ctrl);
 		nvme_unquiesce_io_queues(ctrl);
 		nvme_mpath_update(ctrl);
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index ae091e0e4ecf..06418e01ab69 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -1331,6 +1331,12 @@ nvmf_create_ctrl(struct device *dev, const char *buf)
 		goto out_module_put;
 	}
 
+	/* Reset controller to start TLS */
+	if (opts->concat) {
+		pr_debug("resetting for secure concatenation\n");
+		nvme_reset_ctrl(ctrl);
+	}
+
 	module_put(ops->module);
 	return ctrl;
 
-- 
2.35.3

Re: [PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> 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.
> The current implementation does not allow to easily skip
> connection attempts on the I/O queues, so we connect I/O
> queues, but disable namespace scanning on these queues.
> With that no I/O can be issued on these queues, so we
> can tear them down quickly without having to wait for
> quiescing etc.

We shouldn't have to connect io queues here. The scan prevention
is just a hack...

> Once that is done we can reset the controller directly
> after the ->create_ctrl() callback.

Why not set opts->nr_io_queues = 0 for secure concatenation and
setting it to the original value before resetting?

Re: [PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Hannes Reinecke]

On 4/7/24 23:46, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> 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.
>> The current implementation does not allow to easily skip
>> connection attempts on the I/O queues, so we connect I/O
>> queues, but disable namespace scanning on these queues.
>> With that no I/O can be issued on these queues, so we
>> can tear them down quickly without having to wait for
>> quiescing etc.
> 
> We shouldn't have to connect io queues here. The scan prevention
> is just a hack...
> 
Oh, believe me, I tried. What _should_ be possible is to create a
controller with just admin queues, and skip the io queue setup part.
But once you do that it's impossible to create io queues after reset
(which is what you'd need here).
I tried to fix this, but that ended up with a massive rewrite/reordering
of the init path. Which sure has its merits, but I deemed out of scope
for this patchset.

So I decided for this 'hack', and shelved the init path reorg for a
later patchset.

>> Once that is done we can reset the controller directly
>> after the ->create_ctrl() callback.
> 
> Why not set opts->nr_io_queues = 0 for secure concatenation and
> setting it to the original value before resetting?

See above. The io tagset is allocated in the init path _only_.
And creating of the io tagset is tied with connecting the io queues.
The reset code requires the tagset to be present; it just reconnects
the io queues.
So either you do some trickery here like skipping connecting io queues
(or, indeed, skipping namespace scanning),
or you end up with a massive reshuffling of the init code path trying
to separate tagset creation from io queue connections.

Cheers,

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

Re: [PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Sagi Grimberg]

On 08/04/2024 9:21, Hannes Reinecke wrote:
> On 4/7/24 23:46, Sagi Grimberg wrote:
>>
>>
>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>> 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.
>>> The current implementation does not allow to easily skip
>>> connection attempts on the I/O queues, so we connect I/O
>>> queues, but disable namespace scanning on these queues.
>>> With that no I/O can be issued on these queues, so we
>>> can tear them down quickly without having to wait for
>>> quiescing etc.
>>
>> We shouldn't have to connect io queues here. The scan prevention
>> is just a hack...
>>
> Oh, believe me, I tried. What _should_ be possible is to create a
> controller with just admin queues, and skip the io queue setup part.
> But once you do that it's impossible to create io queues after reset
> (which is what you'd need here).
> I tried to fix this, but that ended up with a massive rewrite/reordering
> of the init path. Which sure has its merits, but I deemed out of scope
> for this patchset.
>
> So I decided for this 'hack', and shelved the init path reorg for a
> later patchset.

Interesting that it requires a full rewrite. Can you share some info
what particularly breaks when you change the nr_io_queues between
resets? nr_io_queues can be changed across resets from the controller side
as well.

>
>>> Once that is done we can reset the controller directly
>>> after the ->create_ctrl() callback.
>>
>> Why not set opts->nr_io_queues = 0 for secure concatenation and
>> setting it to the original value before resetting?
>
> See above. The io tagset is allocated in the init path _only_.

ok. So? What is the issue here? is this because ns scanning is checking
the existence of a ctrl->tagset? I predicted that this check will be 
problematic
to assume the ctrl state in the future.

We can add a controller flag for this.

> And creating of the io tagset is tied with connecting the io queues.

Where exactly?

>
> The reset code requires the tagset to be present; it just reconnects
> the io queues.

it connects io_queues if ctrl->queue_count > 1

> So either you do some trickery here like skipping connecting io queues
> (or, indeed, skipping namespace scanning),
> or you end up with a massive reshuffling of the init code path trying
> to separate tagset creation from io queue connections.

Hmm, I'm not exactly sure what separation exactly is required here.

Re: [PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Hannes Reinecke]

On 4/8/24 23:21, Sagi Grimberg wrote:
> 
> 
> On 08/04/2024 9:21, Hannes Reinecke wrote:
>> On 4/7/24 23:46, Sagi Grimberg wrote:
>>>
>>>
>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>> 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.
>>>> The current implementation does not allow to easily skip
>>>> connection attempts on the I/O queues, so we connect I/O
>>>> queues, but disable namespace scanning on these queues.
>>>> With that no I/O can be issued on these queues, so we
>>>> can tear them down quickly without having to wait for
>>>> quiescing etc.
>>>
>>> We shouldn't have to connect io queues here. The scan prevention
>>> is just a hack...
>>>
>> Oh, believe me, I tried. What _should_ be possible is to create a
>> controller with just admin queues, and skip the io queue setup part.
>> But once you do that it's impossible to create io queues after reset
>> (which is what you'd need here).
>> I tried to fix this, but that ended up with a massive rewrite/reordering
>> of the init path. Which sure has its merits, but I deemed out of scope
>> for this patchset.
>>
>> So I decided for this 'hack', and shelved the init path reorg for a
>> later patchset.
> 
> Interesting that it requires a full rewrite. Can you share some info
> what particularly breaks when you change the nr_io_queues between
> resets? nr_io_queues can be changed across resets from the controller side
> as well.
> 
Just look at the 'new' parameter to nvme_tcp_configure_io_queues(),
and try to modify the code to _not_ required the parameter.
Key observation here is that the _size_ of the I/O tagset is actually
defined by the parameters for the connect command; we cannot grow
beyond that. Which means that we should be allocate the I/O tagset
right at the start, and only modify the number of queue which we _use_
based on the information we get back from the controller.
Currently the io tagset allocation is tied to the number of queues
for the controller, and trying to modify that is an even worse hack
than suppressing the namespace scan.

Anyway,it's quite a different patchset which is basically unrelated
to the secure concatenation work.

I do have a preliminary patchset for this if there's interest.

>>
>>>> Once that is done we can reset the controller directly
>>>> after the ->create_ctrl() callback.
>>>
>>> Why not set opts->nr_io_queues = 0 for secure concatenation and
>>> setting it to the original value before resetting?
>>
>> See above. The io tagset is allocated in the init path _only_.
> 
> ok. So? What is the issue here? is this because ns scanning is checking
> the existence of a ctrl->tagset? I predicted that this check will be 
> problematic to assume the ctrl state in the future.
> 
> We can add a controller flag for this.
> 
Problem is that namespace scanning will create block devices, which will
create uevent, which will cause udev to start scan for signatures etc.
At the same time we're resetting the queue, causing all sorts of race 
conditions and timing issues for the outstanding I/Os.
And all of that for namespaces which have a questionable existence
as they are only valid for _encrypted_ connections.

>> And creating of the io tagset is tied with connecting the io queues.
> 
> Where exactly?
> 
Check the 'new' parameter for nvme_configure_io_queues().
That is being called _only_ when we have more than 1 queue.
And is doing both, allocating the io tagset _and_ issue a 'connect' call
on each queue. We'd need to split that into allocating the io tagset
(which should happen always) and the 'connect' call (which should be
done for non-concat connection or concat connections with TLS enabled).

Cheers,

Hannes

>>
>> The reset code requires the tagset to be present; it just reconnects
>> the io queues.
> 
> it connects io_queues if ctrl->queue_count > 1
> 
and allocates the io tagset ...

>> So either you do some trickery here like skipping connecting io queues
>> (or, indeed, skipping namespace scanning),
>> or you end up with a massive reshuffling of the init code path trying
>> to separate tagset creation from io queue connections.
> 
> Hmm, I'm not exactly sure what separation exactly is required here.

The tagset will always need to be allocated, but the 'connect' command 
should be skipped for non-TLS enabled secure concat connections.

Cheers,

Hannes

Re: [PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Sagi Grimberg]

On 09/04/2024 1:08, Hannes Reinecke wrote:
> On 4/8/24 23:21, Sagi Grimberg wrote:
>>
>>
>> On 08/04/2024 9:21, Hannes Reinecke wrote:
>>> On 4/7/24 23:46, Sagi Grimberg wrote:
>>>>
>>>>
>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>> 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.
>>>>> The current implementation does not allow to easily skip
>>>>> connection attempts on the I/O queues, so we connect I/O
>>>>> queues, but disable namespace scanning on these queues.
>>>>> With that no I/O can be issued on these queues, so we
>>>>> can tear them down quickly without having to wait for
>>>>> quiescing etc.
>>>>
>>>> We shouldn't have to connect io queues here. The scan prevention
>>>> is just a hack...
>>>>
>>> Oh, believe me, I tried. What _should_ be possible is to create a
>>> controller with just admin queues, and skip the io queue setup part.
>>> But once you do that it's impossible to create io queues after reset
>>> (which is what you'd need here).
>>> I tried to fix this, but that ended up with a massive 
>>> rewrite/reordering
>>> of the init path. Which sure has its merits, but I deemed out of scope
>>> for this patchset.
>>>
>>> So I decided for this 'hack', and shelved the init path reorg for a
>>> later patchset.
>>
>> Interesting that it requires a full rewrite. Can you share some info
>> what particularly breaks when you change the nr_io_queues between
>> resets? nr_io_queues can be changed across resets from the controller 
>> side
>> as well.
>>
> Just look at the 'new' parameter to nvme_tcp_configure_io_queues(),
> and try to modify the code to _not_ required the parameter.

What happens if you set ctrl->queue_count to 0 and then restore it?

> Key observation here is that the _size_ of the I/O tagset is actually
> defined by the parameters for the connect command; we cannot grow
> beyond that. Which means that we should be allocate the I/O tagset
> right at the start, and only modify the number of queue which we _use_
> based on the information we get back from the controller.

What does the tagset has to do with connecting the queues?

> Currently the io tagset allocation is tied to the number of queues
> for the controller, and trying to modify that is an even worse hack
> than suppressing the namespace scan.

I don't think it is... All I'm saying is that if we are connecting 64 io 
queues
to just right after reconnect them with tls, we are doing something wrong.

>
> Anyway,it's quite a different patchset which is basically unrelated
> to the secure concatenation work.

Well, they are required by the secure concatenation, but agree that
they are preparatory.

>
> I do have a preliminary patchset for this if there's interest.

Again, the fact that we connect io queues, disconnect them and then
connect them again is backwards I think.

Re: [PATCH 12/17] nvme-fabrics: reset connection for secure concatenation

[Hannes Reinecke]

On 4/21/24 13:20, Sagi Grimberg wrote:
> 
> 
> On 09/04/2024 1:08, Hannes Reinecke wrote:
>> On 4/8/24 23:21, Sagi Grimberg wrote:
>>>
>>>
>>> On 08/04/2024 9:21, Hannes Reinecke wrote:
>>>> On 4/7/24 23:46, Sagi Grimberg wrote:
>>>>>
>>>>>
>>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>>> 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.
>>>>>> The current implementation does not allow to easily skip
>>>>>> connection attempts on the I/O queues, so we connect I/O
>>>>>> queues, but disable namespace scanning on these queues.
>>>>>> With that no I/O can be issued on these queues, so we
>>>>>> can tear them down quickly without having to wait for
>>>>>> quiescing etc.
>>>>>
>>>>> We shouldn't have to connect io queues here. The scan prevention
>>>>> is just a hack...
>>>>>
>>>> Oh, believe me, I tried. What _should_ be possible is to create a
>>>> controller with just admin queues, and skip the io queue setup part.
>>>> But once you do that it's impossible to create io queues after reset
>>>> (which is what you'd need here).
>>>> I tried to fix this, but that ended up with a massive 
>>>> rewrite/reordering
>>>> of the init path. Which sure has its merits, but I deemed out of scope
>>>> for this patchset.
>>>>
>>>> So I decided for this 'hack', and shelved the init path reorg for a
>>>> later patchset.
>>>
>>> Interesting that it requires a full rewrite. Can you share some info
>>> what particularly breaks when you change the nr_io_queues between
>>> resets? nr_io_queues can be changed across resets from the controller 
>>> side
>>> as well.
>>>
>> Just look at the 'new' parameter to nvme_tcp_configure_io_queues(),
>> and try to modify the code to _not_ required the parameter.
> 
> What happens if you set ctrl->queue_count to 0 and then restore it?
> 
That way lays madness.

The tagset is allocated only _once_ when nvme_tcp_configure_io_queues()
is called with new == true (ie during the initial connect call).
So we cannot just skip the call and delegate it to reset as then the 
tagset will never be allocated.

And we have to call 'nvme_tcp_alloc_io_queues()' before the tagset is
allocated, as nvme_alloc_io_tag_set() recurses into 
nvme_tcp_init_request(), which requires the queues to be allocated.

So the only 'simple' choice is to not call nvme_tcp_start_io_queues(),
but then all queues are already connected and we're having to tear them
down _anyway_.

Essentially we will just skip sending the 'connect' command for the I/O
queues, everything else will have to stay the same.
Which we can, but then we're not saving that much here.

>> Key observation here is that the _size_ of the I/O tagset is actually
>> defined by the parameters for the connect command; we cannot grow
>> beyond that. Which means that we should be allocate the I/O tagset
>> right at the start, and only modify the number of queue which we _use_
>> based on the information we get back from the controller.
> 
> What does the tagset has to do with connecting the queues?
> 
See above. allocating the tagset recurses into init_request, which 
requires the queues to be allocated.

>> Currently the io tagset allocation is tied to the number of queues
>> for the controller, and trying to modify that is an even worse hack
>> than suppressing the namespace scan.
> 
> I don't think it is... All I'm saying is that if we are connecting 64 io 
> queues
> to just right after reconnect them with tls, we are doing something wrong.
> 

No argument here.

>>
>> Anyway,it's quite a different patchset which is basically unrelated
>> to the secure concatenation work.
> 
> Well, they are required by the secure concatenation, but agree that
> they are preparatory.
> 
>>
>> I do have a preliminary patchset for this if there's interest.
> 
> Again, the fact that we connect io queues, disconnect them and then
> connect them again is backwards I think.
> 
Indeed.
But I'll guess we'll have a discussion at LSF about initial connect 
logic anyway.

Cheers,

Hannes

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

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 94152ded123a..3811ee9cd040 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2219,6 +2219,22 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
 	}
 }
 
+static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
+{
+	if (!ctrl->opts->concat)
+		return false;
+	/*
+	 * If a key has been generated and TLS has not been enabled
+	 * reset the queue to start TLS handshake.
+	 */
+	if (ctrl->opts->tls_key && !ctrl->tls_key) {
+		dev_info(ctrl->device, "Reset to enable TLS with generated PSK\n");
+		nvme_reset_ctrl(ctrl);
+		return true;
+	}
+	return false;
+}
+
 static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
 {
 	if (!ctrl->opts->concat)
@@ -2321,6 +2337,9 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
 	if (nvme_tcp_setup_ctrl(ctrl, false))
 		goto requeue;
 
+	if (nvme_tcp_reset_for_secure_concat(ctrl))
+		return;
+
 	dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
 			ctrl->nr_reconnects);
 
@@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
 	if (nvme_tcp_setup_ctrl(ctrl, false))
 		goto out_fail;
 
+	nvme_tcp_reset_for_secure_concat(ctrl);
 	return;
 
 out_fail:
-- 
2.35.3

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

[Sagi Grimberg]

On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> With TP8018 a new key will be generated from the DH-HMAC-CHAP
> protocol after reset or recovery, but we need to start over
> to establish a new TLS connection with the new keys.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 94152ded123a..3811ee9cd040 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2219,6 +2219,22 @@ static void nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>   	}
>   }
>   
> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
> +{
> +	if (!ctrl->opts->concat)
> +		return false;
> +	/*
> +	 * If a key has been generated and TLS has not been enabled
> +	 * reset the queue to start TLS handshake.
> +	 */
> +	if (ctrl->opts->tls_key && !ctrl->tls_key) {
> +		dev_info(ctrl->device, "Reset to enable TLS with generated PSK\n");
> +		nvme_reset_ctrl(ctrl);
> +		return true;
> +	}
> +	return false;
> +}
> +
>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
>   {
>   	if (!ctrl->opts->concat)
> @@ -2321,6 +2337,9 @@ static void nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>   	if (nvme_tcp_setup_ctrl(ctrl, false))
>   		goto requeue;
>   
> +	if (nvme_tcp_reset_for_secure_concat(ctrl))
> +		return;
> +
>   	dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
>   			ctrl->nr_reconnects);
>   
> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
>   	if (nvme_tcp_setup_ctrl(ctrl, false))
>   		goto out_fail;
>   
> +	nvme_tcp_reset_for_secure_concat(ctrl);

This is really strange. I'd imagine that this would be needed only if 
the derived psk expired no?

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

[Hannes Reinecke]

On 4/7/24 23:49, Sagi Grimberg wrote:
> 
> 
> On 18/03/2024 17:03, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>> protocol after reset or recovery, but we need to start over
>> to establish a new TLS connection with the new keys.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 94152ded123a..3811ee9cd040 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -2219,6 +2219,22 @@ static void nvme_tcp_reconnect_or_remove(struct 
>> nvme_ctrl *ctrl)
>>       }
>>   }
>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
>> +{
>> +    if (!ctrl->opts->concat)
>> +        return false;
>> +    /*
>> +     * If a key has been generated and TLS has not been enabled
>> +     * reset the queue to start TLS handshake.
>> +     */
>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>> +        dev_info(ctrl->device, "Reset to enable TLS with generated 
>> PSK\n");
>> +        nvme_reset_ctrl(ctrl);
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
>>   {
>>       if (!ctrl->opts->concat)
>> @@ -2321,6 +2337,9 @@ static void nvme_tcp_reconnect_ctrl_work(struct 
>> work_struct *work)
>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>           goto requeue;
>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>> +        return;
>> +
>>       dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
>>               ctrl->nr_reconnects);
>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>> work_struct *work)
>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>           goto out_fail;
>> +    nvme_tcp_reset_for_secure_concat(ctrl);
> 
> This is really strange. I'd imagine that this would be needed only if 
> the derived psk expired no?

You are absolutely correct. But once you reset the connection the 
derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
generated a new one.

Remember: for secure concatenation we _always_ have a two-step 
connection setup. The initial connection runs for DH-HMAC-CHAP
to generate the PSK, and then a connection reset to start over
with TLS and the generated PSK.
So upon reset we have to invalidate the generated PSK, reset the
connection to run DH-HMAC-CHAP, and reset _again_ to start over
with the new PSK.

Cheers,

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

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

[Sagi Grimberg]

On 08/04/2024 9:25, Hannes Reinecke wrote:
> On 4/7/24 23:49, Sagi Grimberg wrote:
>>
>>
>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>>> protocol after reset or recovery, but we need to start over
>>> to establish a new TLS connection with the new keys.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>> index 94152ded123a..3811ee9cd040 100644
>>> --- a/drivers/nvme/host/tcp.c
>>> +++ b/drivers/nvme/host/tcp.c
>>> @@ -2219,6 +2219,22 @@ static void 
>>> nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>       }
>>>   }
>>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
>>> +{
>>> +    if (!ctrl->opts->concat)
>>> +        return false;
>>> +    /*
>>> +     * If a key has been generated and TLS has not been enabled
>>> +     * reset the queue to start TLS handshake.
>>> +     */
>>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>>> +        dev_info(ctrl->device, "Reset to enable TLS with generated 
>>> PSK\n");
>>> +        nvme_reset_ctrl(ctrl);
>>> +        return true;
>>> +    }
>>> +    return false;
>>> +}
>>> +
>>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
>>>   {
>>>       if (!ctrl->opts->concat)
>>> @@ -2321,6 +2337,9 @@ static void 
>>> nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>           goto requeue;
>>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>>> +        return;
>>> +
>>>       dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
>>>               ctrl->nr_reconnects);
>>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>>> work_struct *work)
>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>           goto out_fail;
>>> +    nvme_tcp_reset_for_secure_concat(ctrl);
>>
>> This is really strange. I'd imagine that this would be needed only if 
>> the derived psk expired no?
>
> You are absolutely correct. But once you reset the connection the 
> derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
> generated a new one.
>
> Remember: for secure concatenation we _always_ have a two-step 
> connection setup. The initial connection runs for DH-HMAC-CHAP
> to generate the PSK, and then a connection reset to start over
> with TLS and the generated PSK.
> So upon reset we have to invalidate the generated PSK, reset the
> connection to run DH-HMAC-CHAP, and reset _again_ to start over
> with the new PSK.

So what is the point of setting the derived psk with a timeout?

Seems rather silly doing that _every_ reset/reconnect... But ok...

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

[Hannes Reinecke]

On 4/8/24 23:23, Sagi Grimberg wrote:
> 
> 
> On 08/04/2024 9:25, Hannes Reinecke wrote:
>> On 4/7/24 23:49, Sagi Grimberg wrote:
>>>
>>>
>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>> From: Hannes Reinecke <hare@suse.de>
>>>>
>>>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>>>> protocol after reset or recovery, but we need to start over
>>>> to establish a new TLS connection with the new keys.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>> ---
>>>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>>>   1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>> index 94152ded123a..3811ee9cd040 100644
>>>> --- a/drivers/nvme/host/tcp.c
>>>> +++ b/drivers/nvme/host/tcp.c
>>>> @@ -2219,6 +2219,22 @@ static void 
>>>> nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>>       }
>>>>   }
>>>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
>>>> +{
>>>> +    if (!ctrl->opts->concat)
>>>> +        return false;
>>>> +    /*
>>>> +     * If a key has been generated and TLS has not been enabled
>>>> +     * reset the queue to start TLS handshake.
>>>> +     */
>>>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>>>> +        dev_info(ctrl->device, "Reset to enable TLS with generated 
>>>> PSK\n");
>>>> +        nvme_reset_ctrl(ctrl);
>>>> +        return true;
>>>> +    }
>>>> +    return false;
>>>> +}
>>>> +
>>>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl *ctrl)
>>>>   {
>>>>       if (!ctrl->opts->concat)
>>>> @@ -2321,6 +2337,9 @@ static void 
>>>> nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>           goto requeue;
>>>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>>>> +        return;
>>>> +
>>>>       dev_info(ctrl->device, "Successfully reconnected (%d attempt)\n",
>>>>               ctrl->nr_reconnects);
>>>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>>>> work_struct *work)
>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>           goto out_fail;
>>>> +    nvme_tcp_reset_for_secure_concat(ctrl);
>>>
>>> This is really strange. I'd imagine that this would be needed only if 
>>> the derived psk expired no?
>>
>> You are absolutely correct. But once you reset the connection the 
>> derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
>> generated a new one.
>>
>> Remember: for secure concatenation we _always_ have a two-step 
>> connection setup. The initial connection runs for DH-HMAC-CHAP
>> to generate the PSK, and then a connection reset to start over
>> with TLS and the generated PSK.
>> So upon reset we have to invalidate the generated PSK, reset the
>> connection to run DH-HMAC-CHAP, and reset _again_ to start over
>> with the new PSK.
> 
> So what is the point of setting the derived psk with a timeout?
> 
> Seems rather silly doing that _every_ reset/reconnect... But ok...

Mandated by TP8018. Generated TLS PSK should be renewed after a
certain time. Makes sense in general, but still a pain.

Cheers,

Hannes

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

[Sagi Grimberg]

On 09/04/2024 1:10, Hannes Reinecke wrote:
> On 4/8/24 23:23, Sagi Grimberg wrote:
>>
>>
>> On 08/04/2024 9:25, Hannes Reinecke wrote:
>>> On 4/7/24 23:49, Sagi Grimberg wrote:
>>>>
>>>>
>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>> From: Hannes Reinecke <hare@suse.de>
>>>>>
>>>>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>>>>> protocol after reset or recovery, but we need to start over
>>>>> to establish a new TLS connection with the new keys.
>>>>>
>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>> ---
>>>>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>>>>   1 file changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>>> index 94152ded123a..3811ee9cd040 100644
>>>>> --- a/drivers/nvme/host/tcp.c
>>>>> +++ b/drivers/nvme/host/tcp.c
>>>>> @@ -2219,6 +2219,22 @@ static void 
>>>>> nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>>>       }
>>>>>   }
>>>>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
>>>>> +{
>>>>> +    if (!ctrl->opts->concat)
>>>>> +        return false;
>>>>> +    /*
>>>>> +     * If a key has been generated and TLS has not been enabled
>>>>> +     * reset the queue to start TLS handshake.
>>>>> +     */
>>>>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>>>>> +        dev_info(ctrl->device, "Reset to enable TLS with 
>>>>> generated PSK\n");
>>>>> +        nvme_reset_ctrl(ctrl);
>>>>> +        return true;
>>>>> +    }
>>>>> +    return false;
>>>>> +}
>>>>> +
>>>>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl 
>>>>> *ctrl)
>>>>>   {
>>>>>       if (!ctrl->opts->concat)
>>>>> @@ -2321,6 +2337,9 @@ static void 
>>>>> nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>           goto requeue;
>>>>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>>>>> +        return;
>>>>> +
>>>>>       dev_info(ctrl->device, "Successfully reconnected (%d 
>>>>> attempt)\n",
>>>>>               ctrl->nr_reconnects);
>>>>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>>>>> work_struct *work)
>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>           goto out_fail;
>>>>> +    nvme_tcp_reset_for_secure_concat(ctrl);
>>>>
>>>> This is really strange. I'd imagine that this would be needed only 
>>>> if the derived psk expired no?
>>>
>>> You are absolutely correct. But once you reset the connection the 
>>> derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
>>> generated a new one.
>>>
>>> Remember: for secure concatenation we _always_ have a two-step 
>>> connection setup. The initial connection runs for DH-HMAC-CHAP
>>> to generate the PSK, and then a connection reset to start over
>>> with TLS and the generated PSK.
>>> So upon reset we have to invalidate the generated PSK, reset the
>>> connection to run DH-HMAC-CHAP, and reset _again_ to start over
>>> with the new PSK.
>>
>> So what is the point of setting the derived psk with a timeout?
>>
>> Seems rather silly doing that _every_ reset/reconnect... But ok...
>
> Mandated by TP8018. Generated TLS PSK should be renewed after a
> certain time. Makes sense in general, but still a pain.

I understand that psk can expire, I don't understand why we need to 
reset every
single time. What if I'm doing the loop: while true; do nvme reset 
/dev/nvme0; done?

Will it do 2 resets every time?

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

[Hannes Reinecke]

On 4/21/24 13:22, Sagi Grimberg wrote:
> 
> 
> On 09/04/2024 1:10, Hannes Reinecke wrote:
>> On 4/8/24 23:23, Sagi Grimberg wrote:
>>>
>>>
>>> On 08/04/2024 9:25, Hannes Reinecke wrote:
>>>> On 4/7/24 23:49, Sagi Grimberg wrote:
>>>>>
>>>>>
>>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>>> From: Hannes Reinecke <hare@suse.de>
>>>>>>
>>>>>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>>>>>> protocol after reset or recovery, but we need to start over
>>>>>> to establish a new TLS connection with the new keys.
>>>>>>
>>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>>> ---
>>>>>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>>>>>   1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>>>> index 94152ded123a..3811ee9cd040 100644
>>>>>> --- a/drivers/nvme/host/tcp.c
>>>>>> +++ b/drivers/nvme/host/tcp.c
>>>>>> @@ -2219,6 +2219,22 @@ static void 
>>>>>> nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>>>>       }
>>>>>>   }
>>>>>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl *ctrl)
>>>>>> +{
>>>>>> +    if (!ctrl->opts->concat)
>>>>>> +        return false;
>>>>>> +    /*
>>>>>> +     * If a key has been generated and TLS has not been enabled
>>>>>> +     * reset the queue to start TLS handshake.
>>>>>> +     */
>>>>>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>>>>>> +        dev_info(ctrl->device, "Reset to enable TLS with 
>>>>>> generated PSK\n");
>>>>>> +        nvme_reset_ctrl(ctrl);
>>>>>> +        return true;
>>>>>> +    }
>>>>>> +    return false;
>>>>>> +}
>>>>>> +
>>>>>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl 
>>>>>> *ctrl)
>>>>>>   {
>>>>>>       if (!ctrl->opts->concat)
>>>>>> @@ -2321,6 +2337,9 @@ static void 
>>>>>> nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>>           goto requeue;
>>>>>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>>>>>> +        return;
>>>>>> +
>>>>>>       dev_info(ctrl->device, "Successfully reconnected (%d 
>>>>>> attempt)\n",
>>>>>>               ctrl->nr_reconnects);
>>>>>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>>>>>> work_struct *work)
>>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>>           goto out_fail;
>>>>>> +    nvme_tcp_reset_for_secure_concat(ctrl);
>>>>>
>>>>> This is really strange. I'd imagine that this would be needed only 
>>>>> if the derived psk expired no?
>>>>
>>>> You are absolutely correct. But once you reset the connection the 
>>>> derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
>>>> generated a new one.
>>>>
>>>> Remember: for secure concatenation we _always_ have a two-step 
>>>> connection setup. The initial connection runs for DH-HMAC-CHAP
>>>> to generate the PSK, and then a connection reset to start over
>>>> with TLS and the generated PSK.
>>>> So upon reset we have to invalidate the generated PSK, reset the
>>>> connection to run DH-HMAC-CHAP, and reset _again_ to start over
>>>> with the new PSK.
>>>
>>> So what is the point of setting the derived psk with a timeout?
>>>
>>> Seems rather silly doing that _every_ reset/reconnect... But ok...
>>
>> Mandated by TP8018. Generated TLS PSK should be renewed after a
>> certain time. Makes sense in general, but still a pain.
> 
> I understand that psk can expire, I don't understand why we need to 
> reset every
> single time. What if I'm doing the loop: while true; do nvme reset 
> /dev/nvme0; done?
> 
> Will it do 2 resets every time?

For secure concatenation: yes.

Secure concatenation consists on running DH-HMAC-CHAP, generate a TLS 
PSK from the generated credentials, and then start TLS with the 
generated TLS PSK. This is run for every connection attempt, so each
connection attempt gets a new TLS PSK; after the connection is reset
the old PSK _cannot_ be reused, and we need to clear them up via
some sort of garbage collection. By adding an expiration time to the
key we achieve exactly this.
And that is why we need to reset twice; we need to run the DH-HMAC-CHAP
protocol _after_ recovery, and use the generated keys to establish a
TLS connection. And as the TLS connection is established before the
initial connect is sent we need to reset the connection after the
DH-HMAC-CHAP protocol is run (as this is run _after_ the connect command).

Cheers,

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

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

[Sagi Grimberg]

On 21/04/2024 17:37, Hannes Reinecke wrote:
> On 4/21/24 13:22, Sagi Grimberg wrote:
>>
>>
>> On 09/04/2024 1:10, Hannes Reinecke wrote:
>>> On 4/8/24 23:23, Sagi Grimberg wrote:
>>>>
>>>>
>>>> On 08/04/2024 9:25, Hannes Reinecke wrote:
>>>>> On 4/7/24 23:49, Sagi Grimberg wrote:
>>>>>>
>>>>>>
>>>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>>>> From: Hannes Reinecke <hare@suse.de>
>>>>>>>
>>>>>>> With TP8018 a new key will be generated from the DH-HMAC-CHAP
>>>>>>> protocol after reset or recovery, but we need to start over
>>>>>>> to establish a new TLS connection with the new keys.
>>>>>>>
>>>>>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>>>>>> ---
>>>>>>>   drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
>>>>>>>   1 file changed, 20 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>>>>>>> index 94152ded123a..3811ee9cd040 100644
>>>>>>> --- a/drivers/nvme/host/tcp.c
>>>>>>> +++ b/drivers/nvme/host/tcp.c
>>>>>>> @@ -2219,6 +2219,22 @@ static void 
>>>>>>> nvme_tcp_reconnect_or_remove(struct nvme_ctrl *ctrl)
>>>>>>>       }
>>>>>>>   }
>>>>>>> +static bool nvme_tcp_reset_for_secure_concat(struct nvme_ctrl 
>>>>>>> *ctrl)
>>>>>>> +{
>>>>>>> +    if (!ctrl->opts->concat)
>>>>>>> +        return false;
>>>>>>> +    /*
>>>>>>> +     * If a key has been generated and TLS has not been enabled
>>>>>>> +     * reset the queue to start TLS handshake.
>>>>>>> +     */
>>>>>>> +    if (ctrl->opts->tls_key && !ctrl->tls_key) {
>>>>>>> +        dev_info(ctrl->device, "Reset to enable TLS with 
>>>>>>> generated PSK\n");
>>>>>>> +        nvme_reset_ctrl(ctrl);
>>>>>>> +        return true;
>>>>>>> +    }
>>>>>>> +    return false;
>>>>>>> +}
>>>>>>> +
>>>>>>>   static void nvme_tcp_revoke_generated_tls_key(struct nvme_ctrl 
>>>>>>> *ctrl)
>>>>>>>   {
>>>>>>>       if (!ctrl->opts->concat)
>>>>>>> @@ -2321,6 +2337,9 @@ static void 
>>>>>>> nvme_tcp_reconnect_ctrl_work(struct work_struct *work)
>>>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>>>           goto requeue;
>>>>>>> +    if (nvme_tcp_reset_for_secure_concat(ctrl))
>>>>>>> +        return;
>>>>>>> +
>>>>>>>       dev_info(ctrl->device, "Successfully reconnected (%d 
>>>>>>> attempt)\n",
>>>>>>>               ctrl->nr_reconnects);
>>>>>>> @@ -2396,6 +2415,7 @@ static void nvme_reset_ctrl_work(struct 
>>>>>>> work_struct *work)
>>>>>>>       if (nvme_tcp_setup_ctrl(ctrl, false))
>>>>>>>           goto out_fail;
>>>>>>> +    nvme_tcp_reset_for_secure_concat(ctrl);
>>>>>>
>>>>>> This is really strange. I'd imagine that this would be needed 
>>>>>> only if the derived psk expired no?
>>>>>
>>>>> You are absolutely correct. But once you reset the connection the 
>>>>> derived PSK of the previous connection _is_ expired as DH-HMAC-CHAP
>>>>> generated a new one.
>>>>>
>>>>> Remember: for secure concatenation we _always_ have a two-step 
>>>>> connection setup. The initial connection runs for DH-HMAC-CHAP
>>>>> to generate the PSK, and then a connection reset to start over
>>>>> with TLS and the generated PSK.
>>>>> So upon reset we have to invalidate the generated PSK, reset the
>>>>> connection to run DH-HMAC-CHAP, and reset _again_ to start over
>>>>> with the new PSK.
>>>>
>>>> So what is the point of setting the derived psk with a timeout?
>>>>
>>>> Seems rather silly doing that _every_ reset/reconnect... But ok...
>>>
>>> Mandated by TP8018. Generated TLS PSK should be renewed after a
>>> certain time. Makes sense in general, but still a pain.
>>
>> I understand that psk can expire, I don't understand why we need to 
>> reset every
>> single time. What if I'm doing the loop: while true; do nvme reset 
>> /dev/nvme0; done?
>>
>> Will it do 2 resets every time?
>
> For secure concatenation: yes.
>
> Secure concatenation consists on running DH-HMAC-CHAP, generate a TLS 
> PSK from the generated credentials, and then start TLS with the 
> generated TLS PSK. This is run for every connection attempt, so each
> connection attempt gets a new TLS PSK; after the connection is reset
> the old PSK _cannot_ be reused, and we need to clear them up via
> some sort of garbage collection. By adding an expiration time to the
> key we achieve exactly this.
> And that is why we need to reset twice; we need to run the DH-HMAC-CHAP
> protocol _after_ recovery, and use the generated keys to establish a
> TLS connection. And as the TLS connection is established before the
> initial connect is sent we need to reset the connection after the
> DH-HMAC-CHAP protocol is run (as this is run _after_ the connect 
> command).

Got it.

[PATCH 14/17] nvmet-auth: allow to clear DH-HMAC-CHAP keys

[Hannes Reinecke]

As we can set DH-HMAC-CHAP keys, we should also be
able to unset them.

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

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 3ddbc3880cac..7327fdd46147 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -25,6 +25,18 @@ int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
 	unsigned char key_hash;
 	char *dhchap_secret;
 
+	if (!strlen(secret)) {
+		if (set_ctrl) {
+			kfree(host->dhchap_ctrl_secret);
+			host->dhchap_ctrl_secret = NULL;
+			host->dhchap_ctrl_key_hash = 0;
+		} else {
+			kfree(host->dhchap_secret);
+			host->dhchap_secret = NULL;
+			host->dhchap_key_hash = 0;
+		}
+		return 0;
+	}
 	if (sscanf(secret, "DHHC-1:%hhd:%*s", &key_hash) != 1)
 		return -EINVAL;
 	if (key_hash > 3) {
-- 
2.35.3

Re: [PATCH 14/17] nvmet-auth: allow to clear DH-HMAC-CHAP keys

[Sagi Grimberg]

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

[PATCH 15/17] nvme-target: do not check authentication status for admin commands twice

[Hannes Reinecke]

nvmet_check_ctrl_status() checks the authentication status, so
we don't need to do that prior to calling it.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/admin-cmd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index f5b7054a4a05..936194de4a52 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -1005,8 +1005,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 
 	if (nvme_is_fabrics(cmd))
 		return nvmet_parse_fabrics_admin_cmd(req);
-	if (unlikely(!nvmet_check_auth_status(req)))
-		return NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
+
 	if (nvmet_is_disc_subsys(nvmet_req_subsys(req)))
 		return nvmet_parse_discovery_cmd(req);
 
-- 
2.35.3

Re: [PATCH 15/17] nvme-target: do not check authentication status for admin commands twice

[Sagi Grimberg]

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

[PATCH 16/17] nvme-target: do not check authentication status for I/O commands twice

[Hannes Reinecke]

nvmet_check_ctrl_status() checks the authentication status, so
we don't need to do that prior to calling it.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/target/core.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 6bbe4df0166c..1d1a7026e817 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -891,9 +891,6 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 	if (nvme_is_fabrics(cmd))
 		return nvmet_parse_fabrics_io_cmd(req);
 
-	if (unlikely(!nvmet_check_auth_status(req)))
-		return NVME_SC_AUTH_REQUIRED | NVME_SC_DNR;
-
 	ret = nvmet_check_ctrl_status(req);
 	if (unlikely(ret))
 		return ret;
-- 
2.35.3

Re: [PATCH 16/17] nvme-target: do not check authentication status for I/O commands twice

[Sagi Grimberg]

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

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

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

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

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

diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 7327fdd46147..cb5592025289 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -15,6 +15,7 @@
 #include <linux/ctype.h>
 #include <linux/random.h>
 #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
 #include <asm/unaligned.h>
 
 #include "nvmet.h"
@@ -136,7 +137,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;
@@ -163,6 +164,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");
@@ -234,6 +240,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);
@@ -262,6 +271,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)
@@ -541,3 +556,56 @@ 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_put(sq->ctrl->tls_key);
+	sq->ctrl->tls_key = tls_key;
+#endif
+
+out_free_digest:
+	kfree_sensitive(digest);
+out_free_psk:
+	kfree_sensitive(psk);
+}
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index eb7785be0ca7..fe7f4b3adb77 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -43,8 +43,26 @@ static u16 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
 		 data->auth_protocol[0].dhchap.halen,
 		 data->auth_protocol[0].dhchap.dhlen);
 	req->sq->dhchap_tid = le16_to_cpu(data->t_id);
-	if (data->sc_c)
-		return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+	if (data->sc_c != NVME_AUTH_SECP_NOSC) {
+		if (!IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
+			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+		/* Secure concatenation can only be enabled on the admin queue */
+		if (req->sq->qid)
+			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+		switch (data->sc_c) {
+		case NVME_AUTH_SECP_NEWTLSPSK:
+			if (nvmet_queue_is_tls(req->sq))
+				return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+			break;
+		case NVME_AUTH_SECP_REPLACETLSPSK:
+			if (!nvmet_queue_is_tls(req->sq))
+				return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+			break;
+		default:
+			return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+		}
+		ctrl->concat = true;
+	}
 
 	if (data->napd != 1)
 		return NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE;
@@ -103,6 +121,13 @@ static u16 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
 			 nvme_auth_dhgroup_name(fallback_dhgid));
 		ctrl->dh_gid = fallback_dhgid;
 	}
+	if (ctrl->dh_gid == NVME_AUTH_DHGROUP_NULL &&
+	    ctrl->concat) {
+		pr_debug("%s: ctrl %d qid %d: NULL DH group invalid "
+			 "for secure channel concatenation\n", __func__,
+			 ctrl->cntlid, req->sq->qid);
+		return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+	}
 	pr_debug("%s: ctrl %d qid %d: selected DH group %s (%d)\n",
 		 __func__, ctrl->cntlid, req->sq->qid,
 		 nvme_auth_dhgroup_name(ctrl->dh_gid), ctrl->dh_gid);
@@ -154,6 +179,12 @@ static u16 nvmet_auth_reply(struct nvmet_req *req, void *d)
 	kfree(response);
 	pr_debug("%s: ctrl %d qid %d host authenticated\n",
 		 __func__, ctrl->cntlid, req->sq->qid);
+	if (!data->cvalid && ctrl->concat) {
+		pr_debug("%s: ctrl %d qid %d invalid challenge\n",
+			 __func__, ctrl->cntlid, req->sq->qid);
+		return NVME_AUTH_DHCHAP_FAILURE_FAILED;
+	}
+	req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
 	if (data->cvalid) {
 		req->sq->dhchap_c2 = kmemdup(data->rval + data->hl, data->hl,
 					     GFP_KERNEL);
@@ -163,11 +194,14 @@ static u16 nvmet_auth_reply(struct nvmet_req *req, void *d)
 		pr_debug("%s: ctrl %d qid %d challenge %*ph\n",
 			 __func__, ctrl->cntlid, req->sq->qid, data->hl,
 			 req->sq->dhchap_c2);
-	} else {
+	}
+	if (req->sq->dhchap_s2 == 0) {
+		if (ctrl->concat)
+			nvmet_auth_insert_psk(req->sq);
 		req->sq->authenticated = true;
+		kfree(req->sq->dhchap_c2);
 		req->sq->dhchap_c2 = NULL;
 	}
-	req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
 
 	return 0;
 }
@@ -240,7 +274,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 			pr_debug("%s: ctrl %d qid %d reset negotiation\n", __func__,
 				 ctrl->cntlid, req->sq->qid);
 			if (!req->sq->qid) {
-				if (nvmet_setup_auth(ctrl) < 0) {
+				if (nvmet_setup_auth(ctrl, req) < 0) {
 					status = NVME_SC_INTERNAL;
 					pr_err("ctrl %d qid 0 failed to setup"
 					       "re-authentication",
@@ -296,6 +330,8 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 		}
 		goto done_kfree;
 	case NVME_AUTH_DHCHAP_MESSAGE_SUCCESS2:
+		if (ctrl->concat)
+			nvmet_auth_insert_psk(req->sq);
 		req->sq->authenticated = true;
 		pr_debug("%s: ctrl %d qid %d ctrl authenticated\n",
 			 __func__, ctrl->cntlid, req->sq->qid);
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 08e9c6b6f551..5e3c98e1bd6a 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -199,10 +199,20 @@ 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);
+
+	/* Do not authenticate I/O queues for secure concatenation */
+	if (ctrl->concat && req->sq->qid)
+		needs_auth = false;
+
+	pr_debug("%s: ctrl %d qid %d should %sauthenticate, tls psk %08x\n",
+		 __func__, ctrl->cntlid, req->sq->qid,
+		 needs_auth ? "" : "not ",
+		 req->sq->tls_key ? key_serial(req->sq->tls_key) : 0);
 	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)
@@ -254,7 +264,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);
@@ -271,12 +281,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:
@@ -319,6 +330,8 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 	ctrl = nvmet_ctrl_find_get(d->subsysnqn, d->hostnqn,
 				   le16_to_cpu(d->cntlid), req);
 	if (!ctrl) {
+		pr_warn("invalid controller (%x)\n",
+			le16_to_cpu(d->cntlid));
 		status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 		goto out;
 	}
@@ -335,7 +348,7 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
 		goto out_ctrl_put;
 
 	pr_debug("adding queue %d to ctrl %d.\n", qid, ctrl->cntlid);
-	req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl));
+	req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl, req));
 out:
 	kfree(d);
 complete:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 7c6e7e65b032..765da2b932c5 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -121,6 +121,9 @@ struct nvmet_sq {
 	u32			dhchap_s2;
 	u8			*dhchap_skey;
 	int			dhchap_skey_len;
+#endif
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+	struct key		*tls_key;
 #endif
 	struct completion	free_done;
 	struct completion	confirm_done;
@@ -235,6 +238,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;
@@ -244,6 +248,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 {
@@ -707,13 +714,21 @@ 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 bool nvmet_queue_is_tls(struct 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);
@@ -723,16 +738,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;
 }
@@ -745,11 +762,12 @@ static inline bool nvmet_check_auth_status(struct nvmet_req *req)
 {
 	return true;
 }
-static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl)
+static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl,
+				  struct nvmet_req *req)
 {
 	return false;
 }
 static inline const char *nvmet_dhchap_dhgroup_name(u8 dhgid) { return NULL; }
+static inline void nvmet_auth_insert_psk(struct nvmet_sq *sq) {};
 #endif
-
 #endif /* _NVMET_H */
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index c8655fc5aa5b..4718d4d87a85 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1071,10 +1071,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;
@@ -1605,6 +1606,9 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
 	/* stop accepting incoming data */
 	queue->rcv_state = NVMET_TCP_RECV_ERR;
 
+	if (queue->nvme_sq.tls_key)
+		key_put(queue->nvme_sq.tls_key);
+
 	nvmet_tcp_uninit_data_in_cmds(queue);
 	nvmet_sq_destroy(&queue->nvme_sq);
 	cancel_work_sync(&queue->io_work);
@@ -1811,6 +1815,32 @@ 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 = tls_key;
+		}
+	}
 	if (status)
 		nvmet_tcp_schedule_release_queue(queue);
 	else
-- 
2.35.3