nvme: implement secure concatenation

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/nvme/host/fabrics.c | 7 ++++++-
  drivers/nvme/host/sysfs.c   | 9 +++++++--
  drivers/nvme/host/tcp.c     | 8 +++++++-
  3 files changed, 20 insertions(+), 4 deletions(-)

drivers/nvme/host/fabrics.c +0 -0

index 0141c0a6942f..75aa69457353 100644
@@ -639,7 +639,12 @@ static struct key *nvmf_parse_key(int key_id)
  	key = key_lookup(key_id);
  	if (IS_ERR(key))

Quoted 25 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/nvme/host/fabrics.c | 7 ++++++-
  drivers/nvme/host/sysfs.c   | 9 +++++++--
  drivers/nvme/host/tcp.c     | 8 +++++++-

  3 files changed, 20 insertions(+), 4 deletions(-)

drivers/nvme/host/fabrics.c +4 -1

index 0141c0a6942f..75aa69457353 100644
@@ -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);

drivers/nvme/host/fabrics.c static struct key *nvmf_parse_key(int key_id) · line 649

Quoted 4 lines

drivers/nvme/host/fabrics.c +2 -0

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

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Sagi Grimberg <[email protected]>

---
  drivers/nvme/common/auth.c | 76 ++++++++++++++++++++++++++++++++++++++
  include/linux/nvme-auth.h  |  2 +
  2 files changed, 78 insertions(+)

drivers/nvme/common/auth.c +1 -0

index a3455f1d67fa..7a4b6589351d 100644
@@ -11,6 +11,7 @@
  #include <asm/unaligned.h>
  #include <crypto/hash.h>
  #include <crypto/dh.h>
+#include <crypto/hkdf.h>
  #include <linux/nvme.h>
  #include <linux/nvme-auth.h>
 

drivers/nvme/common/auth.c +73 -0

+{
+	struct crypto_shash *tfm;
+	struct shash_desc *shash;
+	u8 *psk;
+	const char *hmac_name;
+	int ret, psk_len;
+
+	if (!c1 || !c2) {
+		pr_warn("%s: invalid parameter\n", __func__);
+		return 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");

include/linux/nvme-auth.h +2 -0

index c1d0bc5d9624..31dc1db2d4d6 100644
@@ -40,5 +40,7 @@ int nvme_auth_gen_pubkey(struct crypto_kpp *dh_tfm,
  int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
  				u8 *ctrl_key, size_t ctrl_key_len,
  				u8 *sess_key, size_t sess_key_len);
+u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
 
  #endif /* _NVME_AUTH_H */

Quoted 24 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Sagi Grimberg <[email protected]>

---
  drivers/nvme/common/auth.c | 76 ++++++++++++++++++++++++++++++++++++++ include/linux/nvme-auth.h  |  2 +

  2 files changed, 78 insertions(+)

drivers/nvme/common/auth.c +1 -0

index a3455f1d67fa..7a4b6589351d 100644
@@ -11,6 +11,7 @@
   #include <asm/unaligned.h>
   #include <crypto/hash.h>
   #include <crypto/dh.h>
+#include <crypto/hkdf.h>
   #include <linux/nvme.h>
   #include <linux/nvme-auth.h>

@@ -471,5 +472,80 @@ int nvme_auth_generate_key(u8 *secret, struct 

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

Quoted 5 lines

drivers/nvme/common/auth.c +2 -0

 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?

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

Reviewed-by Sagi Grimberg <[email protected]>

---
  drivers/nvme/common/auth.c | 105 +++++++++++++++++++++++++++++++++++++
  include/linux/nvme-auth.h  |   2 +
  2 files changed, 107 insertions(+)

drivers/nvme/common/auth.c +0 -0

index 7a4b6589351d..f56f08a4461e 100644

drivers/nvme/common/auth.c +103 -0

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

include/linux/nvme-auth.h +2 -0

index 31dc1db2d4d6..2cbb9249a8b3 100644
@@ -42,5 +42,7 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
  				u8 *sess_key, size_t sess_key_len);
  u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
  			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
+u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+			      char *subsysnqn, char *hostnqn);
 
  #endif /* _NVME_AUTH_H */

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/nvme/common/auth.c | 71 ++++++++++++++++++++++++++++++++++++++
  include/linux/nvme-auth.h  |  1 +
  2 files changed, 72 insertions(+)

drivers/nvme/common/auth.c +0 -0

index f56f08a4461e..90d525afc240 100644

drivers/nvme/common/auth.c +16 -0

+	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",

drivers/nvme/common/auth.c +43 -0

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

include/linux/nvme-auth.h +1 -0

index 2cbb9249a8b3..335236fb2b73 100644
@@ -44,5 +44,6 @@ u8 *nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
  			   u8 *c1, u8 *c2, size_t hash_len, size_t *ret_len);
  u8 *nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
  			      char *subsysnqn, char *hostnqn);
