nvme: fixes for secure concatenation

PATCH Superseded Hannes Reinecke 5 participants 23 messages Jul 22, 2024 – Aug 13, 2024

Subsystems (3)

NVMe NVMe target NVM express fabrics authentication

lore

Share & tools

lore

Hannes Reinecke

author

2024-07-22 12:02 UTC #0 lore raw

v8 cover nvme: fixes for secure concatenation

From: Hannes Reinecke <[email protected]>

Hi all,

here's a list of fixes split off from the secure concatenation patchset as they really are unrelated and just are assorted fixes to get things rolling.
The most important here is the first patch, which implements TP8018 to support the 'version 1' format for TLS PSK identifiers. And it also updates the sysfs interface to allow us to re-construct the nvme-cli commandline from sysfs attributes.

As usual, comments and reviews are welcome.

Changes to v7:
- Include reviews from Sagi

Changes to v6:
- Include reviews from Christoph
- Add patch to split off tls attributes into a separate group

Hannes Reinecke (9):
nvme-keyring: restrict match length for version '1' identifiers
nvme-tcp: sanitize TLS key handling
nvme-tcp: check for invalidated or revoked key
nvme: add a newline to the 'tls_key' sysfs attribute
nvme: split off TLS sysfs attributes into a separate group
nvme-sysfs: add 'tls_configured_key' sysfs attribute
nvme-sysfs: add 'tls_keyring' attribute
nvmet-auth: allow to clear DH-HMAC-CHAP keys
nvme-target: do not check authentication status for admin commands
twice

 drivers/nvme/common/keyring.c   | 58 +++++++++++++++++----
 drivers/nvme/host/core.c        |  1 -
 drivers/nvme/host/fabrics.c     |  2 +-
 drivers/nvme/host/nvme.h        |  2 +-
 drivers/nvme/host/sysfs.c       | 90 +++++++++++++++++++++++++--------
 drivers/nvme/host/tcp.c         | 55 +++++++++++++++-----
 drivers/nvme/target/admin-cmd.c |  2 -
 drivers/nvme/target/auth.c      | 12 +++++
 include/linux/nvme-keyring.h    |  6 ++-
 9 files changed, 177 insertions(+), 51 deletions(-)

Signature · 1 line

2.35.3

Hannes Reinecke

author

2024-07-22 12:02 UTC #1 lore raw

PATCH v8 1/9 nvme-keyring: restrict match length for version '1' identifiers

TP8018 introduced a new TLS PSK identifier version (version 1), which appended a PSK hash value to the existing identifier (cf NVMe TCP specification v1.1, section 3.6.1.3 'TLS PSK and PSK Identity Derivation').
An original (version 0) identifier has the form:

NVMe0<type><hmac> <hostnqn> <subsysnqn>

and a version 1 identifier has the form:

NVMe1<type><hmac> <hostnqn> <subsysnqn> <hash>

This patch modifies the lookup algorthm to compare only the first part of the identifier (excluding the hash value) to handle both version 0 and version 1 identifiers.
And the spec declares 'version 0' identifiers obsolete, so the lookup algorithm is modified to prever v1 identifiers.

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

Reviewed-by Sagi Grimberg <[email protected]>

---
 drivers/nvme/common/keyring.c | 36 +++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

drivers/nvme/common/keyring.c +26 ~~-10~~

index 6f7e7a8fa5ae..05e89307c8aa 100644
@@ -36,14 +36,12 @@ static bool nvme_tls_psk_match(const struct key *key,
 		pr_debug("%s: no key description\n", __func__);
 		return false;
 	}
-	match_len = strlen(key->description);
-	pr_debug("%s: id %s len %zd\n", __func__, key->description, match_len);
-
 	if (!match_data->raw_data) {
 		pr_debug("%s: no match data\n", __func__);
 		return false;
 	}
 	match_id = match_data->raw_data;
+	match_len = strlen(match_id);
 	pr_debug("%s: match '%s' '%s' len %zd\n",
 		 __func__, match_id, key->description, match_len);
 	return !memcmp(key->description, match_id, match_len);

@@ -71,7 +69,7 @@ static struct key_type nvme_tls_psk_key_type = {
 
 static struct key *nvme_tls_psk_lookup(struct key *keyring,
 		const char *hostnqn, const char *subnqn,
-		int hmac, bool generated)
+		u8 hmac, u8 psk_ver, bool generated)
 {
 	char *identity;
 	size_t identity_len = (NVMF_NQN_SIZE) * 2 + 11;

@@ -82,8 +80,8 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
 	if (!identity)
 		return ERR_PTR(-ENOMEM);
 
-	snprintf(identity, identity_len, "NVMe0%c%02d %s %s",
-		 generated ? 'G' : 'R', hmac, hostnqn, subnqn);
+	snprintf(identity, identity_len, "NVMe%u%c%02u %s %s",
+		 psk_ver, generated ? 'G' : 'R', hmac, hostnqn, subnqn);
 
 	if (!keyring)
 		keyring = nvme_keyring;

@@ -107,21 +105,38 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
 /*
  * NVMe PSK priority list
  *
- * 'Retained' PSKs (ie 'generated == false')
- * should be preferred to 'generated' PSKs,
- * and SHA-384 should be preferred to SHA-256.
+ * 'Retained' PSKs (ie 'generated == false') should be preferred to 'generated'
+ * PSKs, PSKs with hash (psk_ver 1) should be preferred to PSKs without hash
+ * (psk_ver 0), and SHA-384 should be preferred to SHA-256.
  */
 static struct nvme_tls_psk_priority_list {
 	bool generated;
+	u8 psk_ver;
 	enum nvme_tcp_tls_cipher cipher;
 } nvme_tls_psk_prio[] = {
 	{ .generated = false,
+	  .psk_ver = 1,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
+	{ .generated = false,
+	  .psk_ver = 1,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
+	{ .generated = false,
+	  .psk_ver = 0,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
 	{ .generated = false,
+	  .psk_ver = 0,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
+	{ .generated = true,
+	  .psk_ver = 1,
+	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
+	{ .generated = true,
+	  .psk_ver = 1,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
 	{ .generated = true,
+	  .psk_ver = 0,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
 	{ .generated = true,
+	  .psk_ver = 0,
 	  .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
 };

@@ -137,10 +152,11 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 
 	for (prio = 0; prio < ARRAY_SIZE(nvme_tls_psk_prio); prio++) {
 		bool generated = nvme_tls_psk_prio[prio].generated;
+		u8 ver = nvme_tls_psk_prio[prio].psk_ver;
 		enum nvme_tcp_tls_cipher cipher = nvme_tls_psk_prio[prio].cipher;
 
 		tls_key = nvme_tls_psk_lookup(keyring, hostnqn, subnqn,
-					      cipher, generated);
+					      cipher, ver, generated);
 		if (!IS_ERR(tls_key)) {
 			tls_key_id = tls_key->serial;
 			key_put(tls_key);

Signature · 1 line

2.35.3

Christoph Hellwig

maintainer Reviewed-by on v8 1/9

2024-07-23 14:43 UTC #2 lore raw

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

Looks good:

Reviewed-by Christoph Hellwig <[email protected]>

Hannes Reinecke

author

2024-07-22 12:02 UTC #3 lore raw

PATCH v8 2/9 nvme-tcp: sanitize TLS key handling

In reply to Hannes Reinecke

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 on an unencrypted connection (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 generated key in opts->tls_key (as we're using the same TLS key for all queues), the key serial of the resulting TLS handshake in ctrl->tls_pskid (to signal that TLS on the admin queue is enabled), and a simple flag for the queues to indicated that TLS has been enabled.

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

Reviewed-by Christoph Hellwig <[email protected]>

---
 drivers/nvme/host/core.c  |  1 -
 drivers/nvme/host/nvme.h  |  2 +-
 drivers/nvme/host/sysfs.c |  4 +--
 drivers/nvme/host/tcp.c   | 53 +++++++++++++++++++++++++++++----------
 4 files changed, 43 insertions(+), 17 deletions(-)

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

index e78ef31eeef0..663fa14162ab 100644
@@ -4675,7 +4675,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);
 	cleanup_srcu_struct(&ctrl->srcu);

drivers/nvme/host/nvme.h +1 -1

index 9ac587e64166..185fb986a5cc 100644
@@ -373,7 +373,7 @@ struct nvme_ctrl {
 	struct nvme_dhchap_key *ctrl_key;
 	u16 transaction;
 #endif
-	struct key *tls_key;
+	key_serial_t tls_pskid;
 
 	/* Power saving configuration */
 	u64 ps_max_latency_us;

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

index ba05faaac562..72675b59a7a7 100644
@@ -670,9 +670,9 @@ static ssize_t tls_key_show(struct device *dev,
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
 
-	if (!ctrl->tls_key)
+	if (!ctrl->tls_pskid)
 		return 0;
-	return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
+	return sysfs_emit(buf, "%08x", ctrl->tls_pskid);
 }
 static DEVICE_ATTR_RO(tls_key);
 #endif

drivers/nvme/host/tcp.c +40 ~~-13~~

129 lines not on this page · raw message

Signature · 1 line

2.35.3

Hannes Reinecke

author

2024-07-22 12:02 UTC #4 lore raw

PATCH v8 3/9 nvme-tcp: check for invalidated or revoked key

In reply to Hannes Reinecke

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

Reviewed-by Sagi Grimberg <[email protected]>

---
 drivers/nvme/common/keyring.c | 22 ++++++++++++++++++++++
 drivers/nvme/host/fabrics.c   |  2 +-
 drivers/nvme/host/tcp.c       |  2 +-
 include/linux/nvme-keyring.h  |  6 +++++-
 4 files changed, 29 insertions(+), 3 deletions(-)

drivers/nvme/common/keyring.c +22 -0

index 05e89307c8aa..ed5167f942d8 100644
@@ -20,6 +20,28 @@ key_serial_t nvme_keyring_id(void)
 }
 EXPORT_SYMBOL_GPL(nvme_keyring_id);
 
+static bool nvme_tls_psk_revoked(struct key *psk)
+{
+	return test_bit(KEY_FLAG_REVOKED, &psk->flags) ||
+		test_bit(KEY_FLAG_INVALIDATED, &psk->flags);
+}
+
+struct key *nvme_tls_key_lookup(key_serial_t key_id)
+{
+	struct key *key = key_lookup(key_id);
+
+	if (IS_ERR(key)) {
+		pr_err("key id %08x not found\n", key_id);
+		return key;
+	}
+	if (nvme_tls_psk_revoked(key)) {
+		pr_err("key id %08x revoked\n", key_id);
+		return ERR_PTR(-EKEYREVOKED);
+	}
+	return key;
+}
+EXPORT_SYMBOL_GPL(nvme_tls_key_lookup);
+
 static void nvme_tls_psk_describe(const struct key *key, struct seq_file *m)
 {
 	seq_puts(m, key->description);

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

index f5f545fa0103..432efcbf9e2f 100644
@@ -665,7 +665,7 @@ static struct key *nvmf_parse_key(int key_id)
 		return ERR_PTR(-EINVAL);
 	}
 
-	key = key_lookup(key_id);
+	key = nvme_tls_key_lookup(key_id);
 	if (IS_ERR(key))
 		pr_err("key id %08x not found\n", key_id);
 	else

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

index b305873e588e..e3d82e91151a 100644
@@ -1596,7 +1596,7 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
 		goto out_complete;
 	}
 
-	tls_key = key_lookup(pskid);
+	tls_key = nvme_tls_key_lookup(pskid);
 	if (IS_ERR(tls_key)) {
 		dev_warn(ctrl->ctrl.device, "queue %d: Invalid key %x\n",
 			 qid, pskid);

include/linux/nvme-keyring.h +5 -1

index e10333d78dbb..19d2b256180f 100644
@@ -12,7 +12,7 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
 		const char *hostnqn, const char *subnqn);
 
 key_serial_t nvme_keyring_id(void);
-
+struct key *nvme_tls_key_lookup(key_serial_t key_id);
 #else
 
 static inline key_serial_t nvme_tls_psk_default(struct key *keyring,

@@ -24,5 +24,9 @@ static inline key_serial_t nvme_keyring_id(void)
 {
 	return 0;
 }
+static inline struct key *nvme_tls_key_lookup(key_serial_t key_id)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
 #endif /* !CONFIG_NVME_KEYRING */
 #endif /* _NVME_KEYRING_H */

Signature · 1 line

2.35.3

Christoph Hellwig

maintainer Reviewed-by on v8 3/9

2024-07-23 14:44 UTC #5 lore raw

Re: [PATCH 3/9] nvme-tcp: check for invalidated or revoked key

Looks good:

Reviewed-by Christoph Hellwig <[email protected]>

Keith Busch

maintainer

2024-07-29 14:56 UTC #6 lore raw

Re: [PATCH 3/9] nvme-tcp: check for invalidated or revoked key

In reply to Hannes Reinecke

Quoted 38 lines · Hannes Reinecke

drivers/nvme/common/keyring.c +22 -0

index 05e89307c8aa..ed5167f942d8 100644
@@ -20,6 +20,28 @@ key_serial_t nvme_keyring_id(void)
 }
 EXPORT_SYMBOL_GPL(nvme_keyring_id);
 
+static bool nvme_tls_psk_revoked(struct key *psk)
+{
+	return test_bit(KEY_FLAG_REVOKED, &psk->flags) ||
+		test_bit(KEY_FLAG_INVALIDATED, &psk->flags);
+}
+
+struct key *nvme_tls_key_lookup(key_serial_t key_id)
+{
+	struct key *key = key_lookup(key_id);
+
+	if (IS_ERR(key)) {
+		pr_err("key id %08x not found\n", key_id);
+		return key;
+	}
+	if (nvme_tls_psk_revoked(key)) {
+		pr_err("key id %08x revoked\n", key_id);
+		return ERR_PTR(-EKEYREVOKED);
+	}
+	return key;
+}
+EXPORT_SYMBOL_GPL(nvme_tls_key_lookup);
+
 static void nvme_tls_psk_describe(const struct key *key, struct seq_file *m)
 {
 	seq_puts(m, key->description);

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

index f5f545fa0103..432efcbf9e2f 100644

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

Quoted 5 lines

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

@@ -665,7 +665,7 @@ static struct key *nvmf_parse_key(int key_id)
 		return ERR_PTR(-EINVAL);
 	}
 
-	key = key_lookup(key_id);
+	key = nvme_tls_key_lookup(key_id);

We've had some fallout before with nvme modules vs built-in, so I test for this now. Here's the relevant parts of my config:

CONFIG_NVME_KEYRING=m
...
CONFIG_NVME_FABRICS=y
...
CONFIG_NVME_TCP=m

And that gets this error:

vmlinux.o: in function `nvmf_parse_key':
/home/kbusch/src/linux/drivers/nvme/host/fabrics.c:668: undefined reference to `nvme_tls_key_lookup'

Sagi Grimberg

maintainer

2024-07-31 09:48 UTC #7 lore raw

Re: [PATCH 3/9] nvme-tcp: check for invalidated or revoked key

Quoted 23 lines

Quoted 9 lines
  static void nvme_tls_psk_describe(const struct key *key, struct seq_file *m)
  {
  	seq_puts(m, key->description);
drivers/nvme/host/fabrics.c +0 -0
index f5f545fa0103..432efcbf9e2f 100644
drivers/nvme/host/fabrics.c static struct key *nvmf_parse_key(int key_id) · line 668
Quoted 5 lines
drivers/nvme/host/fabrics.c +1 -1
@@ -665,7 +665,7 @@ static struct key *nvmf_parse_key(int key_id)
  		return ERR_PTR(-EINVAL);
  	}
 
-	key = key_lookup(key_id);
+	key = nvme_tls_key_lookup(key_id);
We've had some fallout before with nvme modules vs built-in, so I test for this now. Here's the relevant parts of my config:
CONFIG_NVME_KEYRING=m
...
CONFIG_NVME_FABRICS=y
...
CONFIG_NVME_TCP=m

Quoted 4 lines

And that gets this error:
vmlinux.o: in function `nvmf_parse_key':
/home/kbusch/src/linux/drivers/nvme/host/fabrics.c:668: undefined reference to `nvme_tls_key_lookup'

Hannes, can you take look.

Hannes Reinecke

maintainer

2024-08-12 06:25 UTC #8 lore raw

Re: [PATCH 3/9] nvme-tcp: check for invalidated or revoked key

Quoted 22 lines · Sagi Grimberg

Quoted 22 lines
drivers/nvme/host/fabrics.c static struct key *nvmf_parse_key(int key_id) · line 667
Quoted 13 lines
static void nvme_tls_psk_describe(const struct key *key, struct seq_file *m) {
seq_puts(m, key->description);
drivers/nvme/host/fabrics.c +1 -1
index f5f545fa0103..432efcbf9e2f 100644
@@ -665,7 +665,7 @@ static struct key *nvmf_parse_key(int key_id)
           return ERR_PTR(-EINVAL);
       }
-    key = key_lookup(key_id);
+    key = nvme_tls_key_lookup(key_id);
We've had some fallout before with nvme modules vs built-in, so I test for this now. Here's the relevant parts of my config:
CONFIG_NVME_KEYRING=m
...
CONFIG_NVME_FABRICS=y
...
CONFIG_NVME_TCP=m

Quoted 7 lines

Quoted 4 lines
And that gets this error:
vmlinux.o: in function `nvmf_parse_key':
/home/kbusch/src/linux/drivers/nvme/host/fabrics.c:668: undefined reference to `nvme_tls_key_lookup'
Hannes, can you take look.

Just back from vacation, but yeah, I'll take a look.

Cheers,

Hannes

Signature · 4 lines

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

Hannes Reinecke

maintainer

2024-08-12 14:09 UTC #9 lore raw

Re: [PATCH 3/9] nvme-tcp: check for invalidated or revoked key

Quoted 23 lines · Hannes Reinecke

Quoted 22 lines · Sagi Grimberg
Quoted 22 lines
drivers/nvme/host/fabrics.c static struct key *nvmf_parse_key(int key_id) · line 667
Quoted 13 lines
static void nvme_tls_psk_describe(const struct key *key, struct seq_file *m) {
seq_puts(m, key->description);
drivers/nvme/host/fabrics.c +1 -1
index f5f545fa0103..432efcbf9e2f 100644
@@ -665,7 +665,7 @@ static struct key *nvmf_parse_key(int key_id)
           return ERR_PTR(-EINVAL);
       }
-    key = key_lookup(key_id);
+    key = nvme_tls_key_lookup(key_id);
We've had some fallout before with nvme modules vs built-in, so I test for this now. Here's the relevant parts of my config:
CONFIG_NVME_KEYRING=m
...
CONFIG_NVME_FABRICS=y
...
CONFIG_NVME_TCP=m

Quoted 9 lines

Quoted 7 lines
Quoted 4 lines
And that gets this error:
vmlinux.o: in function `nvmf_parse_key':
/home/kbusch/src/linux/drivers/nvme/host/fabrics.c:668: undefined reference to `nvme_tls_key_lookup'
Hannes, can you take look.
Just back from vacation, but yeah, I'll take a look.

Should be fixed with:

drivers/nvme/host/Kconfig +1 -0

index a3caef75aa0a..883aaab2d83e 100644
@@ -109,6 +109,7 @@ config NVME_HOST_AUTH
        bool "NVMe over Fabrics In-Band Authentication in host side"
        depends on NVME_CORE
        select NVME_AUTH
+       select NVME_KEYRING if NVME_TCP_TLS
        help
          This provides support for NVMe over Fabrics In-Band

Authentication in
host side.

Will send an updated series.

Cheers,

Hannes

Signature · 4 lines

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

Keith Busch

maintainer

2024-08-12 15:17 UTC #10 lore raw

Re: [PATCH 3/9] nvme-tcp: check for invalidated or revoked key

Quoted 16 lines · Hannes Reinecke

Should be fixed with:

drivers/nvme/host/Kconfig +1 -0

index a3caef75aa0a..883aaab2d83e 100644
@@ -109,6 +109,7 @@ config NVME_HOST_AUTH
        bool "NVMe over Fabrics In-Band Authentication in host side"
        depends on NVME_CORE
        select NVME_AUTH
+       select NVME_KEYRING if NVME_TCP_TLS
        help
          This provides support for NVMe over Fabrics In-Band Authentication

in
host side.

Quoted 1 line

Will send an updated series.

If this is the only change you have, no need to resend. Looks good on my end, I've pushed this out to the nvme tree a few minutes ago.

Sagi Grimberg

maintainer

2024-08-13 19:30 UTC #11 lore raw

Re: [PATCH 3/9] nvme-tcp: check for invalidated or revoked key

Quoted 17 lines · Keith Busch

Quoted 16 lines · Hannes Reinecke

Should be fixed with:

drivers/nvme/host/Kconfig +1 -0

index a3caef75aa0a..883aaab2d83e 100644
@@ -109,6 +109,7 @@ config NVME_HOST_AUTH
         bool "NVMe over Fabrics In-Band Authentication in host side"
         depends on NVME_CORE
         select NVME_AUTH
+       select NVME_KEYRING if NVME_TCP_TLS
         help
           This provides support for NVMe over Fabrics In-Band Authentication

in
host side.

Quoted 3 lines

Quoted 1 line
Will send an updated series.
If this is the only change you have, no need to resend. Looks good on my end, I've pushed this out to the nvme tree a few minutes ago.

Nice.

Hannes Reinecke

author

2024-07-22 12:02 UTC #12 lore raw

PATCH v8 4/9 nvme: add a newline to the 'tls_key' sysfs attribute

In reply to Hannes Reinecke

Print a newline for easier userspace handling.

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

Reviewed-by Sagi Grimberg <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 drivers/nvme/host/sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 72675b59a7a7..c391ad6c5a88 100644
@@ -672,7 +672,7 @@ static ssize_t tls_key_show(struct device *dev,
 
 	if (!ctrl->tls_pskid)
 		return 0;
-	return sysfs_emit(buf, "%08x", ctrl->tls_pskid);
+	return sysfs_emit(buf, "%08x\n", ctrl->tls_pskid);
 }
 static DEVICE_ATTR_RO(tls_key);
 #endif

Signature · 1 line

2.35.3

Hannes Reinecke

author

2024-07-22 12:02 UTC #13 lore raw

PATCH v8 5/9 nvme: split off TLS sysfs attributes into a separate group

In reply to Hannes Reinecke

From: Hannes Reinecke <[email protected]>

Split off TLS sysfs attributes into a separate group to improve readability and to keep all TLS related handling in one section.

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

Reviewed-by Sagi Grimberg <[email protected]>

---
 drivers/nvme/host/sysfs.c | 62 ++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 21 deletions(-)

drivers/nvme/host/sysfs.c +41 ~~-21~~

index c391ad6c5a88..dc7ceb53147f 100644
@@ -664,19 +664,6 @@ static DEVICE_ATTR(dhchap_ctrl_secret, S_IRUGO | S_IWUSR,
 	nvme_ctrl_dhchap_ctrl_secret_show, nvme_ctrl_dhchap_ctrl_secret_store);
 #endif
 
-#ifdef CONFIG_NVME_TCP_TLS
-static ssize_t tls_key_show(struct device *dev,
-			    struct device_attribute *attr, char *buf)
-{
-	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
-
-	if (!ctrl->tls_pskid)
-		return 0;
-	return sysfs_emit(buf, "%08x\n", ctrl->tls_pskid);
-}
-static DEVICE_ATTR_RO(tls_key);
-#endif
-
 static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_reset_controller.attr,
 	&dev_attr_rescan_controller.attr,

@@ -703,9 +690,6 @@ static struct attribute *nvme_dev_attrs[] = {
 #ifdef CONFIG_NVME_HOST_AUTH
 	&dev_attr_dhchap_secret.attr,
 	&dev_attr_dhchap_ctrl_secret.attr,
-#endif
-#ifdef CONFIG_NVME_TCP_TLS
-	&dev_attr_tls_key.attr,
 #endif
 	&dev_attr_adm_passthru_err_log_enabled.attr,
 	NULL

@@ -737,11 +721,6 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
 	if (a == &dev_attr_dhchap_ctrl_secret.attr && !ctrl->opts)
 		return 0;
 #endif
-#ifdef CONFIG_NVME_TCP_TLS
-	if (a == &dev_attr_tls_key.attr &&
-	    (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
-		return 0;
-#endif
 
 	return a->mode;
 }

@@ -752,8 +731,49 @@ const struct attribute_group nvme_dev_attrs_group = {
 };
 EXPORT_SYMBOL_GPL(nvme_dev_attrs_group);
 
+#ifdef CONFIG_NVME_TCP_TLS
+static ssize_t tls_key_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	if (!ctrl->tls_pskid)
+		return 0;
+	return sysfs_emit(buf, "%08x\n", ctrl->tls_pskid);
+}
+static DEVICE_ATTR_RO(tls_key);
+
+static struct attribute *nvme_tls_attrs[] = {
+	&dev_attr_tls_key.attr,
+};
+
+static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,
+		struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	if (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp"))
+		return 0;
+
+	if (a == &dev_attr_tls_key.attr &&
+	    !ctrl->opts->tls)
+		return 0;
+
+	return a->mode;
+}
+
+const struct attribute_group nvme_tls_attrs_group = {
+	.attrs		= nvme_tls_attrs,
+	.is_visible	= nvme_tls_attrs_are_visible,
+};
+#endif
+
 const struct attribute_group *nvme_dev_attr_groups[] = {
 	&nvme_dev_attrs_group,
+#ifdef CONFIG_NVME_TCP_TLS
+	&nvme_tls_attrs_group,
+#endif
 	NULL,
 };

Signature · 1 line

2.35.3

Christoph Hellwig

maintainer

2024-07-23 14:49 UTC #14 lore raw

Re: [PATCH 5/9] nvme: split off TLS sysfs attributes into a separate group

drivers/nvme/host/sysfs.c const struct attribute_group nvme_dev_attrs_group = { · line 760

Quoted 11 lines · Hannes Reinecke

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

+static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,
+		struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	if (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp"))
+		return 0;
+
+	if (a == &dev_attr_tls_key.attr &&
+	    !ctrl->opts->tls)

As-is there is only a single attribute and you don't need the check for the specific attribute. And the point of making this a separate group is that we'll want to not have that check in the future as well, right?

I would have also kinda expected that this attribute group lives in tcp.c, is there a good reason to keep it in the core code?

Hannes Reinecke

maintainer

2024-07-23 17:30 UTC #15 lore raw

Re: [PATCH 5/9] nvme: split off TLS sysfs attributes into a separate group

Quoted 18 lines · Christoph Hellwig

drivers/nvme/host/sysfs.c const struct attribute_group nvme_dev_attrs_group = { · line 760
Quoted 11 lines · Hannes Reinecke
drivers/nvme/host/sysfs.c +11 -0
+static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,
+		struct attribute *a, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	if (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp"))
+		return 0;
+
+	if (a == &dev_attr_tls_key.attr &&
+	    !ctrl->opts->tls)
As-is there is only a single attribute and you don't need the check for the specific attribute. And the point of making this a separate group is that we'll want to not have that check in the future as well, right?

Correct. It'll be filled out with the next two patches.

Quoted 3 lines

I would have also kinda expected that this attribute group lives in tcp.c, is there a good reason to keep it in the core code?

I knew this argument would be coming.
Thing is: these options depend on CONFIG_NVME_TCP_TLS, so they will always be encapsulated in some #ifdef, be it in sysfs.c or in tcp.c.

And as we always will have to have an #ifdef in one of the files I thought it easier to keep it in sysfs.c, and save us having to have an exported attribute_group which might even be empty.

But if you insist ...

Cheers,

Hannes

Signature · 5 lines

Dr. Hannes Reinecke                Kernel Storage Architect
[email protected]                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich

Sagi Grimberg

maintainer

2024-07-23 18:17 UTC #16 lore raw

Re: [PATCH 5/9] nvme: split off TLS sysfs attributes into a separate group

Quoted 8 lines

Quoted 3 lines
I would have also kinda expected that this attribute group lives in tcp.c, is there a good reason to keep it in the core code?
I knew this argument would be coming.
Thing is: these options depend on CONFIG_NVME_TCP_TLS, so they will always be encapsulated in some #ifdef, be it in sysfs.c or in tcp.c.

Quoted 5 lines

And as we always will have to have an #ifdef in one of the files I thought it easier to keep it in sysfs.c, and save us having to have an exported attribute_group which might even be empty.
But if you insist ...

I think that tls awareness has already leaked to core code (for good reasons I think), so at this point I'm fine either way....

Christoph Hellwig

maintainer

2024-07-24 13:41 UTC #17 lore raw

Re: [PATCH 5/9] nvme: split off TLS sysfs attributes into a separate group

In reply to Hannes Reinecke

Quoted 7 lines · Hannes Reinecke

Quoted 3 lines
I would have also kinda expected that this attribute group lives in tcp.c, is there a good reason to keep it in the core code?
I knew this argument would be coming.
Thing is: these options depend on CONFIG_NVME_TCP_TLS, so they will always be encapsulated in some #ifdef, be it in sysfs.c or in tcp.c.

I don't really mind the ifdefs per se. It's just that we'll have this code built into nvme-core.ko for common kernel setups where no one uses nvme-tcp at all.

Quoted 3 lines

And as we always will have to have an #ifdef in one of the files I thought it easier to keep it in sysfs.c, and save us having to have an exported attribute_group which might even be empty.

I'm ok with this series of fixes going forward without it for now, but in the long run I'd like to keep the core code as lean as possible.

Hannes Reinecke

author

2024-07-22 12:02 UTC #18 lore raw

PATCH v8 6/9 nvme-sysfs: add 'tls_configured_key' sysfs attribute

In reply to Hannes Reinecke

There is a difference between the negotiated TLS key (which is always present for a TLS encrypted connection) and the configured TLS key (which is specified with the --tls_key command line option). To differentate between these two add a new sysfs attribute 'tls_configured_key' to hold the specified on the command line.

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

Reviewed-by Sagi Grimberg <[email protected]>

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

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

index dc7ceb53147f..2055dad7bc63 100644
@@ -743,8 +743,19 @@ static ssize_t tls_key_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(tls_key);
 
+static ssize_t tls_configured_key_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	struct key *key = ctrl->opts->tls_key;
+
+	return sysfs_emit(buf, "%08x\n", key_serial(key));
+}
+static DEVICE_ATTR_RO(tls_configured_key);
+
 static struct attribute *nvme_tls_attrs[] = {
 	&dev_attr_tls_key.attr,
+	&dev_attr_tls_configured_key.attr,
 };
 
 static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,

@@ -759,6 +770,9 @@ static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,
 	if (a == &dev_attr_tls_key.attr &&
 	    !ctrl->opts->tls)
 		return 0;
+	if (a == &dev_attr_tls_configured_key.attr &&
+	    !ctrl->opts->tls_key)
+		return 0;
 
 	return a->mode;
 }

Signature · 1 line

2.35.3

Hannes Reinecke

author

2024-07-22 12:02 UTC #19 lore raw

PATCH v8 7/9 nvme-sysfs: add 'tls_keyring' attribute

In reply to Hannes Reinecke

Add a 'tls_keyring' attribute to display the contents of the --keyring option from the connect string. Adding this attribute allows us to recreate the original connect string from sysfs settings.

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

Reviewed-by Sagi Grimberg <[email protected]>

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

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

index 2055dad7bc63..eb345551d6fe 100644
@@ -753,9 +753,20 @@ static ssize_t tls_configured_key_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(tls_configured_key);
 
+static ssize_t tls_keyring_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	struct key *keyring = ctrl->opts->keyring;
+
+	return sysfs_emit(buf, "%s\n", keyring->description);
+}
+static DEVICE_ATTR_RO(tls_keyring);
+
 static struct attribute *nvme_tls_attrs[] = {
 	&dev_attr_tls_key.attr,
 	&dev_attr_tls_configured_key.attr,
+	&dev_attr_tls_keyring.attr,
 };
 
 static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,

@@ -773,6 +784,9 @@ static umode_t nvme_tls_attrs_are_visible(struct kobject *kobj,
 	if (a == &dev_attr_tls_configured_key.attr &&
 	    !ctrl->opts->tls_key)
 		return 0;
+	if (a == &dev_attr_tls_keyring.attr &&
+	    !ctrl->opts->keyring)
+		return 0;
 
 	return a->mode;
 }

Signature · 1 line

2.35.3

Hannes Reinecke

author

2024-07-22 12:02 UTC #20 lore raw

PATCH v8 8/9 nvmet-auth: allow to clear DH-HMAC-CHAP keys

In reply to Hannes Reinecke

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

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

Reviewed-by Sagi Grimberg <[email protected]>

---
 drivers/nvme/target/auth.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/nvme/target/auth.c +12 -0

index 8bc3f431c77f..7897d02c681d 100644
@@ -25,6 +25,18 @@ int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
 	unsigned char key_hash;
 	char *dhchap_secret;
 
+	if (!strlen(secret)) {
+		if (set_ctrl) {
+			kfree(host->dhchap_ctrl_secret);
+			host->dhchap_ctrl_secret = NULL;
+			host->dhchap_ctrl_key_hash = 0;
+		} else {
+			kfree(host->dhchap_secret);
+			host->dhchap_secret = NULL;
+			host->dhchap_key_hash = 0;
+		}
+		return 0;
+	}
 	if (sscanf(secret, "DHHC-1:%hhd:%*s", &key_hash) != 1)
 		return -EINVAL;
 	if (key_hash > 3) {

Signature · 1 line

2.35.3

Hannes Reinecke

author

2024-07-22 12:02 UTC #21 lore raw

PATCH v8 9/9 nvme-target: do not check authentication status for admin commands twice

In reply to Hannes Reinecke

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

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

Reviewed-by Sagi Grimberg <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 drivers/nvme/target/admin-cmd.c | 2 --
 1 file changed, 2 deletions(-)

drivers/nvme/target/admin-cmd.c +0 -2

index f7e1156ac7ec..d64480f01f4a 100644
@@ -1005,8 +1005,6 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 
 	if (nvme_is_fabrics(cmd))
 		return nvmet_parse_fabrics_admin_cmd(req);
-	if (unlikely(!nvmet_check_auth_status(req)))
-		return NVME_SC_AUTH_REQUIRED | NVME_STATUS_DNR;
 	if (nvmet_is_disc_subsys(nvmet_req_subsys(req)))
 		return nvmet_parse_discovery_cmd(req);

Signature · 1 line

2.35.3

Sagi Grimberg

maintainer

2024-07-28 20:50 UTC #22 lore raw

In reply to Hannes Reinecke

Quoted 12 lines · Hannes Reinecke

From: Hannes Reinecke <[email protected]>
Hi all,
here's a list of fixes split off from the secure concatenation patchset as they really are unrelated and just are assorted fixes to get things rolling.
The most important here is the first patch, which implements TP8018 to support the 'version 1' format for TLS PSK identifiers. And it also updates the sysfs interface to allow us to re-construct the nvme-cli commandline from sysfs attributes.

Quoted 1 line

As usual, comments and reviews are welcome.

Keith, for clarification, I think that both Christoph and I are good with this being included.