+u8 *nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len, u8 *psk_digest);
 
  #endif /* _NVME_AUTH_H */

Quoted 10 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/nvme/common/auth.c | 71 ++++++++++++++++++++++++++++++++++++++ include/linux/nvme-auth.h  |  1 +

  2 files changed, 72 insertions(+)

drivers/nvme/common/auth.c u8 *nvme_auth_generate_digest(u8 hmac_id, u8 · line 657

Quoted 11 lines

drivers/nvme/common/auth.c +2 -0

index f56f08a4461e..90d525afc240 100644
@@ -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.

Quoted 20 lines

drivers/nvme/common/auth.c +20 -0

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

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

Quoted 6 lines

drivers/nvme/common/auth.c +6 -0

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

From: Hannes Reinecke <[email protected]>

There is a difference between TLS configured (ie the user has provisioned/requested a key) and TLS enabled (ie the connection is encrypted with TLS). 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 <[email protected]>

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

drivers/nvme/host/core.c +0 -1

index 2baf5786a92f..9b601655f423 100644
@@ -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);

drivers/nvme/host/sysfs.c +1 -1

index ec581608f16c..1f9e57fbfee3 100644
@@ -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;

drivers/nvme/host/tcp.c +41 -13

index 4a58886e1354..7018dc0dd026 100644
@@ -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,

Quoted 8 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

There is a difference between TLS configured (ie the user has provisioned/requested a key) and TLS enabled (ie the connection is encrypted with TLS). 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?

Quoted 4 lines

So to differentiate between those two states store the provisioned key in opts->tls_key (as we're using the same TLS key for all queues) and 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 ?

Quoted 12 lines · Sagi Grimberg

Quoted 8 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

There is a difference between TLS configured (ie the user has provisioned/requested a key) and TLS enabled (ie the connection is encrypted with TLS). 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).

Quoted 2 lines

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

Quoted 10 lines

Quoted 5 lines

So to differentiate between those two states store the provisioned key in opts->tls_key (as we're using the same TLS key for all queues) and 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 :-)

Quoted 13 lines · Hannes Reinecke

Quoted 12 lines · Sagi Grimberg

Quoted 8 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

There is a difference between TLS configured (ie the user has provisioned/requested a key) and TLS enabled (ie the connection is encrypted with TLS). 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).

Quoted 2 lines

Quoted 2 lines

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

Can you answer this?

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

So to differentiate between those two states store the provisioned key in opts->tls_key (as we're using the same TLS key for all queues) and 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 ?

Quoted 7 lines

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.

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

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/nvme/host/tcp.c | 1 -
  1 file changed, 1 deletion(-)

drivers/nvme/host/tcp.c +0 -0

index 7018dc0dd026..66675b2dc197 100644

Quoted 20 lines · 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 <[email protected]>

---
  drivers/nvme/host/tcp.c | 1 -

  1 file changed, 1 deletion(-)

drivers/nvme/host/tcp.c +0 -1

index 7018dc0dd026..66675b2dc197 100644
@@ -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);

Quoted 1 line

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

From: Hannes Reinecke <[email protected]>

Add a fabrics option 'concat' to request secure channel concatenation.

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

Signed-off-by Hannes Reinecke <[email protected]>

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

drivers/nvme/host/auth.c +68 -3

index a264b3ae078b..a4033a8f9607 100644
@@ -12,6 +12,7 @@
  #include "nvme.h"
  #include "fabrics.h"
  #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
 
  #define CHAP_BUF_SIZE 4096
  static struct kmem_cache *nvme_chap_buf_cache;

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

drivers/nvme/host/auth.c +26 -1

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

drivers/nvme/host/fabrics.c +0 -0

index 75aa69457353..ae091e0e4ecf 100644

drivers/nvme/host/fabrics.c +2 -0

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

drivers/nvme/host/fabrics.c +10 -0

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

drivers/nvme/host/fabrics.h +3 -0

index 06cc54851b1b..accfc7228366 100644
@@ -73,6 +73,7 @@ enum {
  	NVMF_OPT_TLS		= 1 << 25,
  	NVMF_OPT_KEYRING	= 1 << 26,
  	NVMF_OPT_TLS_KEY	= 1 << 27,
+	NVMF_OPT_CONCAT		= 1 << 28,
  };
 
  /**

@@ -108,6 +109,7 @@ enum {
   * @keyring:    Keyring to use for key lookups
   * @tls_key:    TLS key for encrypted connections (TCP)
   * @tls:        Start TLS encrypted connections (TCP)
+ * @concat:     Enabled Secure channel concatenation (TCP)
   * @disable_sqflow: disable controller sq flow control
   * @hdr_digest: generate/verify header digest (TCP)
   * @data_digest: generate/verify data digest (TCP)

@@ -137,6 +139,7 @@ struct nvmf_ctrl_options {
  	struct key		*keyring;
  	struct key		*tls_key;
  	bool			tls;
+	bool			concat;
  	bool			disable_sqflow;
  	bool			hdr_digest;
  	bool			data_digest;

drivers/nvme/host/sysfs.c +2 -0

index 07ec26de2d91..edd96f501d8a 100644
@@ -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;

drivers/nvme/host/tcp.c +47 -6

index 66675b2dc197..94152ded123a 100644
@@ -219,7 +219,7 @@ static inline bool nvme_tcp_tls_configured(struct nvme_ctrl *ctrl)
  	if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
  		return 0;
 
-	return ctrl->opts->tls;
+	return ctrl->opts->tls || ctrl->opts->concat;
  }
 
  static inline struct blk_mq_tags *nvme_tcp_tagset(struct nvme_tcp_queue *queue)

@@ -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,
  };
 

include/linux/nvme.h +7 -0

index ce0c1143c7e4..b29310424ee1 100644
@@ -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;
  };

Quoted 3 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

Add a fabrics option 'concat' to request secure channel concatenation.

maybe name it secconcat ? or secure_concat ?

Quoted 2 lines

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.

Quoted 123 lines

Signed-off-by Hannes Reinecke <[email protected]>

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

drivers/nvme/host/auth.c +7 -1

index a264b3ae078b..a4033a8f9607 100644
@@ -12,6 +12,7 @@
   #include "nvme.h"
   #include "fabrics.h"
   #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
   #define CHAP_BUF_SIZE 4096
   static struct kmem_cache *nvme_chap_buf_cache;

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

drivers/nvme/host/auth.c +2 -1

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

drivers/nvme/host/auth.c +4 -1

@@ -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",

drivers/nvme/host/auth.c +55 -0

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

drivers/nvme/host/auth.c static void nvme_auth_free_dhchap(struct · line 756

Quoted 10 lines

drivers/nvme/host/auth.c +10 -0

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

Quoted 52 lines

drivers/nvme/host/auth.c +8 -0

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

drivers/nvme/host/auth.c +12 -1

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

drivers/nvme/host/auth.c +6 -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);

drivers/nvme/host/fabrics.c +0 -0

index 75aa69457353..ae091e0e4ecf 100644

drivers/nvme/host/fabrics.c int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl) · line 470

Quoted 10 lines

drivers/nvme/host/fabrics.c +3 -2

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

Quoted 25 lines

drivers/nvme/host/fabrics.c +5 -0

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

drivers/nvme/host/fabrics.c static int nvmf_parse_options(struct nvmf_ctrl_options *opts, · line 1042

Quoted 5 lines

drivers/nvme/host/fabrics.c +5 -0

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

Quoted 180 lines

          default:
              pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n", p);

drivers/nvme/host/fabrics.c +10 -0

@@ -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)) {

drivers/nvme/host/fabrics.h +3 -0

index 06cc54851b1b..accfc7228366 100644
@@ -73,6 +73,7 @@ enum {
       NVMF_OPT_TLS        = 1 << 25,
       NVMF_OPT_KEYRING    = 1 << 26,
       NVMF_OPT_TLS_KEY    = 1 << 27,
+    NVMF_OPT_CONCAT        = 1 << 28,
   };
   /**

@@ -108,6 +109,7 @@ enum {
    * @keyring:    Keyring to use for key lookups
    * @tls_key:    TLS key for encrypted connections (TCP)
    * @tls:        Start TLS encrypted connections (TCP)
+ * @concat:     Enabled Secure channel concatenation (TCP)
    * @disable_sqflow: disable controller sq flow control
    * @hdr_digest: generate/verify header digest (TCP)
    * @data_digest: generate/verify data digest (TCP)

@@ -137,6 +139,7 @@ struct nvmf_ctrl_options {
       struct key        *keyring;
       struct key        *tls_key;
       bool            tls;
+    bool            concat;
       bool            disable_sqflow;
       bool            hdr_digest;
       bool            data_digest;

drivers/nvme/host/sysfs.c +2 -0

index 07ec26de2d91..edd96f501d8a 100644
@@ -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;

drivers/nvme/host/tcp.c +2 -2

index 66675b2dc197..94152ded123a 100644
@@ -219,7 +219,7 @@ static inline bool nvme_tcp_tls_configured(struct 
 nvme_ctrl *ctrl)
       if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
           return 0;
-    return ctrl->opts->tls;
+    return ctrl->opts->tls || ctrl->opts->concat;
   }
   static inline struct blk_mq_tags *nvme_tcp_tagset(struct nvme_tcp_queue *queue)

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

drivers/nvme/host/tcp.c +40 -3

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

drivers/nvme/host/tcp.c +3 -0

@@ -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,
  };

include/linux/nvme.h +2 -0

index ce0c1143c7e4..b29310424ee1 100644
@@ -1663,6 +1663,13 @@ enum {
       NVME_AUTH_DHGROUP_INVALID    = 0xff,
   };
+enum {
+    NVME_AUTH_SECP_NOSC        = 0x00,

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

Quoted 8 lines

include/linux/nvme.h +5 -0

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

Quoted 12 lines · Sagi Grimberg

Quoted 3 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

Add a fabrics option 'concat' to request secure channel concatenation.

maybe name it secconcat ? or secure_concat ?

Quoted 2 lines

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

Quoted 3 lines

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.

Quoted 131 lines

Quoted 131 lines

Signed-off-by Hannes Reinecke <[email protected]>

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

drivers/nvme/host/auth.c +7 -1

index a264b3ae078b..a4033a8f9607 100644
@@ -12,6 +12,7 @@
   #include "nvme.h"
   #include "fabrics.h"
   #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
   #define CHAP_BUF_SIZE 4096
   static struct kmem_cache *nvme_chap_buf_cache;

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

drivers/nvme/host/auth.c +2 -1

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

drivers/nvme/host/auth.c +4 -1

@@ -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",

drivers/nvme/host/auth.c +61 -0

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

Quoted 8 lines

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

Quoted 4 lines

drivers/nvme/host/auth.c +4 -0

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

Quoted 13 lines · Hannes Reinecke

Quoted 12 lines · Sagi Grimberg

Quoted 3 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

Add a fabrics option 'concat' to request secure channel concatenation.

maybe name it secconcat ? or secure_concat ?

Quoted 2 lines

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 131 lines

Quoted 131 lines

Quoted 131 lines

Signed-off-by Hannes Reinecke <[email protected]>

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

drivers/nvme/host/auth.c +7 -1

index a264b3ae078b..a4033a8f9607 100644
@@ -12,6 +12,7 @@
   #include "nvme.h"
   #include "fabrics.h"
   #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
   #define CHAP_BUF_SIZE 4096
   static struct kmem_cache *nvme_chap_buf_cache;

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

drivers/nvme/host/auth.c +2 -1

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

drivers/nvme/host/auth.c +4 -1

@@ -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",

drivers/nvme/host/auth.c +61 -0

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

Quoted 8 lines

Quoted 8 lines

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

Quoted 4 lines

drivers/nvme/host/auth.c +4 -0

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

Quoted 168 lines · Sagi Grimberg

Quoted 13 lines · Hannes Reinecke

Quoted 12 lines · Sagi Grimberg

Quoted 3 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

Add a fabrics option 'concat' to request secure channel concatenation.

maybe name it secconcat ? or secure_concat ?

Quoted 2 lines

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 140 lines

Quoted 140 lines

Quoted 126 lines

Signed-off-by Hannes Reinecke <[email protected]>

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

drivers/nvme/host/auth.c +7 -1

index a264b3ae078b..a4033a8f9607 100644
@@ -12,6 +12,7 @@
   #include "nvme.h"
   #include "fabrics.h"
   #include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
   #define CHAP_BUF_SIZE 4096
   static struct kmem_cache *nvme_chap_buf_cache;

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

drivers/nvme/host/auth.c +2 -1

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

drivers/nvme/host/auth.c +4 -1

@@ -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",

drivers/nvme/host/auth.c +56 -0

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

drivers/nvme/host/auth.c static void nvme_auth_free_dhchap(struct · line 757

Quoted 10 lines

drivers/nvme/host/auth.c +9 -0

+             "%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?

Quoted 2 lines

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.

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.

Quoted 11 lines · 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.

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

Quoted 2 lines

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?

Quoted 15 lines · Sagi Grimberg

Quoted 11 lines · 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.

Quoted 3 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 27 lines · Hannes Reinecke

Quoted 15 lines · Sagi Grimberg

Quoted 11 lines · 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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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?

Quoted 1 line

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.

Quoted 1 line

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

Where exactly?

Quoted 3 lines

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

it connects io_queues if ctrl->queue_count > 1

Quoted 4 lines

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.

Quoted 33 lines · Sagi Grimberg

Quoted 29 lines · Hannes Reinecke

Quoted 15 lines · Sagi Grimberg

Quoted 11 lines · 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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 39 lines · Hannes Reinecke

Quoted 38 lines · Sagi Grimberg

Quoted 27 lines · Hannes Reinecke

Quoted 15 lines · Sagi Grimberg

Quoted 11 lines · 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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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?

Quoted 5 lines

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?

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

---
  drivers/nvme/host/tcp.c | 20 ++++++++++++++++++++
  1 file changed, 20 insertions(+)

drivers/nvme/host/tcp.c +19 -0

index 94152ded123a..3811ee9cd040 100644
@@ -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);
 

Quoted 47 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

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

  1 file changed, 20 insertions(+)

drivers/nvme/host/tcp.c +16 -0

index 94152ded123a..3811ee9cd040 100644
@@ -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)

drivers/nvme/host/tcp.c +3 -0

@@ -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",

drivers/nvme/host/tcp.c static void nvme_reset_ctrl_work(struct · line 2418

Quoted 6 lines

drivers/nvme/host/tcp.c +1 -0

               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?

Quoted 50 lines · Sagi Grimberg

Quoted 47 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

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

  1 file changed, 20 insertions(+)

drivers/nvme/host/tcp.c +16 -0

index 94152ded123a..3811ee9cd040 100644
@@ -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)

drivers/nvme/host/tcp.c +3 -0

@@ -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",

Quoted 8 lines

drivers/nvme/host/tcp.c static void nvme_reset_ctrl_work(struct · line 2418

Quoted 6 lines

drivers/nvme/host/tcp.c +1 -0

               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.

Quoted 63 lines · Hannes Reinecke

Quoted 50 lines · Sagi Grimberg

Quoted 47 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>

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

Signed-off-by Hannes Reinecke <[email protected]>

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

  1 file changed, 20 insertions(+)

drivers/nvme/host/tcp.c +16 -0

index 94152ded123a..3811ee9cd040 100644
@@ -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)

drivers/nvme/host/tcp.c +3 -0

@@ -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",

Quoted 8 lines

drivers/nvme/host/tcp.c static void nvme_reset_ctrl_work(struct · line 2418

Quoted 6 lines

drivers/nvme/host/tcp.c +1 -0

               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.

Quoted 6 lines

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.