* [PATCH 01/16] nvme-keyring: restrict match length for version '1' identifiers
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 21:47 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 02/16] nvme-tcp: sanitize TLS key handling Hannes Reinecke
` (15 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
TP8018 changed the TLS PSK identifiers to append a PSK hash value,
so to lookup identifiers we should just consider the length of
the match value, not the length of the identifiers to compare
against.
And we should modify the PSK lookup algorithm to prefer v1 identifiers
as they can be uniquely identified.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
drivers/nvme/common/keyring.c | 32 +++++++++++++++++++++++++-------
1 file changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index 6f7e7a8fa5ae..c60ebbdc52b8 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -36,14 +36,12 @@ static bool nvme_tls_psk_match(const struct key *key,
pr_debug("%s: no key description\n", __func__);
return false;
}
- match_len = strlen(key->description);
- pr_debug("%s: id %s len %zd\n", __func__, key->description, match_len);
-
if (!match_data->raw_data) {
pr_debug("%s: no match data\n", __func__);
return false;
}
match_id = match_data->raw_data;
+ match_len = strlen(match_id);
pr_debug("%s: match '%s' '%s' len %zd\n",
__func__, match_id, key->description, match_len);
return !memcmp(key->description, match_id, match_len);
@@ -71,7 +69,7 @@ static struct key_type nvme_tls_psk_key_type = {
static struct key *nvme_tls_psk_lookup(struct key *keyring,
const char *hostnqn, const char *subnqn,
- int hmac, bool generated)
+ u8 hmac, u8 psk_ver, bool generated)
{
char *identity;
size_t identity_len = (NVMF_NQN_SIZE) * 2 + 11;
@@ -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;
@@ -109,19 +107,38 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
*
* 'Retained' PSKs (ie 'generated == false')
* should be preferred to 'generated' PSKs,
+ * PSKs with hash (psk_ver 1) should be
+ * preferred to PSKs without (psk_ver 0),
* and SHA-384 should be preferred to SHA-256.
*/
static struct nvme_tls_psk_priority_list {
bool generated;
+ u8 psk_ver;
enum nvme_tcp_tls_cipher cipher;
} nvme_tls_psk_prio[] = {
{ .generated = false,
+ .psk_ver = 1,
+ .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
+ { .generated = false,
+ .psk_ver = 1,
+ .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
+ { .generated = false,
+ .psk_ver = 0,
.cipher = NVME_TCP_TLS_CIPHER_SHA384, },
{ .generated = false,
+ .psk_ver = 0,
+ .cipher = NVME_TCP_TLS_CIPHER_SHA256, },
+ { .generated = true,
+ .psk_ver = 1,
+ .cipher = NVME_TCP_TLS_CIPHER_SHA384, },
+ { .generated = true,
+ .psk_ver = 1,
.cipher = NVME_TCP_TLS_CIPHER_SHA256, },
{ .generated = true,
+ .psk_ver = 0,
.cipher = NVME_TCP_TLS_CIPHER_SHA384, },
{ .generated = true,
+ .psk_ver = 0,
.cipher = NVME_TCP_TLS_CIPHER_SHA256, },
};
@@ -137,10 +154,11 @@ key_serial_t nvme_tls_psk_default(struct key *keyring,
for (prio = 0; prio < ARRAY_SIZE(nvme_tls_psk_prio); prio++) {
bool generated = nvme_tls_psk_prio[prio].generated;
+ u8 ver = nvme_tls_psk_prio[prio].psk_ver;
enum nvme_tcp_tls_cipher cipher = nvme_tls_psk_prio[prio].cipher;
tls_key = nvme_tls_psk_lookup(keyring, hostnqn, subnqn,
- cipher, generated);
+ cipher, ver, generated);
if (!IS_ERR(tls_key)) {
tls_key_id = tls_key->serial;
key_put(tls_key);
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 02/16] nvme-tcp: sanitize TLS key handling
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
2024-07-17 9:10 ` [PATCH 01/16] nvme-keyring: restrict match length for version '1' identifiers Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 21:53 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 03/16] nvme-tcp: check for invalidated or revoked key Hannes Reinecke
` (14 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, 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 unencrypted (ie with
TLS configured, but not enabled), and then the queue is reset to
run over TLS (ie TLS configured _and_ enabled).
So to differentiate between those two states store the provisioned
key in opts->tls_key (as we're using the same TLS key for all queues)
and only the key serial of the key negotiated by the TLS handshake
in queue->tls_pskid.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
drivers/nvme/host/core.c | 1 -
drivers/nvme/host/nvme.h | 2 +-
drivers/nvme/host/sysfs.c | 4 ++--
drivers/nvme/host/tcp.c | 47 ++++++++++++++++++++++++++++-----------
4 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8d8e7a3549c6..947f1e631ee5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4641,7 +4641,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);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index c63f2b452369..cdb53323f4eb 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -370,7 +370,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;
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 3c55f7edd181..5b1dee8a66ef 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -671,9 +671,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
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index a2a47d3ab99f..92ad5b8cc1b4 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -165,6 +165,7 @@ struct nvme_tcp_queue {
bool hdr_digest;
bool data_digest;
+ bool tls_enabled;
struct ahash_request *rcv_hash;
struct ahash_request *snd_hash;
__le32 exp_ddgst;
@@ -213,7 +214,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_enabled;
+}
+
+static inline bool nvme_tcp_tls_configured(struct nvme_ctrl *ctrl)
{
if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
return 0;
@@ -368,7 +377,7 @@ static inline bool nvme_tcp_queue_has_pending(struct nvme_tcp_queue *queue)
static inline bool nvme_tcp_queue_more(struct nvme_tcp_queue *queue)
{
- return !nvme_tcp_tls(&queue->ctrl->ctrl) &&
+ return !nvme_tcp_tls_enabled(queue) &&
nvme_tcp_queue_has_pending(queue);
}
@@ -1427,7 +1436,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);
}
@@ -1439,7 +1448,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) {
@@ -1587,7 +1596,10 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
qid, pskid);
queue->tls_err = -ENOKEY;
} else {
- ctrl->ctrl.tls_key = tls_key;
+ queue->tls_enabled = true;
+ if (qid == 0)
+ ctrl->ctrl.tls_pskid = key_serial(tls_key);
+ key_put(tls_key);
queue->tls_err = 0;
}
@@ -1768,7 +1780,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;
@@ -1829,6 +1841,8 @@ 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 */
+ queue->tls_enabled = false;
mutex_unlock(&queue->queue_lock);
}
@@ -1925,16 +1939,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;
+ }
}
}
@@ -1957,13 +1972,14 @@ static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
{
int i, ret;
- if (nvme_tcp_tls(ctrl) && !ctrl->tls_key) {
+ if (nvme_tcp_tls_configured(ctrl) && !ctrl->tls_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));
+ ctrl->tls_pskid);
if (ret)
goto out_free_queues;
}
@@ -2144,6 +2160,11 @@ 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_pskid) {
+ dev_dbg(ctrl->device, "Wipe negotiated TLS_PSK %08x\n",
+ ctrl->tls_pskid);
+ ctrl->tls_pskid = 0;
+ }
}
static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 02/16] nvme-tcp: sanitize TLS key handling
2024-07-17 9:10 ` [PATCH 02/16] nvme-tcp: sanitize TLS key handling Hannes Reinecke
@ 2024-07-17 21:53 ` Sagi Grimberg
2024-07-18 7:10 ` Hannes Reinecke
0 siblings, 1 reply; 37+ messages in thread
From: Sagi Grimberg @ 2024-07-17 21:53 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 17/07/2024 12:10, Hannes Reinecke wrote:
> There is a difference between TLS configured (ie the user has
> provisioned/requested a key) and TLS enabled (ie the connection
> is encrypted with TLS). This becomes important for secure concatenation,
> where the initial authentication is run unencrypted (ie with
> TLS configured, but not enabled), and then the queue is reset to
> run over TLS (ie TLS configured _and_ enabled).
> So to differentiate between those two states store the provisioned
> key in opts->tls_key (as we're using the same TLS key for all queues)
> and only the key serial of the key negotiated by the TLS handshake
> in queue->tls_pskid.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---
> drivers/nvme/host/core.c | 1 -
> drivers/nvme/host/nvme.h | 2 +-
> drivers/nvme/host/sysfs.c | 4 ++--
> drivers/nvme/host/tcp.c | 47 ++++++++++++++++++++++++++++-----------
> 4 files changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8d8e7a3549c6..947f1e631ee5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4641,7 +4641,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);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index c63f2b452369..cdb53323f4eb 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -370,7 +370,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;
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 3c55f7edd181..5b1dee8a66ef 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -671,9 +671,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
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index a2a47d3ab99f..92ad5b8cc1b4 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -165,6 +165,7 @@ struct nvme_tcp_queue {
>
> bool hdr_digest;
> bool data_digest;
> + bool tls_enabled;
I swear I'll ask this every single time that I don't understand it. Why
is this per-queue and
not per controller?
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 02/16] nvme-tcp: sanitize TLS key handling
2024-07-17 21:53 ` Sagi Grimberg
@ 2024-07-18 7:10 ` Hannes Reinecke
0 siblings, 0 replies; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-18 7:10 UTC (permalink / raw)
To: Sagi Grimberg, Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 7/17/24 23:53, Sagi Grimberg wrote:
>
>
> On 17/07/2024 12:10, Hannes Reinecke wrote:
>> There is a difference between TLS configured (ie the user has
>> provisioned/requested a key) and TLS enabled (ie the connection
>> is encrypted with TLS). This becomes important for secure concatenation,
>> where the initial authentication is run unencrypted (ie with
>> TLS configured, but not enabled), and then the queue is reset to
>> run over TLS (ie TLS configured _and_ enabled).
>> So to differentiate between those two states store the provisioned
>> key in opts->tls_key (as we're using the same TLS key for all queues)
>> and only the key serial of the key negotiated by the TLS handshake
>> in queue->tls_pskid.
>>
>> Signed-off-by: Hannes Reinecke <hare@kernel.org>
>> ---
>> drivers/nvme/host/core.c | 1 -
>> drivers/nvme/host/nvme.h | 2 +-
>> drivers/nvme/host/sysfs.c | 4 ++--
>> drivers/nvme/host/tcp.c | 47 ++++++++++++++++++++++++++++-----------
>> 4 files changed, 37 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 8d8e7a3549c6..947f1e631ee5 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -4641,7 +4641,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);
>> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
>> index c63f2b452369..cdb53323f4eb 100644
>> --- a/drivers/nvme/host/nvme.h
>> +++ b/drivers/nvme/host/nvme.h
>> @@ -370,7 +370,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;
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 3c55f7edd181..5b1dee8a66ef 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -671,9 +671,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
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index a2a47d3ab99f..92ad5b8cc1b4 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -165,6 +165,7 @@ struct nvme_tcp_queue {
>> bool hdr_digest;
>> bool data_digest;
>> + bool tls_enabled;
>
> I swear I'll ask this every single time that I don't understand it. Why
> is this per-queue and
> not per controller?
TLS is a per-queue setting (each queue has it's own TCP connection, and
that connection might or might not have TLS enabled), _and_, more
importantly, it's a queue _status_ (ie the queue is TLS enabled), and
set ex-post _after_ TLS handshake is run.
This is to differentiate against the TLS _configuration_ (ie the option
'--tls'), which is indeed per controller, but is the _intention_ to
enable TLS.
The distinction becomes important for secure concatenation, as there
one can have an admin queue where TLS should be enabled (the
configuration setting), but currently is not as authentication is
ongoing (the status), then a reset of the admin queue, at the end
of which TLS is enabled (the status).
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 03/16] nvme-tcp: check for invalidated or revoked key
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
2024-07-17 9:10 ` [PATCH 01/16] nvme-keyring: restrict match length for version '1' identifiers Hannes Reinecke
2024-07-17 9:10 ` [PATCH 02/16] nvme-tcp: sanitize TLS key handling Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 21:55 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 04/16] nvme: add a newline to the 'tls_key' sysfs attribute Hannes Reinecke
` (13 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, 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 <hare@kernel.org>
---
drivers/nvme/common/keyring.c | 21 +++++++++++++++++++++
drivers/nvme/host/fabrics.c | 2 +-
drivers/nvme/host/tcp.c | 2 +-
include/linux/nvme-keyring.h | 3 ++-
4 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index c60ebbdc52b8..5b886fd6c3e7 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -20,6 +20,27 @@ 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);
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 44e342a46f39..c62d8890f3a8 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -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
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 92ad5b8cc1b4..5885aa452aa1 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1590,7 +1590,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);
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index e10333d78dbb..e092e3e6ffb2 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
@@ -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,6 @@ 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 */
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 04/16] nvme: add a newline to the 'tls_key' sysfs attribute
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (2 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 03/16] nvme-tcp: check for invalidated or revoked key Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 21:55 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 05/16] nvme-sysfs: add 'tls_configured_key' " Hannes Reinecke
` (12 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Print a newline for easier userspace handling.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/host/sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 5b1dee8a66ef..be36206cb594 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -673,7 +673,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
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 05/16] nvme-sysfs: add 'tls_configured_key' sysfs attribute
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (3 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 04/16] nvme: add a newline to the 'tls_key' sysfs attribute Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 21:58 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 06/16] nvme-sysfs: add 'tls_keyring' attribute Hannes Reinecke
` (11 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, 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 <hare@kernel.org>
---
drivers/nvme/host/sysfs.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index be36206cb594..e5cd738660b1 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -676,6 +676,19 @@ static ssize_t tls_key_show(struct device *dev,
return sysfs_emit(buf, "%08x\n", ctrl->tls_pskid);
}
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;
+
+ if (!key)
+ return 0;
+
+ return sysfs_emit(buf, "%08x\n", key_serial(key));
+}
+static DEVICE_ATTR_RO(tls_configured_key);
#endif
static struct attribute *nvme_dev_attrs[] = {
@@ -707,6 +720,7 @@ static struct attribute *nvme_dev_attrs[] = {
#endif
#ifdef CONFIG_NVME_TCP_TLS
&dev_attr_tls_key.attr,
+ &dev_attr_tls_configured_key.attr,
#endif
&dev_attr_adm_passthru_err_log_enabled.attr,
NULL
@@ -742,6 +756,9 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
if (a == &dev_attr_tls_key.attr &&
(!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
return 0;
+ if (a == &dev_attr_tls_configured_key.attr &&
+ (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
+ return 0;
#endif
return a->mode;
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 05/16] nvme-sysfs: add 'tls_configured_key' sysfs attribute
2024-07-17 9:10 ` [PATCH 05/16] nvme-sysfs: add 'tls_configured_key' " Hannes Reinecke
@ 2024-07-17 21:58 ` Sagi Grimberg
2024-07-18 7:13 ` Hannes Reinecke
0 siblings, 1 reply; 37+ messages in thread
From: Sagi Grimberg @ 2024-07-17 21:58 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 17/07/2024 12:10, Hannes Reinecke wrote:
> 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 <hare@kernel.org>
> ---
> drivers/nvme/host/sysfs.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index be36206cb594..e5cd738660b1 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -676,6 +676,19 @@ static ssize_t tls_key_show(struct device *dev,
> return sysfs_emit(buf, "%08x\n", ctrl->tls_pskid);
> }
> 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;
> +
> + if (!key)
> + return 0;
Shouldn't this check move to the are_visible part so it isn't visible if
tls_key is not provided?
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 05/16] nvme-sysfs: add 'tls_configured_key' sysfs attribute
2024-07-17 21:58 ` Sagi Grimberg
@ 2024-07-18 7:13 ` Hannes Reinecke
0 siblings, 0 replies; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-18 7:13 UTC (permalink / raw)
To: Sagi Grimberg, Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 7/17/24 23:58, Sagi Grimberg wrote:
>
>
> On 17/07/2024 12:10, Hannes Reinecke wrote:
>> 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 <hare@kernel.org>
>> ---
>> drivers/nvme/host/sysfs.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index be36206cb594..e5cd738660b1 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -676,6 +676,19 @@ static ssize_t tls_key_show(struct device *dev,
>> return sysfs_emit(buf, "%08x\n", ctrl->tls_pskid);
>> }
>> 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;
>> +
>> + if (!key)
>> + return 0;
>
> Shouldn't this check move to the are_visible part so it isn't visible if
> tls_key is not provided?
Guess you are right. I thought we could not as the tls_key might change
with secure concatenation, but that doesn't affect the configured key.
Will be updating the patch.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 06/16] nvme-sysfs: add 'tls_keyring' attribute
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (4 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 05/16] nvme-sysfs: add 'tls_configured_key' " Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 21:58 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 07/16] crypto,fs: Separate out hkdf_extract() and hkdf_expand() Hannes Reinecke
` (10 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, 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 <hare@kernel.org>
---
drivers/nvme/host/sysfs.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index e5cd738660b1..462d71e2fbf8 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -689,6 +689,18 @@ static ssize_t tls_configured_key_show(struct device *dev,
return sysfs_emit(buf, "%08x\n", key_serial(key));
}
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;
+
+ if (!keyring)
+ return 0;
+ return sysfs_emit(buf, "%s\n", keyring->description);
+}
+static DEVICE_ATTR_RO(tls_keyring);
#endif
static struct attribute *nvme_dev_attrs[] = {
@@ -721,6 +733,7 @@ static struct attribute *nvme_dev_attrs[] = {
#ifdef CONFIG_NVME_TCP_TLS
&dev_attr_tls_key.attr,
&dev_attr_tls_configured_key.attr,
+ &dev_attr_tls_keyring.attr,
#endif
&dev_attr_adm_passthru_err_log_enabled.attr,
NULL
@@ -759,6 +772,9 @@ static umode_t nvme_dev_attrs_are_visible(struct kobject *kobj,
if (a == &dev_attr_tls_configured_key.attr &&
(!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
return 0;
+ if (a == &dev_attr_tls_keyring.attr &&
+ (!ctrl->opts || strcmp(ctrl->opts->transport, "tcp")))
+ return 0;
#endif
return a->mode;
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 06/16] nvme-sysfs: add 'tls_keyring' attribute
2024-07-17 9:10 ` [PATCH 06/16] nvme-sysfs: add 'tls_keyring' attribute Hannes Reinecke
@ 2024-07-17 21:58 ` Sagi Grimberg
0 siblings, 0 replies; 37+ messages in thread
From: Sagi Grimberg @ 2024-07-17 21:58 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 17/07/2024 12:10, Hannes Reinecke wrote:
> 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 <hare@kernel.org>
> ---
> drivers/nvme/host/sysfs.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index e5cd738660b1..462d71e2fbf8 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -689,6 +689,18 @@ static ssize_t tls_configured_key_show(struct device *dev,
> return sysfs_emit(buf, "%08x\n", key_serial(key));
> }
> 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;
> +
> + if (!keyring)
> + return 0;
Same comment as the last patch.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 07/16] crypto,fs: Separate out hkdf_extract() and hkdf_expand()
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (5 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 06/16] nvme-sysfs: add 'tls_keyring' attribute Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 21:39 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 08/16] nvme: add nvme_auth_generate_psk() Hannes Reinecke
` (9 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Separate out the HKDF functions into a separate file to make them
available to other callers.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
crypto/Makefile | 1 +
crypto/hkdf.c | 112 ++++++++++++++++++++++++++++++++++++++++++
fs/crypto/hkdf.c | 68 ++++---------------------
include/crypto/hkdf.h | 18 +++++++
4 files changed, 140 insertions(+), 59 deletions(-)
create mode 100644 crypto/hkdf.c
create mode 100644 include/crypto/hkdf.h
diff --git a/crypto/Makefile b/crypto/Makefile
index edbbaa3ffef5..b77fc360f0ff 100644
--- a/crypto/Makefile
+++ b/crypto/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_CRYPTO_ECHAINIV) += echainiv.o
crypto_hash-y += ahash.o
crypto_hash-y += shash.o
+crypto_hash-y += hkdf.o
obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o
obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o
diff --git a/crypto/hkdf.c b/crypto/hkdf.c
new file mode 100644
index 000000000000..22e343851c0b
--- /dev/null
+++ b/crypto/hkdf.c
@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Implementation of HKDF ("HMAC-based Extract-and-Expand Key Derivation
+ * Function"), aka RFC 5869. See also the original paper (Krawczyk 2010):
+ * "Cryptographic Extraction and Key Derivation: The HKDF Scheme".
+ *
+ * This is used to derive keys from the fscrypt master keys.
+ *
+ * Copyright 2019 Google LLC
+ */
+
+#include <crypto/hash.h>
+#include <crypto/sha2.h>
+#include <crypto/hkdf.h>
+
+/*
+ * HKDF consists of two steps:
+ *
+ * 1. HKDF-Extract: extract a pseudorandom key of length HKDF_HASHLEN bytes from
+ * the input keying material and optional salt.
+ * 2. HKDF-Expand: expand the pseudorandom key into output keying material of
+ * any length, parameterized by an application-specific info string.
+ *
+ */
+
+/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
+int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
+ unsigned int ikmlen, u8 *prk)
+{
+ unsigned int prklen = crypto_shash_digestsize(hmac_tfm);
+ u8 *default_salt;
+ int err;
+
+ default_salt = kzalloc(prklen, GFP_KERNEL);
+ if (!default_salt)
+ return -ENOMEM;
+ err = crypto_shash_setkey(hmac_tfm, default_salt, prklen);
+ if (!err)
+ err = crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
+
+ kfree(default_salt);
+ return err;
+}
+EXPORT_SYMBOL_GPL(hkdf_extract);
+
+/*
+ * HKDF-Expand (RFC 5869 section 2.3).
+ * This expands the pseudorandom key, which was already keyed into @hmac_tfm,
+ * into @okmlen bytes of output keying material parameterized by the
+ * application-specific @info of length @infolen bytes.
+ * This is thread-safe and may be called by multiple threads in parallel.
+ */
+int hkdf_expand(struct crypto_shash *hmac_tfm,
+ const u8 *info, unsigned int infolen,
+ u8 *okm, unsigned int okmlen)
+{
+ SHASH_DESC_ON_STACK(desc, hmac_tfm);
+ unsigned int i, hashlen = crypto_shash_digestsize(hmac_tfm);
+ int err;
+ const u8 *prev = NULL;
+ u8 counter = 1;
+ u8 *tmp;
+
+ if (WARN_ON(okmlen > 255 * hashlen))
+ return -EINVAL;
+
+ tmp = kzalloc(hashlen, GFP_KERNEL);
+ if (!tmp)
+ return -ENOMEM;
+
+ desc->tfm = hmac_tfm;
+
+ for (i = 0; i < okmlen; i += hashlen) {
+
+ err = crypto_shash_init(desc);
+ if (err)
+ goto out;
+
+ if (prev) {
+ err = crypto_shash_update(desc, prev, hashlen);
+ if (err)
+ goto out;
+ }
+
+ err = crypto_shash_update(desc, info, infolen);
+ if (err)
+ goto out;
+
+ BUILD_BUG_ON(sizeof(counter) != 1);
+ if (okmlen - i < hashlen) {
+ err = crypto_shash_finup(desc, &counter, 1, tmp);
+ if (err)
+ goto out;
+ memcpy(&okm[i], tmp, okmlen - i);
+ memzero_explicit(tmp, sizeof(tmp));
+ } else {
+ err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
+ if (err)
+ goto out;
+ }
+ counter++;
+ prev = &okm[i];
+ }
+ err = 0;
+out:
+ if (unlikely(err))
+ memzero_explicit(okm, okmlen); /* so caller doesn't need to */
+ shash_desc_zero(desc);
+ kfree(tmp);
+ return err;
+}
+EXPORT_SYMBOL_GPL(hkdf_expand);
diff --git a/fs/crypto/hkdf.c b/fs/crypto/hkdf.c
index 5a384dad2c72..9c2f9aca9412 100644
--- a/fs/crypto/hkdf.c
+++ b/fs/crypto/hkdf.c
@@ -11,6 +11,7 @@
#include <crypto/hash.h>
#include <crypto/sha2.h>
+#include <crypto/hkdf.h>
#include "fscrypt_private.h"
@@ -44,20 +45,6 @@
* there's no way to persist a random salt per master key from kernel mode.
*/
-/* HKDF-Extract (RFC 5869 section 2.2), unsalted */
-static int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
- unsigned int ikmlen, u8 prk[HKDF_HASHLEN])
-{
- static const u8 default_salt[HKDF_HASHLEN];
- int err;
-
- err = crypto_shash_setkey(hmac_tfm, default_salt, HKDF_HASHLEN);
- if (err)
- return err;
-
- return crypto_shash_tfm_digest(hmac_tfm, ikm, ikmlen, prk);
-}
-
/*
* Compute HKDF-Extract using the given master key as the input keying material,
* and prepare an HMAC transform object keyed by the resulting pseudorandom key.
@@ -118,61 +105,24 @@ int fscrypt_hkdf_expand(const struct fscrypt_hkdf *hkdf, u8 context,
u8 *okm, unsigned int okmlen)
{
SHASH_DESC_ON_STACK(desc, hkdf->hmac_tfm);
- u8 prefix[9];
- unsigned int i;
+ u8 *prefix;
int err;
- const u8 *prev = NULL;
- u8 counter = 1;
- u8 tmp[HKDF_HASHLEN];
if (WARN_ON_ONCE(okmlen > 255 * HKDF_HASHLEN))
return -EINVAL;
+ prefix = kzalloc(okmlen + 9, GFP_KERNEL);
+ if (!prefix)
+ return -ENOMEM;
desc->tfm = hkdf->hmac_tfm;
memcpy(prefix, "fscrypt\0", 8);
prefix[8] = context;
+ memcpy(prefix + 9, info, infolen);
- for (i = 0; i < okmlen; i += HKDF_HASHLEN) {
-
- err = crypto_shash_init(desc);
- if (err)
- goto out;
-
- if (prev) {
- err = crypto_shash_update(desc, prev, HKDF_HASHLEN);
- if (err)
- goto out;
- }
-
- err = crypto_shash_update(desc, prefix, sizeof(prefix));
- if (err)
- goto out;
-
- err = crypto_shash_update(desc, info, infolen);
- if (err)
- goto out;
-
- BUILD_BUG_ON(sizeof(counter) != 1);
- if (okmlen - i < HKDF_HASHLEN) {
- err = crypto_shash_finup(desc, &counter, 1, tmp);
- if (err)
- goto out;
- memcpy(&okm[i], tmp, okmlen - i);
- memzero_explicit(tmp, sizeof(tmp));
- } else {
- err = crypto_shash_finup(desc, &counter, 1, &okm[i]);
- if (err)
- goto out;
- }
- counter++;
- prev = &okm[i];
- }
- err = 0;
-out:
- if (unlikely(err))
- memzero_explicit(okm, okmlen); /* so caller doesn't need to */
- shash_desc_zero(desc);
+ err = hkdf_expand(hkdf->hmac_tfm, prefix, infolen + 8,
+ okm, okmlen);
+ kfree(prefix);
return err;
}
diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
new file mode 100644
index 000000000000..bf06c080d7ed
--- /dev/null
+++ b/include/crypto/hkdf.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * HKDF: HMAC-based Key Derivation Function (HKDF), RFC 5869
+ *
+ * Extracted from fs/crypto/hkdf.c, which has
+ * Copyright 2019 Google LLC
+ */
+
+#ifndef _CRYPTO_HKDF_H
+#define _CRYPTO_HKDF_H
+
+int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
+ unsigned int ikmlen, u8 *prk);
+int hkdf_expand(struct crypto_shash *hmac_tfm,
+ const u8 *info, unsigned int infolen,
+ u8 *okm, unsigned int okmlen);
+
+#endif
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 08/16] nvme: add nvme_auth_generate_psk()
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (6 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 07/16] crypto,fs: Separate out hkdf_extract() and hkdf_expand() Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 9:10 ` [PATCH 09/16] nvme: add nvme_auth_generate_digest() Hannes Reinecke
` (8 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Add a function to generate a NVMe PSK from the shared credentials
negotiated by DH-HMAC-CHAP.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/common/auth.c | 87 ++++++++++++++++++++++++++++++++++++++
include/linux/nvme-auth.h | 3 ++
2 files changed, 90 insertions(+)
diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index a3455f1d67fa..f6d21960b140 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -11,6 +11,7 @@
#include <asm/unaligned.h>
#include <crypto/hash.h>
#include <crypto/dh.h>
+#include <crypto/hkdf.h>
#include <linux/nvme.h>
#include <linux/nvme-auth.h>
@@ -471,5 +472,91 @@ int nvme_auth_generate_key(u8 *secret, struct nvme_dhchap_key **ret_key)
}
EXPORT_SYMBOL_GPL(nvme_auth_generate_key);
+/*
+ * nvme_auth_generate_psk - Generate a PSK for TLS
+ *
+ * Generate a PSK for TLS as specified in NVMe base specification, section 8.13.5.9:
+ * Generated PSK for TLS
+ *
+ * The generated PSK for TLS shall be computed applying the HMAC function using the
+ * hash function H( ) selected by the HashID parameter in the DH-HMAC-CHAP_Challenge
+ * message with the session key KS as key to the concatenation of the two challenges
+ * C1 and C2 (i.e., generated PSK = HMAC(KS, C1 || C2)).
+ */
+int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+ u8 *c1, u8 *c2, size_t hash_len, u8 **ret_psk,size_t *ret_len)
+{
+ struct crypto_shash *tfm;
+ struct shash_desc *shash;
+ u8 *psk;
+ const char *hmac_name;
+ int ret, psk_len;
+
+ if (!c1 || !c2) {
+ pr_warn("%s: invalid parameter\n", __func__);
+ return -EINVAL;
+ }
+
+ hmac_name = nvme_auth_hmac_name(hmac_id);
+ if (!hmac_name) {
+ pr_warn("%s: invalid hash algoritm %d\n",
+ __func__, hmac_id);
+ return -EINVAL;
+ }
+
+ tfm = crypto_alloc_shash(hmac_name, 0, 0);
+ if (IS_ERR(tfm))
+ return PTR_ERR(tfm);
+
+ psk_len = crypto_shash_digestsize(tfm);
+ psk = kzalloc(psk_len, GFP_KERNEL);
+ if (!psk) {
+ ret = -ENOMEM;
+ goto out_free_tfm;
+ }
+
+ shash = kmalloc(sizeof(struct shash_desc) +
+ crypto_shash_descsize(tfm),
+ GFP_KERNEL);
+ if (!shash) {
+ ret = -ENOMEM;
+ goto out_free_psk;
+ }
+
+ shash->tfm = tfm;
+ ret = crypto_shash_setkey(tfm, skey, skey_len);
+ if (ret)
+ goto out_free_shash;
+
+ ret = crypto_shash_init(shash);
+ if (ret)
+ goto out_free_shash;
+
+ ret = crypto_shash_update(shash, c1, hash_len);
+ if (ret)
+ goto out_free_shash;
+
+ ret = crypto_shash_update(shash, c2, hash_len);
+ if (ret)
+ goto out_free_shash;
+
+ ret = crypto_shash_final(shash, psk);
+ if (!ret) {
+ *ret_psk = psk;
+ *ret_len = psk_len;
+ }
+
+out_free_shash:
+ kfree_sensitive(shash);
+out_free_psk:
+ if (ret)
+ kfree_sensitive(psk);
+out_free_tfm:
+ crypto_free_shash(tfm);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_generate_psk);
+
MODULE_DESCRIPTION("NVMe Authentication framework");
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index c1d0bc5d9624..b13884b04dfd 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -40,5 +40,8 @@ int nvme_auth_gen_pubkey(struct crypto_kpp *dh_tfm,
int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
u8 *ctrl_key, size_t ctrl_key_len,
u8 *sess_key, size_t sess_key_len);
+int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
+ u8 *c1, u8 *c2, size_t hash_len,
+ u8 **ret_psk, size_t *ret_len);
#endif /* _NVME_AUTH_H */
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 09/16] nvme: add nvme_auth_generate_digest()
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (7 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 08/16] nvme: add nvme_auth_generate_psk() Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 9:10 ` [PATCH 10/16] nvme: add nvme_auth_derive_tls_psk() Hannes Reinecke
` (7 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Add a function to calculate the PSK digest as specified in TP8018.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/common/auth.c | 126 +++++++++++++++++++++++++++++++++++++
include/linux/nvme-auth.h | 2 +
2 files changed, 128 insertions(+)
diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index f6d21960b140..891079c3dbd5 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -558,5 +558,131 @@ int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
}
EXPORT_SYMBOL_GPL(nvme_auth_generate_psk);
+/*
+ * Generate a TLS PSK digest as specified in TP8018 Section 3.6.1.3:
+ * TLS PSK and PSK identity Derivation
+ *
+ * The PSK digest shall be computed by encoding in Base64 (refer to RFC 4648)
+ * the result of the application of the HMAC function using the hash function
+ * specified in item 4 above (ie the hash function of the cipher suite associated
+ * with the PSK identity) with the PSK as HMAC key to the concatenation of:
+ * - the NQN of the host (i.e., NQNh) not including the null terminator;
+ * - a space character;
+ * - the NQN of the NVM subsystem (i.e., NQNc) not including the null terminator;
+ * - a space character; and
+ * - the seventeen ASCII characters "NVMe-over-Fabrics"
+ * (i.e., <PSK digest> = Base64(HMAC(PSK, NQNh || " " || NQNc || " " || "NVMe-over-Fabrics"))).
+ * The length of the PSK digest depends on the hash function used to compute
+ * it as follows:
+ * - If the SHA-256 hash function is used, the resulting PSK digest is 44 characters long; or
+ * - If the SHA-384 hash function is used, the resulting PSK digest is 64 characters long.
+ */
+int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+ char *subsysnqn, char *hostnqn, u8 **ret_digest)
+{
+ struct crypto_shash *tfm;
+ struct shash_desc *shash;
+ u8 *digest, *hmac;
+ const char *hmac_name;
+ size_t digest_len, hmac_len;
+ int ret;
+
+ if (WARN_ON(!subsysnqn || !hostnqn))
+ return -EINVAL;
+
+ hmac_name = nvme_auth_hmac_name(hmac_id);
+ if (!hmac_name) {
+ pr_warn("%s: invalid hash algoritm %d\n",
+ __func__, hmac_id);
+ return -EINVAL;
+ }
+
+ switch (nvme_auth_hmac_hash_len(hmac_id)) {
+ case 32:
+ hmac_len = 44;
+ break;
+ case 48:
+ hmac_len = 64;
+ break;
+ default:
+ pr_warn("%s: invalid hash algorithm '%s'\n",
+ __func__, hmac_name);
+ return -EINVAL;
+ }
+
+ hmac = kzalloc(hmac_len + 1, GFP_KERNEL);
+ if (!hmac)
+ return -ENOMEM;
+
+ tfm = crypto_alloc_shash(hmac_name, 0, 0);
+ if (IS_ERR(tfm)) {
+ ret = PTR_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_digest = hmac;
+ 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);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_generate_digest);
+
MODULE_DESCRIPTION("NVMe Authentication framework");
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index b13884b04dfd..998f06bf10fd 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -43,5 +43,7 @@ int nvme_auth_gen_shared_secret(struct crypto_kpp *dh_tfm,
int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
u8 *c1, u8 *c2, size_t hash_len,
u8 **ret_psk, size_t *ret_len);
+int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
+ char *subsysnqn, char *hostnqn, u8 **ret_digest);
#endif /* _NVME_AUTH_H */
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 10/16] nvme: add nvme_auth_derive_tls_psk()
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (8 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 09/16] nvme: add nvme_auth_generate_digest() Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 22:01 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 11/16] nvme-keyring: add nvme_tls_psk_refresh() Hannes Reinecke
` (6 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Add a function to derive the TLS PSK as specified TP8018.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
drivers/nvme/common/auth.c | 90 ++++++++++++++++++++++++++++++++++++++
include/linux/nvme-auth.h | 2 +
2 files changed, 92 insertions(+)
diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 891079c3dbd5..a4ccc4e76581 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -684,5 +684,95 @@ int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
}
EXPORT_SYMBOL_GPL(nvme_auth_generate_digest);
+/*
+ * Derive a TLS PSK as specified in TP8018 Section 3.6.1.3:
+ * TLS PSK and PSK identity Derivation
+ *
+ * The TLS PSK shall be derived as follows from an input PSK
+ * (i.e., either a retained PSK or a generated PSK) and a PSK
+ * identity using the HKDF-Extract and HKDF-Expand-Label operations
+ * (refer to RFC 5869 and RFC 8446) where the hash function is the
+ * one specified by the hash specifier of the PSK identity:
+ * 1. PRK = HKDF-Extract(0, Input PSK); and
+ * 2. TLS PSK = HKDF-Expand-Label(PRK, "nvme-tls-psk", PskIdentityContext, L),
+ * where PskIdentityContext is the hash identifier indicated in
+ * the PSK identity concatenated to a space character and to the
+ * Base64 PSK digest (i.e., "<hash> <PSK digest>") and L is the
+ * output size in bytes of the hash function (i.e., 32 for SHA-256
+ * and 48 for SHA-384).
+ */
+int nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len,
+ u8 *psk_digest, u8 **ret_psk)
+{
+ 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 -EINVAL;
+ }
+ if (hmac_id == NVME_AUTH_HASH_SHA512) {
+ pr_warn("%s: unsupported hash algorithm %s\n",
+ __func__, hmac_name);
+ return -EINVAL;
+ }
+
+ hmac_tfm = crypto_alloc_shash(hmac_name, 0, 0);
+ if (IS_ERR(hmac_tfm))
+ return PTR_ERR(hmac_tfm);
+
+ prk_len = crypto_shash_digestsize(hmac_tfm);
+ prk = kzalloc(prk_len, GFP_KERNEL);
+ if (!prk) {
+ ret = -ENOMEM;
+ goto out_free_shash;
+ }
+
+ ret = hkdf_extract(hmac_tfm, psk, psk_len, prk);
+ if (ret)
+ goto out_free_prk;
+
+ ret = crypto_shash_setkey(hmac_tfm, prk, prk_len);
+ if (ret)
+ goto out_free_prk;
+
+ info_len = strlen(psk_digest) + strlen(psk_prefix) + 1;
+ info = kzalloc(info_len, GFP_KERNEL);
+ if (!info)
+ goto out_free_prk;
+
+ memcpy(info, psk_prefix, strlen(psk_prefix));
+ memcpy(info + strlen(psk_prefix), psk_digest, strlen(psk_digest));
+
+ tls_key = kzalloc(psk_len, GFP_KERNEL);
+ if (!tls_key) {
+ ret = -ENOMEM;
+ goto out_free_info;
+ }
+ ret = hkdf_expand(hmac_tfm, info, strlen(info), tls_key, psk_len);
+ if (ret) {
+ kfree(tls_key);
+ goto out_free_info;
+ }
+ *ret_psk = tls_key;
+
+out_free_info:
+ kfree(info);
+out_free_prk:
+ kfree(prk);
+out_free_shash:
+ crypto_free_shash(hmac_tfm);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(nvme_auth_derive_tls_psk);
+
MODULE_DESCRIPTION("NVMe Authentication framework");
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/nvme-auth.h b/include/linux/nvme-auth.h
index 998f06bf10fd..60e069a6757f 100644
--- a/include/linux/nvme-auth.h
+++ b/include/linux/nvme-auth.h
@@ -45,5 +45,7 @@ int nvme_auth_generate_psk(u8 hmac_id, u8 *skey, size_t skey_len,
u8 **ret_psk, size_t *ret_len);
int nvme_auth_generate_digest(u8 hmac_id, u8 *psk, size_t psk_len,
char *subsysnqn, char *hostnqn, u8 **ret_digest);
+int nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len,
+ u8 *psk_digest, u8 **ret_psk);
#endif /* _NVME_AUTH_H */
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 11/16] nvme-keyring: add nvme_tls_psk_refresh()
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (9 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 10/16] nvme: add nvme_auth_derive_tls_psk() Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 22:04 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 12/16] nvme-tcp: request secure channel concatenation Hannes Reinecke
` (5 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Add a function to refresh a generated PSK in the specified keyring.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
drivers/nvme/common/keyring.c | 50 +++++++++++++++++++++++++++++++++++
include/linux/nvme-keyring.h | 7 +++++
2 files changed, 57 insertions(+)
diff --git a/drivers/nvme/common/keyring.c b/drivers/nvme/common/keyring.c
index 5b886fd6c3e7..9e847c3ee820 100644
--- a/drivers/nvme/common/keyring.c
+++ b/drivers/nvme/common/keyring.c
@@ -123,6 +123,56 @@ static struct key *nvme_tls_psk_lookup(struct key *keyring,
return key_ref_to_ptr(keyref);
}
+struct key *nvme_tls_psk_refresh(struct key *keyring, char *hostnqn, char *subnqn,
+ u8 hmac_id, bool generated, u8 *data, size_t data_len, char *digest)
+{
+ key_perm_t keyperm =
+ KEY_POS_SEARCH | KEY_POS_VIEW | KEY_POS_READ |
+ KEY_POS_WRITE | KEY_POS_LINK | KEY_POS_SETATTR |
+ KEY_USR_SEARCH | KEY_USR_VIEW | KEY_USR_READ;
+ char *identity;
+ size_t identity_len = (NVMF_NQN_SIZE) * 2 + 77;
+ key_ref_t keyref;
+ key_serial_t keyring_id;
+ struct key *key;
+
+ if (!hostnqn || !subnqn || !data || !data_len)
+ return ERR_PTR(-EINVAL);
+
+ identity = kzalloc(identity_len, GFP_KERNEL);
+ if (!identity)
+ return ERR_PTR(-ENOMEM);
+
+ snprintf(identity, identity_len, "NVMe1%c%02d %s %s %s",
+ generated ? 'G' : 'R', hmac_id, hostnqn, subnqn, digest);
+
+ if (!keyring)
+ keyring = nvme_keyring;
+ keyring_id = key_serial(keyring);
+ pr_debug("keyring %x refresh tls psk '%s'\n",
+ keyring_id, identity);
+ keyref = key_create_or_update(make_key_ref(keyring, true),
+ "psk", identity, data, data_len,
+ keyperm, KEY_ALLOC_NOT_IN_QUOTA |
+ KEY_ALLOC_BUILT_IN |
+ KEY_ALLOC_BYPASS_RESTRICTION);
+ if (IS_ERR(keyref)) {
+ pr_debug("refresh tls psk '%s' failed, error %ld\n",
+ identity, PTR_ERR(keyref));
+ kfree(identity);
+ return ERR_PTR(-ENOKEY);
+ }
+ kfree(identity);
+ /*
+ * Set the default timeout to 1 hour
+ * as suggested in TP8018.
+ */
+ key = key_ref_to_ptr(keyref);
+ key_set_timeout(key, 3600);
+ return key;
+}
+EXPORT_SYMBOL_GPL(nvme_tls_psk_refresh);
+
/*
* NVMe PSK priority list
*
diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
index e092e3e6ffb2..ae9bac51291e 100644
--- a/include/linux/nvme-keyring.h
+++ b/include/linux/nvme-keyring.h
@@ -8,6 +8,8 @@
#if IS_ENABLED(CONFIG_NVME_KEYRING)
+struct key *nvme_tls_psk_refresh(struct key *keyring, char *hostnqn, char *subnqn,
+ u8 hmac_id, bool generated, u8 *data, size_t data_len, char *digest);
key_serial_t nvme_tls_psk_default(struct key *keyring,
const char *hostnqn, const char *subnqn);
@@ -15,6 +17,11 @@ key_serial_t nvme_keyring_id(void);
struct key *nvme_tls_key_lookup(key_serial_t key_id);
#else
+static struct key *nvme_tls_psk_refresh(struct key *keyring, char *hostnqn, char *subnqn,
+ u8 hmac_id, bool generated, u8 *data, size_t data_len, char *digest)
+{
+ return -ENOTSUPP;
+}
static inline key_serial_t nvme_tls_psk_default(struct key *keyring,
const char *hostnqn, const char *subnqn)
{
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 12/16] nvme-tcp: request secure channel concatenation
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (10 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 11/16] nvme-keyring: add nvme_tls_psk_refresh() Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 22:31 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 13/16] nvme-fabrics: reset admin connection for secure concatenation Hannes Reinecke
` (4 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Add a fabrics option 'concat' to request secure channel concatenation.
When secure channel concatenation is enabled a 'generated PSK' is inserted
into the keyring such that it's available after reset.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
drivers/nvme/host/auth.c | 105 ++++++++++++++++++++++++++++++++++--
drivers/nvme/host/fabrics.c | 34 ++++++++++--
drivers/nvme/host/fabrics.h | 3 ++
drivers/nvme/host/sysfs.c | 2 +-
drivers/nvme/host/tcp.c | 54 ++++++++++++++++---
include/linux/nvme.h | 7 +++
6 files changed, 191 insertions(+), 14 deletions(-)
diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 371e14f0a203..23ff3df27e80 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -12,6 +12,7 @@
#include "nvme.h"
#include "fabrics.h"
#include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
#define CHAP_BUF_SIZE 4096
static struct kmem_cache *nvme_chap_buf_cache;
@@ -131,7 +132,12 @@ static int nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl,
data->auth_type = NVME_AUTH_COMMON_MESSAGES;
data->auth_id = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE;
data->t_id = cpu_to_le16(chap->transaction);
- data->sc_c = 0; /* No secure channel concatenation */
+ if (!ctrl->opts->concat || chap->qid != 0)
+ data->sc_c = NVME_AUTH_SECP_NOSC;
+ else if (ctrl->opts->tls_key)
+ data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
+ else
+ data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
data->napd = 1;
data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
data->auth_protocol[0].dhchap.halen = 3;
@@ -311,8 +317,9 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
data->hl = chap->hash_len;
data->dhvlen = cpu_to_le16(chap->host_key_len);
memcpy(data->rval, chap->response, chap->hash_len);
- if (ctrl->ctrl_key) {
+ if (ctrl->ctrl_key)
chap->bi_directional = true;
+ if (ctrl->ctrl_key || ctrl->opts->concat) {
get_random_bytes(chap->c2, chap->hash_len);
data->cvalid = 1;
memcpy(data->rval + chap->hash_len, chap->c2,
@@ -322,7 +329,10 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
} else {
memset(chap->c2, 0, chap->hash_len);
}
- chap->s2 = nvme_auth_get_seqnum();
+ if (ctrl->opts->concat)
+ chap->s2 = 0;
+ else
+ chap->s2 = nvme_auth_get_seqnum();
data->seqnum = cpu_to_le32(chap->s2);
if (chap->host_key_len) {
dev_dbg(ctrl->device, "%s: qid %d host public key %*ph\n",
@@ -677,6 +687,76 @@ 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;
+ }
+
+ ret = nvme_auth_generate_psk(chap->hash_id, chap->sess_key,
+ chap->sess_key_len,
+ chap->c1, chap->c2,
+ chap->hash_len, &psk, &psk_len);
+ if (ret) {
+ 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);
+
+ ret = nvme_auth_generate_digest(chap->hash_id, psk, psk_len,
+ ctrl->opts->subsysnqn,
+ ctrl->opts->host->nqn, &digest);
+ if (ret) {
+ 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);
+ ret = nvme_auth_derive_tls_psk(chap->hash_id, psk, psk_len, digest, &tls_psk);
+ if (ret) {
+ dev_warn(ctrl->device,
+ "%s: qid %d failed to derive TLS psk, error %d\n",
+ __func__, chap->qid, ret);
+ goto out_free_digest;
+ };
+
+ tls_key = nvme_tls_psk_refresh(ctrl->opts->keyring, ctrl->opts->host->nqn,
+ ctrl->opts->subsysnqn, chap->hash_id,
+ true, tls_psk, psk_len, digest);
+ if (IS_ERR(tls_key)) {
+ ret = PTR_ERR(tls_key);
+ dev_warn(ctrl->device,
+ "%s: qid %d failed to insert generated key, error %d\n",
+ __func__, chap->qid, ret);
+ tls_key = NULL;
+ kfree_sensitive(tls_psk);
+ }
+ if (ctrl->opts->tls_key) {
+ key_revoke(ctrl->opts->tls_key);
+ key_put(ctrl->opts->tls_key);
+ }
+ ctrl->opts->tls_key = tls_key;
+out_free_digest:
+ kfree_sensitive(digest);
+out_free_psk:
+ kfree_sensitive(psk);
+ return ret;
+}
+
static void nvme_queue_auth_work(struct work_struct *work)
{
struct nvme_dhchap_queue_context *chap =
@@ -831,10 +911,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 +1003,12 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
"qid 0: authentication failed\n");
return;
}
+ /*
+ * Only run authentication on the admin queue for
+ * secure concatenation
+ */
+ if (ctrl->opts->concat)
+ return;
for (q = 1; q < ctrl->queue_count; q++) {
ret = nvme_auth_negotiate(ctrl, q);
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index c62d8890f3a8..f83ac1292a3b 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -472,8 +472,9 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
result = le32_to_cpu(res.u32);
ctrl->cntlid = result & 0xFFFF;
if (result & (NVME_CONNECT_AUTHREQ_ATR | NVME_CONNECT_AUTHREQ_ASCR)) {
- /* Secure concatenation is not implemented */
- if (result & NVME_CONNECT_AUTHREQ_ASCR) {
+ /* Check for secure concatenation */
+ if ((result & NVME_CONNECT_AUTHREQ_ASCR) &&
+ !ctrl->opts->concat) {
dev_warn(ctrl->device,
"qid 0: secure concatenation is not supported\n");
ret = -EOPNOTSUPP;
@@ -550,7 +551,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
/* Secure concatenation is not implemented */
if (result & NVME_CONNECT_AUTHREQ_ASCR) {
dev_warn(ctrl->device,
- "qid 0: secure concatenation is not supported\n");
+ "qid %d: secure concatenation is not supported\n", qid);
ret = -EOPNOTSUPP;
goto out_free_data;
}
@@ -706,6 +707,7 @@ static const match_table_t opt_tokens = {
#endif
#ifdef CONFIG_NVME_TCP_TLS
{ NVMF_OPT_TLS, "tls" },
+ { NVMF_OPT_CONCAT, "concat" },
#endif
{ NVMF_OPT_ERR, NULL }
};
@@ -735,6 +737,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
opts->tls = false;
opts->tls_key = NULL;
opts->keyring = NULL;
+ opts->concat = false;
options = o = kstrdup(buf, GFP_KERNEL);
if (!options)
@@ -1053,6 +1056,14 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
}
opts->tls = true;
break;
+ case NVMF_OPT_CONCAT:
+ if (!IS_ENABLED(CONFIG_NVME_TCP_TLS)) {
+ pr_err("TLS is not supported\n");
+ ret = -EINVAL;
+ goto out;
+ }
+ opts->concat = true;
+ break;
default:
pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
p);
@@ -1079,6 +1090,23 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
pr_warn("failfast tmo (%d) larger than controller loss tmo (%d)\n",
opts->fast_io_fail_tmo, ctrl_loss_tmo);
}
+ if (opts->concat) {
+ if (opts->tls) {
+ pr_err("Secure concatenation over TLS is not supported\n");
+ ret = -EINVAL;
+ goto out;
+ }
+ if (opts->tls_key) {
+ pr_err("Cannot specify a TLS key for secure concatenation\n");
+ ret = -EINVAL;
+ goto out;
+ }
+ if (!opts->dhchap_secret) {
+ pr_err("Need to enable DH-CHAP for secure concatenation\n");
+ ret = -EINVAL;
+ goto out;
+ }
+ }
opts->host = nvmf_host_add(hostnqn, &hostid);
if (IS_ERR(opts->host)) {
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 21d75dc4a3a0..9cf5b020adba 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -66,6 +66,7 @@ enum {
NVMF_OPT_TLS = 1 << 25,
NVMF_OPT_KEYRING = 1 << 26,
NVMF_OPT_TLS_KEY = 1 << 27,
+ NVMF_OPT_CONCAT = 1 << 28,
};
/**
@@ -101,6 +102,7 @@ enum {
* @keyring: Keyring to use for key lookups
* @tls_key: TLS key for encrypted connections (TCP)
* @tls: Start TLS encrypted connections (TCP)
+ * @concat: Enabled Secure channel concatenation (TCP)
* @disable_sqflow: disable controller sq flow control
* @hdr_digest: generate/verify header digest (TCP)
* @data_digest: generate/verify data digest (TCP)
@@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
struct key *keyring;
struct key *tls_key;
bool tls;
+ bool concat;
bool disable_sqflow;
bool hdr_digest;
bool data_digest;
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 462d71e2fbf8..5350eb87ec52 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -683,7 +683,7 @@ static ssize_t tls_configured_key_show(struct device *dev,
struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
struct key *key = ctrl->opts->tls_key;
- if (!key)
+ if (!key || ctrl->opts->concat)
return 0;
return sysfs_emit(buf, "%08x\n", key_serial(key));
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 5885aa452aa1..d6c085ba0114 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -227,7 +227,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)
@@ -1942,7 +1942,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);
@@ -1972,9 +1972,25 @@ static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
{
int i, ret;
- if (nvme_tcp_tls_configured(ctrl) && !ctrl->tls_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 (ctrl->tls_pskid &&
+ ctrl->tls_pskid != key_serial(ctrl->opts->tls_key)) {
+ dev_err(ctrl->device, "Stale PSK id %08x\n", ctrl->tls_pskid);
+ ctrl->tls_pskid = 0;
+ }
+ } else if (!ctrl->tls_pskid) {
+ dev_err(ctrl->device, "no PSK negotiated\n");
+ return -ENOKEY;
+ }
}
for (i = 1; i < ctrl->queue_count; i++) {
@@ -2205,6 +2221,27 @@ 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;
+ /* TLS is not enabled, do not wipe the key */
+ if (!ctrl->tls_pskid)
+ return;
+ /*
+ * Key generated, and TLS enabled:
+ * Revoke the generated 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;
@@ -2308,6 +2345,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);
@@ -2348,6 +2386,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
container_of(work, struct nvme_ctrl, reset_work);
int ret;
+ nvme_tcp_revoke_generated_tls_key(ctrl);
nvme_stop_ctrl(ctrl);
nvme_tcp_teardown_ctrl(ctrl, false);
@@ -2638,6 +2677,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))
@@ -2842,7 +2884,7 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
- NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
+ NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
.create_ctrl = nvme_tcp_create_ctrl,
};
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c12a329dd463..ef85cf69cf99 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1669,6 +1669,13 @@ enum {
NVME_AUTH_DHGROUP_INVALID = 0xff,
};
+enum {
+ NVME_AUTH_SECP_NOSC = 0x00,
+ NVME_AUTH_SECP_SC = 0x01,
+ NVME_AUTH_SECP_NEWTLSPSK = 0x02,
+ NVME_AUTH_SECP_REPLACETLSPSK = 0x03,
+};
+
union nvmf_auth_protocol {
struct nvmf_auth_dhchap_protocol_descriptor dhchap;
};
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 12/16] nvme-tcp: request secure channel concatenation
2024-07-17 9:10 ` [PATCH 12/16] nvme-tcp: request secure channel concatenation Hannes Reinecke
@ 2024-07-17 22:31 ` Sagi Grimberg
2024-07-18 7:30 ` Hannes Reinecke
0 siblings, 1 reply; 37+ messages in thread
From: Sagi Grimberg @ 2024-07-17 22:31 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 17/07/2024 12:10, Hannes Reinecke wrote:
> Add a fabrics option 'concat' to request secure channel concatenation.
> When secure channel concatenation is enabled a 'generated PSK' is inserted
> into the keyring such that it's available after reset.
>
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> ---
> drivers/nvme/host/auth.c | 105 ++++++++++++++++++++++++++++++++++--
> drivers/nvme/host/fabrics.c | 34 ++++++++++--
> drivers/nvme/host/fabrics.h | 3 ++
> drivers/nvme/host/sysfs.c | 2 +-
> drivers/nvme/host/tcp.c | 54 ++++++++++++++++---
> include/linux/nvme.h | 7 +++
> 6 files changed, 191 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 371e14f0a203..23ff3df27e80 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -12,6 +12,7 @@
> #include "nvme.h"
> #include "fabrics.h"
> #include <linux/nvme-auth.h>
> +#include <linux/nvme-keyring.h>
>
> #define CHAP_BUF_SIZE 4096
> static struct kmem_cache *nvme_chap_buf_cache;
> @@ -131,7 +132,12 @@ static int nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl,
> data->auth_type = NVME_AUTH_COMMON_MESSAGES;
> data->auth_id = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE;
> data->t_id = cpu_to_le16(chap->transaction);
> - data->sc_c = 0; /* No secure channel concatenation */
> + if (!ctrl->opts->concat || chap->qid != 0)
> + data->sc_c = NVME_AUTH_SECP_NOSC;
> + else if (ctrl->opts->tls_key)
> + data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
> + else
> + data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
> data->napd = 1;
> data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
> data->auth_protocol[0].dhchap.halen = 3;
> @@ -311,8 +317,9 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
> data->hl = chap->hash_len;
> data->dhvlen = cpu_to_le16(chap->host_key_len);
> memcpy(data->rval, chap->response, chap->hash_len);
> - if (ctrl->ctrl_key) {
> + if (ctrl->ctrl_key)
> chap->bi_directional = true;
> + if (ctrl->ctrl_key || ctrl->opts->concat) {
> get_random_bytes(chap->c2, chap->hash_len);
> data->cvalid = 1;
> memcpy(data->rval + chap->hash_len, chap->c2,
> @@ -322,7 +329,10 @@ static int nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
> } else {
> memset(chap->c2, 0, chap->hash_len);
> }
> - chap->s2 = nvme_auth_get_seqnum();
> + if (ctrl->opts->concat)
> + chap->s2 = 0;
> + else
> + chap->s2 = nvme_auth_get_seqnum();
> data->seqnum = cpu_to_le32(chap->s2);
> if (chap->host_key_len) {
> dev_dbg(ctrl->device, "%s: qid %d host public key %*ph\n",
> @@ -677,6 +687,76 @@ 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;
> + }
> +
> + ret = nvme_auth_generate_psk(chap->hash_id, chap->sess_key,
> + chap->sess_key_len,
> + chap->c1, chap->c2,
> + chap->hash_len, &psk, &psk_len);
> + if (ret) {
> + 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);
> +
> + ret = nvme_auth_generate_digest(chap->hash_id, psk, psk_len,
> + ctrl->opts->subsysnqn,
> + ctrl->opts->host->nqn, &digest);
> + if (ret) {
> + 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);
> + ret = nvme_auth_derive_tls_psk(chap->hash_id, psk, psk_len, digest, &tls_psk);
> + if (ret) {
> + dev_warn(ctrl->device,
> + "%s: qid %d failed to derive TLS psk, error %d\n",
> + __func__, chap->qid, ret);
> + goto out_free_digest;
> + };
> +
> + tls_key = nvme_tls_psk_refresh(ctrl->opts->keyring, ctrl->opts->host->nqn,
> + ctrl->opts->subsysnqn, chap->hash_id,
> + true, tls_psk, psk_len, digest);
> + if (IS_ERR(tls_key)) {
> + ret = PTR_ERR(tls_key);
> + dev_warn(ctrl->device,
> + "%s: qid %d failed to insert generated key, error %d\n",
> + __func__, chap->qid, ret);
> + tls_key = NULL;
> + kfree_sensitive(tls_psk);
> + }
> + if (ctrl->opts->tls_key) {
> + key_revoke(ctrl->opts->tls_key);
> + key_put(ctrl->opts->tls_key);
> + }
> + ctrl->opts->tls_key = tls_key;
> +out_free_digest:
> + kfree_sensitive(digest);
> +out_free_psk:
> + kfree_sensitive(psk);
> + return ret;
> +}
> +
> static void nvme_queue_auth_work(struct work_struct *work)
> {
> struct nvme_dhchap_queue_context *chap =
> @@ -831,10 +911,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) {
Please add a comment here on why you are doing this.
> 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 +1003,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
> + */
Seems like you got some whitespaces misalignment, I'd pass the patchset
through checkpatch.pl
> + if (ctrl->opts->concat)
> + return;
>
> for (q = 1; q < ctrl->queue_count; q++) {
> ret = nvme_auth_negotiate(ctrl, q);
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index c62d8890f3a8..f83ac1292a3b 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -472,8 +472,9 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
> result = le32_to_cpu(res.u32);
> ctrl->cntlid = result & 0xFFFF;
> if (result & (NVME_CONNECT_AUTHREQ_ATR | NVME_CONNECT_AUTHREQ_ASCR)) {
> - /* Secure concatenation is not implemented */
> - if (result & NVME_CONNECT_AUTHREQ_ASCR) {
> + /* Check for secure concatenation */
> + if ((result & NVME_CONNECT_AUTHREQ_ASCR) &&
> + !ctrl->opts->concat) {
Shouldn't you check for the dhchap_secret here instead of concat?
> dev_warn(ctrl->device,
> "qid 0: secure concatenation is not supported\n");
> ret = -EOPNOTSUPP;
> @@ -550,7 +551,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid)
> /* Secure concatenation is not implemented */
> if (result & NVME_CONNECT_AUTHREQ_ASCR) {
> dev_warn(ctrl->device,
> - "qid 0: secure concatenation is not supported\n");
> + "qid %d: secure concatenation is not supported\n", qid);
> ret = -EOPNOTSUPP;
> goto out_free_data;
> }
> @@ -706,6 +707,7 @@ static const match_table_t opt_tokens = {
> #endif
> #ifdef CONFIG_NVME_TCP_TLS
> { NVMF_OPT_TLS, "tls" },
> + { NVMF_OPT_CONCAT, "concat" },
> #endif
> { NVMF_OPT_ERR, NULL }
> };
> @@ -735,6 +737,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
> opts->tls = false;
> opts->tls_key = NULL;
> opts->keyring = NULL;
> + opts->concat = false;
>
> options = o = kstrdup(buf, GFP_KERNEL);
> if (!options)
> @@ -1053,6 +1056,14 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
> }
> opts->tls = true;
> break;
> + case NVMF_OPT_CONCAT:
> + if (!IS_ENABLED(CONFIG_NVME_TCP_TLS)) {
> + pr_err("TLS is not supported\n");
> + ret = -EINVAL;
> + goto out;
> + }
> + opts->concat = true;
> + break;
> default:
> pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
> p);
> @@ -1079,6 +1090,23 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
> pr_warn("failfast tmo (%d) larger than controller loss tmo (%d)\n",
> opts->fast_io_fail_tmo, ctrl_loss_tmo);
> }
> + if (opts->concat) {
> + if (opts->tls) {
> + pr_err("Secure concatenation over TLS is not supported\n");
> + ret = -EINVAL;
> + goto out;
> + }
> + if (opts->tls_key) {
> + pr_err("Cannot specify a TLS key for secure concatenation\n");
> + ret = -EINVAL;
> + goto out;
> + }
> + if (!opts->dhchap_secret) {
> + pr_err("Need to enable DH-CHAP for secure concatenation\n");
> + ret = -EINVAL;
> + goto out;
> + }
> + }
>
> opts->host = nvmf_host_add(hostnqn, &hostid);
> if (IS_ERR(opts->host)) {
> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
> index 21d75dc4a3a0..9cf5b020adba 100644
> --- a/drivers/nvme/host/fabrics.h
> +++ b/drivers/nvme/host/fabrics.h
> @@ -66,6 +66,7 @@ enum {
> NVMF_OPT_TLS = 1 << 25,
> NVMF_OPT_KEYRING = 1 << 26,
> NVMF_OPT_TLS_KEY = 1 << 27,
> + NVMF_OPT_CONCAT = 1 << 28,
> };
>
> /**
> @@ -101,6 +102,7 @@ enum {
> * @keyring: Keyring to use for key lookups
> * @tls_key: TLS key for encrypted connections (TCP)
> * @tls: Start TLS encrypted connections (TCP)
> + * @concat: Enabled Secure channel concatenation (TCP)
> * @disable_sqflow: disable controller sq flow control
> * @hdr_digest: generate/verify header digest (TCP)
> * @data_digest: generate/verify data digest (TCP)
> @@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
> struct key *keyring;
> struct key *tls_key;
> bool tls;
> + bool concat;
> bool disable_sqflow;
> bool hdr_digest;
> bool data_digest;
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index 462d71e2fbf8..5350eb87ec52 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -683,7 +683,7 @@ static ssize_t tls_configured_key_show(struct device *dev,
> struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> struct key *key = ctrl->opts->tls_key;
>
> - if (!key)
> + if (!key || ctrl->opts->concat)
> return 0;
Same comment - move to visible check.
>
> return sysfs_emit(buf, "%08x\n", key_serial(key));
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 5885aa452aa1..d6c085ba0114 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -227,7 +227,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;
I'm wandering if configured/enabled is the best naming...
Maybe move it to a different check helper nvme_tcp_tls_concatenated() ?
btw, shouldn't tls + concat always be passed together? It is getting
confusing...
> }
>
> static inline struct blk_mq_tags *nvme_tcp_tagset(struct nvme_tcp_queue *queue)
> @@ -1942,7 +1942,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);
> @@ -1972,9 +1972,25 @@ static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
> {
> int i, ret;
>
> - if (nvme_tcp_tls_configured(ctrl) && !ctrl->tls_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 (ctrl->tls_pskid &&
> + ctrl->tls_pskid != key_serial(ctrl->opts->tls_key)) {
> + dev_err(ctrl->device, "Stale PSK id %08x\n", ctrl->tls_pskid);
> + ctrl->tls_pskid = 0;
> + }
> + } else if (!ctrl->tls_pskid) {
> + dev_err(ctrl->device, "no PSK negotiated\n");
> + return -ENOKEY;
> + }
> }
>
> for (i = 1; i < ctrl->queue_count; i++) {
> @@ -2205,6 +2221,27 @@ 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;
> + /* TLS is not enabled, do not wipe the key */
> + if (!ctrl->tls_pskid)
> + return;
Gotta say I usually dislike functions that are called and either do
or don't do stuff based on internals.
But if you must, please add _if_needed suffix to this function.
> + /*
> + * Key generated, and TLS enabled:
> + * Revoke the generated 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;
> @@ -2308,6 +2345,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);
> @@ -2348,6 +2386,7 @@ static void nvme_reset_ctrl_work(struct work_struct *work)
> container_of(work, struct nvme_ctrl, reset_work);
> int ret;
>
> + nvme_tcp_revoke_generated_tls_key(ctrl);
> nvme_stop_ctrl(ctrl);
> nvme_tcp_teardown_ctrl(ctrl, false);
>
> @@ -2638,6 +2677,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;
> +
This looks unrelated.
> mutex_lock(&queue->queue_lock);
>
> if (!test_bit(NVME_TCP_Q_LIVE, &queue->flags))
> @@ -2842,7 +2884,7 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
> NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
> NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
> NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
> - NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
> + NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
> .create_ctrl = nvme_tcp_create_ctrl,
> };
>
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index c12a329dd463..ef85cf69cf99 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1669,6 +1669,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;
> };
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 12/16] nvme-tcp: request secure channel concatenation
2024-07-17 22:31 ` Sagi Grimberg
@ 2024-07-18 7:30 ` Hannes Reinecke
0 siblings, 0 replies; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-18 7:30 UTC (permalink / raw)
To: Sagi Grimberg, Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 7/18/24 00:31, Sagi Grimberg wrote:
>
>
> On 17/07/2024 12:10, Hannes Reinecke wrote:
>> Add a fabrics option 'concat' to request secure channel concatenation.
>> When secure channel concatenation is enabled a 'generated PSK' is
>> inserted
>> into the keyring such that it's available after reset.
>>
>> Signed-off-by: Hannes Reinecke <hare@kernel.org>
>> ---
>> drivers/nvme/host/auth.c | 105 ++++++++++++++++++++++++++++++++++--
>> drivers/nvme/host/fabrics.c | 34 ++++++++++--
>> drivers/nvme/host/fabrics.h | 3 ++
>> drivers/nvme/host/sysfs.c | 2 +-
>> drivers/nvme/host/tcp.c | 54 ++++++++++++++++---
>> include/linux/nvme.h | 7 +++
>> 6 files changed, 191 insertions(+), 14 deletions(-)
>>
[ .. ]
>> @@ -831,10 +911,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) {
>
> Please add a comment here on why you are doing this.
>
Okay.
>> 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 +1003,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
>> + */
>
> Seems like you got some whitespaces misalignment, I'd pass the patchset
> through checkpatch.pl
>
Ok.
>> + if (ctrl->opts->concat)
>> + return;
>> for (q = 1; q < ctrl->queue_count; q++) {
>> ret = nvme_auth_negotiate(ctrl, q);
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index c62d8890f3a8..f83ac1292a3b 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -472,8 +472,9 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
>> result = le32_to_cpu(res.u32);
>> ctrl->cntlid = result & 0xFFFF;
>> if (result & (NVME_CONNECT_AUTHREQ_ATR |
>> NVME_CONNECT_AUTHREQ_ASCR)) {
>> - /* Secure concatenation is not implemented */
>> - if (result & NVME_CONNECT_AUTHREQ_ASCR) {
>> + /* Check for secure concatenation */
>> + if ((result & NVME_CONNECT_AUTHREQ_ASCR) &&
>> + !ctrl->opts->concat) {
>
> Shouldn't you check for the dhchap_secret here instead of concat?
>
I _thought_ I had checked for valid option combinations during options
parsing, ie at this position we already know that we have a valid option
combination, and the dhchap_secret will always be set when 'concat is
enabled.
>> dev_warn(ctrl->device,
>> "qid 0: secure concatenation is not supported\n");
>> ret = -EOPNOTSUPP;
>> @@ -550,7 +551,7 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl,
>> u16 qid)
>> /* Secure concatenation is not implemented */
>> if (result & NVME_CONNECT_AUTHREQ_ASCR) {
>> dev_warn(ctrl->device,
>> - "qid 0: secure concatenation is not supported\n");
>> + "qid %d: secure concatenation is not supported\n",
>> qid);
>> ret = -EOPNOTSUPP;
>> goto out_free_data;
>> }
>> @@ -706,6 +707,7 @@ static const match_table_t opt_tokens = {
>> #endif
>> #ifdef CONFIG_NVME_TCP_TLS
>> { NVMF_OPT_TLS, "tls" },
>> + { NVMF_OPT_CONCAT, "concat" },
>> #endif
>> { NVMF_OPT_ERR, NULL }
>> };
>> @@ -735,6 +737,7 @@ static int nvmf_parse_options(struct
>> nvmf_ctrl_options *opts,
>> opts->tls = false;
>> opts->tls_key = NULL;
>> opts->keyring = NULL;
>> + opts->concat = false;
>> options = o = kstrdup(buf, GFP_KERNEL);
>> if (!options)
>> @@ -1053,6 +1056,14 @@ static int nvmf_parse_options(struct
>> nvmf_ctrl_options *opts,
>> }
>> opts->tls = true;
>> break;
>> + case NVMF_OPT_CONCAT:
>> + if (!IS_ENABLED(CONFIG_NVME_TCP_TLS)) {
>> + pr_err("TLS is not supported\n");
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + opts->concat = true;
>> + break;
>> default:
>> pr_warn("unknown parameter or missing value '%s' in ctrl
>> creation request\n",
>> p);
>> @@ -1079,6 +1090,23 @@ static int nvmf_parse_options(struct
>> nvmf_ctrl_options *opts,
>> pr_warn("failfast tmo (%d) larger than controller loss
>> tmo (%d)\n",
>> opts->fast_io_fail_tmo, ctrl_loss_tmo);
>> }
>> + if (opts->concat) {
>> + if (opts->tls) {
>> + pr_err("Secure concatenation over TLS is not supported\n");
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + if (opts->tls_key) {
>> + pr_err("Cannot specify a TLS key for secure
>> concatenation\n");
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + if (!opts->dhchap_secret) {
>> + pr_err("Need to enable DH-CHAP for secure concatenation\n");
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + }
>> opts->host = nvmf_host_add(hostnqn, &hostid);
>> if (IS_ERR(opts->host)) {
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index 21d75dc4a3a0..9cf5b020adba 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -66,6 +66,7 @@ enum {
>> NVMF_OPT_TLS = 1 << 25,
>> NVMF_OPT_KEYRING = 1 << 26,
>> NVMF_OPT_TLS_KEY = 1 << 27,
>> + NVMF_OPT_CONCAT = 1 << 28,
>> };
>> /**
>> @@ -101,6 +102,7 @@ enum {
>> * @keyring: Keyring to use for key lookups
>> * @tls_key: TLS key for encrypted connections (TCP)
>> * @tls: Start TLS encrypted connections (TCP)
>> + * @concat: Enabled Secure channel concatenation (TCP)
>> * @disable_sqflow: disable controller sq flow control
>> * @hdr_digest: generate/verify header digest (TCP)
>> * @data_digest: generate/verify data digest (TCP)
>> @@ -130,6 +132,7 @@ struct nvmf_ctrl_options {
>> struct key *keyring;
>> struct key *tls_key;
>> bool tls;
>> + bool concat;
>> bool disable_sqflow;
>> bool hdr_digest;
>> bool data_digest;
>> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
>> index 462d71e2fbf8..5350eb87ec52 100644
>> --- a/drivers/nvme/host/sysfs.c
>> +++ b/drivers/nvme/host/sysfs.c
>> @@ -683,7 +683,7 @@ static ssize_t tls_configured_key_show(struct
>> device *dev,
>> struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>> struct key *key = ctrl->opts->tls_key;
>> - if (!key)
>> + if (!key || ctrl->opts->concat)
>> return 0;
>
> Same comment - move to visible check.
>
Yeah, okay.
>> return sysfs_emit(buf, "%08x\n", key_serial(key));
>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index 5885aa452aa1..d6c085ba0114 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -227,7 +227,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;
>
> I'm wandering if configured/enabled is the best naming...
> Maybe move it to a different check helper nvme_tcp_tls_concatenated() ?
>
nvme_tcp_tls_configured() is a check to figure out whether TLS handshake
should be attempted. So adding the check for secure concatenation here
seemed logical, and reduced code churn.
> btw, shouldn't tls + concat always be passed together? It is getting
> confusing...
>
--tls enables TLS _before_ the connect command is sent, --concat
enables TLS _after_ the connect command is sent.
The combination of '--tls' and '--concat' would set up a TLS connection,
and then run secure concatenation over TLS.
The spec does allow for that, but I haven't implemented it.
But that's not to say it will never be implemented, so I left this
option open for later and did keep both options separate.
>> }
>> static inline struct blk_mq_tags *nvme_tcp_tagset(struct
>> nvme_tcp_queue *queue)
>> @@ -1942,7 +1942,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);
>> @@ -1972,9 +1972,25 @@ static int __nvme_tcp_alloc_io_queues(struct
>> nvme_ctrl *ctrl)
>> {
>> int i, ret;
>> - if (nvme_tcp_tls_configured(ctrl) && !ctrl->tls_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 (ctrl->tls_pskid &&
>> + ctrl->tls_pskid != key_serial(ctrl->opts->tls_key)) {
>> + dev_err(ctrl->device, "Stale PSK id %08x\n",
>> ctrl->tls_pskid);
>> + ctrl->tls_pskid = 0;
>> + }
>> + } else if (!ctrl->tls_pskid) {
>> + dev_err(ctrl->device, "no PSK negotiated\n");
>> + return -ENOKEY;
>> + }
>> }
>> for (i = 1; i < ctrl->queue_count; i++) {
>> @@ -2205,6 +2221,27 @@ 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;
>> + /* TLS is not enabled, do not wipe the key */
>> + if (!ctrl->tls_pskid)
>> + return;
>
> Gotta say I usually dislike functions that are called and either do
> or don't do stuff based on internals.
>
> But if you must, please add _if_needed suffix to this function.
>
I thought a helper would be good here, but I can easily split off
the checks into a different function (nvme_tcp_key_revoke_needed?).
>> + /*
>> + * Key generated, and TLS enabled:
>> + * Revoke the generated 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;
>> @@ -2308,6 +2345,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);
>> @@ -2348,6 +2386,7 @@ static void nvme_reset_ctrl_work(struct
>> work_struct *work)
>> container_of(work, struct nvme_ctrl, reset_work);
>> int ret;
>> + nvme_tcp_revoke_generated_tls_key(ctrl);
>> nvme_stop_ctrl(ctrl);
>> nvme_tcp_teardown_ctrl(ctrl, false);
>> @@ -2638,6 +2677,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;
>> +
>
> This looks unrelated.
>
Indeed. Will be removing it
>> mutex_lock(&queue->queue_lock);
>> if (!test_bit(NVME_TCP_Q_LIVE, &queue->flags))
>> @@ -2842,7 +2884,7 @@ static struct nvmf_transport_ops
>> nvme_tcp_transport = {
>> NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST |
>> NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
>> NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE | NVMF_OPT_TLS |
>> - NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY,
>> + NVMF_OPT_KEYRING | NVMF_OPT_TLS_KEY | NVMF_OPT_CONCAT,
>> .create_ctrl = nvme_tcp_create_ctrl,
>> };
>> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
>> index c12a329dd463..ef85cf69cf99 100644
>> --- a/include/linux/nvme.h
>> +++ b/include/linux/nvme.h
>> @@ -1669,6 +1669,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;
>> };
>
Thanks for the review!
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 13/16] nvme-fabrics: reset admin connection for secure concatenation
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (11 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 12/16] nvme-tcp: request secure channel concatenation Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 22:32 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 14/16] nvmet-auth: allow to clear DH-HMAC-CHAP keys Hannes Reinecke
` (3 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
When secure concatenation is requested the connection needs to be
reset to enable TLS encryption on the new cnnection.
That implies that the original connection used for the DH-CHAP
negotiation really shouldn't be used, and we should reset as soon
as the DH-CHAP negotiation has succeeded on the admin queue.
Based on an idea from Sagi.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
drivers/nvme/host/tcp.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index d6c085ba0114..0873b3949355 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2251,6 +2251,15 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
if (ret)
return ret;
+ if (new && ctrl->opts && ctrl->opts->concat && !ctrl->tls_pskid) {
+ dev_dbg(ctrl->device, "restart admin queue for secure concatenation\n");
+ nvme_stop_keep_alive(ctrl);
+ nvme_tcp_teardown_admin_queue(ctrl, false);
+ ret = nvme_tcp_configure_admin_queue(ctrl, false);
+ if (ret)
+ return ret;
+ }
+
if (ctrl->icdoff) {
ret = -EOPNOTSUPP;
dev_err(ctrl->device, "icdoff is not supported!\n");
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 14/16] nvmet-auth: allow to clear DH-HMAC-CHAP keys
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (12 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 13/16] nvme-fabrics: reset admin connection for secure concatenation Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 22:32 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 15/16] nvme-target: do not check authentication status for admin commands twice Hannes Reinecke
` (2 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
As we can set DH-HMAC-CHAP keys, we should also be
able to unset them.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/target/auth.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index 7d2633940f9b..aab8a000b733 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -25,6 +25,18 @@ int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
unsigned char key_hash;
char *dhchap_secret;
+ if (!strlen(secret)) {
+ if (set_ctrl) {
+ kfree(host->dhchap_ctrl_secret);
+ host->dhchap_ctrl_secret = NULL;
+ host->dhchap_ctrl_key_hash = 0;
+ } else {
+ kfree(host->dhchap_secret);
+ host->dhchap_secret = NULL;
+ host->dhchap_key_hash = 0;
+ }
+ return 0;
+ }
if (sscanf(secret, "DHHC-1:%hhd:%*s", &key_hash) != 1)
return -EINVAL;
if (key_hash > 3) {
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 15/16] nvme-target: do not check authentication status for admin commands twice
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (13 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 14/16] nvmet-auth: allow to clear DH-HMAC-CHAP keys Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 22:33 ` Sagi Grimberg
2024-07-17 9:10 ` [PATCH 16/16] nvmet-tcp: support secure channel concatenation Hannes Reinecke
2024-07-17 21:38 ` [PATCHv5 00/16] nvme: implement secure concatenation Sagi Grimberg
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
nvmet_check_ctrl_status() checks the authentication status, so
we don't need to do that prior to calling it.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
drivers/nvme/target/admin-cmd.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index f7e1156ac7ec..d64480f01f4a 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -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);
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH 16/16] nvmet-tcp: support secure channel concatenation
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (14 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 15/16] nvme-target: do not check authentication status for admin commands twice Hannes Reinecke
@ 2024-07-17 9:10 ` Hannes Reinecke
2024-07-17 22:36 ` Sagi Grimberg
2024-07-17 21:38 ` [PATCHv5 00/16] nvme: implement secure concatenation Sagi Grimberg
16 siblings, 1 reply; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-17 9:10 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Christoph Hellwig, Keith Busch, linux-nvme, Hannes Reinecke
Evaluate the SC_C flag during DH-CHAP-HMAC negotiation and insert
the generated PSK once negotiation has finished.
Signed-off-by: Hannes Reinecke <hare@kernel.org>
---
drivers/nvme/target/auth.c | 72 +++++++++++++++++++++++++-
drivers/nvme/target/fabrics-cmd-auth.c | 49 +++++++++++++++---
drivers/nvme/target/fabrics-cmd.c | 26 +++++++---
drivers/nvme/target/nvmet.h | 30 ++++++++---
drivers/nvme/target/tcp.c | 25 ++++++++-
5 files changed, 179 insertions(+), 23 deletions(-)
diff --git a/drivers/nvme/target/auth.c b/drivers/nvme/target/auth.c
index aab8a000b733..dc8a619a5174 100644
--- a/drivers/nvme/target/auth.c
+++ b/drivers/nvme/target/auth.c
@@ -15,6 +15,7 @@
#include <linux/ctype.h>
#include <linux/random.h>
#include <linux/nvme-auth.h>
+#include <linux/nvme-keyring.h>
#include <asm/unaligned.h>
#include "nvmet.h"
@@ -138,7 +139,7 @@ int nvmet_setup_dhgroup(struct nvmet_ctrl *ctrl, u8 dhgroup_id)
return ret;
}
-u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl)
+u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
{
int ret = 0;
struct nvmet_host_link *p;
@@ -164,6 +165,11 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl)
goto out_unlock;
}
+ if (nvmet_queue_tls_keyid(req->sq)) {
+ pr_debug("host %s tls enabled\n", ctrl->hostnqn);
+ goto out_unlock;
+ }
+
ret = nvmet_setup_dhgroup(ctrl, host->dhchap_dhgroup_id);
if (ret < 0) {
pr_warn("Failed to setup DH group");
@@ -232,6 +238,9 @@ u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl)
void nvmet_auth_sq_free(struct nvmet_sq *sq)
{
cancel_delayed_work(&sq->auth_expired_work);
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+ sq->tls_key = 0;
+#endif
kfree(sq->dhchap_c1);
sq->dhchap_c1 = NULL;
kfree(sq->dhchap_c2);
@@ -260,6 +269,12 @@ void nvmet_destroy_auth(struct nvmet_ctrl *ctrl)
nvme_auth_free_key(ctrl->ctrl_key);
ctrl->ctrl_key = NULL;
}
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+ if (ctrl->tls_key) {
+ key_put(ctrl->tls_key);
+ ctrl->tls_key = NULL;
+ }
+#endif
}
bool nvmet_check_auth_status(struct nvmet_req *req)
@@ -539,3 +554,58 @@ int nvmet_auth_ctrl_sesskey(struct nvmet_req *req,
return ret;
}
+
+void nvmet_auth_insert_psk(struct nvmet_sq *sq)
+{
+ int hash_len = nvme_auth_hmac_hash_len(sq->ctrl->shash_id);
+ u8 *psk, *digest, *tls_psk;
+ size_t psk_len;
+ int ret;
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+ struct key *tls_key = NULL;
+#endif
+
+ ret = nvme_auth_generate_psk(sq->ctrl->shash_id,
+ sq->dhchap_skey,
+ sq->dhchap_skey_len,
+ sq->dhchap_c1, sq->dhchap_c2,
+ hash_len, &psk, &psk_len);
+ if (ret) {
+ pr_warn("%s: ctrl %d qid %d failed to generate PSK, error %d\n",
+ __func__, sq->ctrl->cntlid, sq->qid, ret);
+ return;
+ }
+ ret = nvme_auth_generate_digest(sq->ctrl->shash_id, psk, psk_len,
+ sq->ctrl->subsysnqn,
+ sq->ctrl->hostnqn, &digest);
+ if (ret) {
+ pr_warn("%s: ctrl %d qid %d failed to generate digest, error %d\n",
+ __func__, sq->ctrl->cntlid, sq->qid, ret);
+ goto out_free_psk;
+ }
+ ret = nvme_auth_derive_tls_psk(sq->ctrl->shash_id, psk, psk_len,
+ digest, &tls_psk);
+ if (ret) {
+ pr_warn("%s: ctrl %d qid %d failed to derive TLS PSK, error %d\n",
+ __func__, sq->ctrl->cntlid, sq->qid, ret);
+ goto out_free_digest;
+ }
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+ tls_key = nvme_tls_psk_refresh(NULL, sq->ctrl->hostnqn, sq->ctrl->subsysnqn,
+ sq->ctrl->shash_id, true, tls_psk, psk_len, digest);
+ if (IS_ERR(tls_key)) {
+ pr_warn("%s: ctrl %d qid %d failed to refresh key, error %ld\n",
+ __func__, sq->ctrl->cntlid, sq->qid, PTR_ERR(tls_key));
+ tls_key = NULL;
+ kfree_sensitive(tls_psk);
+ }
+ if (sq->ctrl->tls_key)
+ key_put(sq->ctrl->tls_key);
+ sq->ctrl->tls_key = tls_key;
+#endif
+
+out_free_digest:
+ kfree_sensitive(digest);
+out_free_psk:
+ kfree_sensitive(psk);
+}
diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index d68c2dae5feb..4c392488c451 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -43,8 +43,26 @@ static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
data->auth_protocol[0].dhchap.halen,
data->auth_protocol[0].dhchap.dhlen);
req->sq->dhchap_tid = le16_to_cpu(data->t_id);
- if (data->sc_c)
- return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+ if (data->sc_c != NVME_AUTH_SECP_NOSC) {
+ if (!IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
+ return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+ /* Secure concatenation can only be enabled on the admin queue */
+ if (req->sq->qid)
+ return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+ switch (data->sc_c) {
+ case NVME_AUTH_SECP_NEWTLSPSK:
+ if (nvmet_queue_tls_keyid(req->sq))
+ return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+ break;
+ case NVME_AUTH_SECP_REPLACETLSPSK:
+ if (!nvmet_queue_tls_keyid(req->sq))
+ return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+ break;
+ default:
+ return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+ }
+ ctrl->concat = true;
+ }
if (data->napd != 1)
return NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE;
@@ -103,6 +121,13 @@ static u8 nvmet_auth_negotiate(struct nvmet_req *req, void *d)
nvme_auth_dhgroup_name(fallback_dhgid));
ctrl->dh_gid = fallback_dhgid;
}
+ if (ctrl->dh_gid == NVME_AUTH_DHGROUP_NULL &&
+ ctrl->concat) {
+ pr_debug("%s: ctrl %d qid %d: NULL DH group invalid "
+ "for secure channel concatenation\n", __func__,
+ ctrl->cntlid, req->sq->qid);
+ return NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH;
+ }
pr_debug("%s: ctrl %d qid %d: selected DH group %s (%d)\n",
__func__, ctrl->cntlid, req->sq->qid,
nvme_auth_dhgroup_name(ctrl->dh_gid), ctrl->dh_gid);
@@ -154,6 +179,12 @@ static u8 nvmet_auth_reply(struct nvmet_req *req, void *d)
kfree(response);
pr_debug("%s: ctrl %d qid %d host authenticated\n",
__func__, ctrl->cntlid, req->sq->qid);
+ if (!data->cvalid && ctrl->concat) {
+ pr_debug("%s: ctrl %d qid %d invalid challenge\n",
+ __func__, ctrl->cntlid, req->sq->qid);
+ return NVME_AUTH_DHCHAP_FAILURE_FAILED;
+ }
+ req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
if (data->cvalid) {
req->sq->dhchap_c2 = kmemdup(data->rval + data->hl, data->hl,
GFP_KERNEL);
@@ -163,11 +194,15 @@ static u8 nvmet_auth_reply(struct nvmet_req *req, void *d)
pr_debug("%s: ctrl %d qid %d challenge %*ph\n",
__func__, ctrl->cntlid, req->sq->qid, data->hl,
req->sq->dhchap_c2);
- } else {
+ }
+ if (req->sq->dhchap_s2 == 0) {
+ if (ctrl->concat)
+ nvmet_auth_insert_psk(req->sq);
req->sq->authenticated = true;
+ kfree(req->sq->dhchap_c2);
req->sq->dhchap_c2 = NULL;
- }
- req->sq->dhchap_s2 = le32_to_cpu(data->seqnum);
+ } else if (!data->cvalid)
+ req->sq->authenticated = true;
return 0;
}
@@ -241,7 +276,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
pr_debug("%s: ctrl %d qid %d reset negotiation\n",
__func__, ctrl->cntlid, req->sq->qid);
if (!req->sq->qid) {
- dhchap_status = nvmet_setup_auth(ctrl);
+ dhchap_status = nvmet_setup_auth(ctrl, req);
if (dhchap_status) {
pr_err("ctrl %d qid 0 failed to setup re-authentication\n",
ctrl->cntlid);
@@ -298,6 +333,8 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
}
goto done_kfree;
case NVME_AUTH_DHCHAP_MESSAGE_SUCCESS2:
+ if (ctrl->concat)
+ nvmet_auth_insert_psk(req->sq);
req->sq->authenticated = true;
pr_debug("%s: ctrl %d qid %d ctrl authenticated\n",
__func__, ctrl->cntlid, req->sq->qid);
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 344ab75d8864..1aabf55ef0a8 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -199,10 +199,19 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
return ret;
}
-static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl)
+static u32 nvmet_connect_result(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
{
+ bool needs_auth = nvmet_has_auth(ctrl, req);
+
+ /* Do not authenticate I/O queues for secure concatenation */
+ if (ctrl->concat && req->sq->qid)
+ needs_auth = false;
+
+ pr_debug("%s: ctrl %d qid %d should %sauthenticate, tls psk %08x\n",
+ __func__, ctrl->cntlid, req->sq->qid,
+ needs_auth ? "" : "not ", nvmet_queue_tls_keyid(req->sq));
return (u32)ctrl->cntlid |
- (nvmet_has_auth(ctrl) ? NVME_CONNECT_AUTHREQ_ATR : 0);
+ (needs_auth ? NVME_CONNECT_AUTHREQ_ATR : 0);
}
static void nvmet_execute_admin_connect(struct nvmet_req *req)
@@ -254,7 +263,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
uuid_copy(&ctrl->hostid, &d->hostid);
- dhchap_status = nvmet_setup_auth(ctrl);
+ dhchap_status = nvmet_setup_auth(ctrl, req);
if (dhchap_status) {
pr_err("Failed to setup authentication, dhchap status %u\n",
dhchap_status);
@@ -272,12 +281,13 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
goto out;
}
- pr_info("creating %s controller %d for subsystem %s for NQN %s%s%s.\n",
+ pr_info("creating %s controller %d for subsystem %s for NQN %s%s%s%s.\n",
nvmet_is_disc_subsys(ctrl->subsys) ? "discovery" : "nvm",
ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn,
- ctrl->pi_support ? " T10-PI is enabled" : "",
- nvmet_has_auth(ctrl) ? " with DH-HMAC-CHAP" : "");
- req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl));
+ ctrl->pi_support ? ", T10-PI" : "",
+ nvmet_has_auth(ctrl, req) ? ", DH-HMAC-CHAP" : "",
+ nvmet_queue_tls_keyid(req->sq) ? ", TLS" : "");
+ req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl, req));
out:
kfree(d);
complete:
@@ -336,7 +346,7 @@ static void nvmet_execute_io_connect(struct nvmet_req *req)
goto out_ctrl_put;
pr_debug("adding queue %d to ctrl %d.\n", qid, ctrl->cntlid);
- req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl));
+ req->cqe->result.u32 = cpu_to_le32(nvmet_connect_result(ctrl, req));
out:
kfree(d);
complete:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 190f55e6d753..9486b43ab822 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -121,6 +121,9 @@ struct nvmet_sq {
u32 dhchap_s2;
u8 *dhchap_skey;
int dhchap_skey_len;
+#endif
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+ struct key *tls_key;
#endif
struct completion free_done;
struct completion confirm_done;
@@ -237,6 +240,7 @@ struct nvmet_ctrl {
u64 err_counter;
struct nvme_error_slot slots[NVMET_ERROR_LOG_SLOTS];
bool pi_support;
+ bool concat;
#ifdef CONFIG_NVME_TARGET_AUTH
struct nvme_dhchap_key *host_key;
struct nvme_dhchap_key *ctrl_key;
@@ -246,6 +250,9 @@ struct nvmet_ctrl {
u8 *dh_key;
size_t dh_keysize;
#endif
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+ struct key *tls_key;
+#endif
};
struct nvmet_subsys {
@@ -716,13 +723,21 @@ static inline void nvmet_req_bio_put(struct nvmet_req *req, struct bio *bio)
bio_put(bio);
}
+#ifdef CONFIG_NVME_TARGET_TCP_TLS
+static inline key_serial_t nvmet_queue_tls_keyid(struct nvmet_sq *sq)
+{
+ return sq->tls_key ? key_serial(sq->tls_key) : 0;
+}
+#else
+static inline key_serial_t nvmet_queue_tls_keyid(struct nvmet_sq *sq) { return 0; }
+#endif
#ifdef CONFIG_NVME_TARGET_AUTH
void nvmet_execute_auth_send(struct nvmet_req *req);
void nvmet_execute_auth_receive(struct nvmet_req *req);
int nvmet_auth_set_key(struct nvmet_host *host, const char *secret,
bool set_ctrl);
int nvmet_auth_set_host_hash(struct nvmet_host *host, const char *hash);
-u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl);
+u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req);
void nvmet_auth_sq_init(struct nvmet_sq *sq);
void nvmet_destroy_auth(struct nvmet_ctrl *ctrl);
void nvmet_auth_sq_free(struct nvmet_sq *sq);
@@ -732,16 +747,18 @@ int nvmet_auth_host_hash(struct nvmet_req *req, u8 *response,
unsigned int hash_len);
int nvmet_auth_ctrl_hash(struct nvmet_req *req, u8 *response,
unsigned int hash_len);
-static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl)
+static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
{
- return ctrl->host_key != NULL;
+ return ctrl->host_key != NULL && !nvmet_queue_tls_keyid(req->sq);
}
int nvmet_auth_ctrl_exponential(struct nvmet_req *req,
u8 *buf, int buf_size);
int nvmet_auth_ctrl_sesskey(struct nvmet_req *req,
u8 *buf, int buf_size);
+void nvmet_auth_insert_psk(struct nvmet_sq *sq);
#else
-static inline u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl)
+static inline u8 nvmet_setup_auth(struct nvmet_ctrl *ctrl,
+ struct nvmet_req *req)
{
return 0;
}
@@ -754,11 +771,12 @@ static inline bool nvmet_check_auth_status(struct nvmet_req *req)
{
return true;
}
-static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl)
+static inline bool nvmet_has_auth(struct nvmet_ctrl *ctrl,
+ struct nvmet_req *req)
{
return false;
}
static inline const char *nvmet_dhchap_dhgroup_name(u8 dhgid) { return NULL; }
+static inline void nvmet_auth_insert_psk(struct nvmet_sq *sq) {};
#endif
-
#endif /* _NVMET_H */
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 5bff0d5464d1..559339fb5ba2 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1073,10 +1073,11 @@ static int nvmet_tcp_done_recv_pdu(struct nvmet_tcp_queue *queue)
if (unlikely(!nvmet_req_init(req, &queue->nvme_cq,
&queue->nvme_sq, &nvmet_tcp_ops))) {
- pr_err("failed cmd %p id %d opcode %d, data_len: %d\n",
+ pr_err("failed cmd %p id %d opcode %d, data_len: %d, status: %04x\n",
req->cmd, req->cmd->common.command_id,
req->cmd->common.opcode,
- le32_to_cpu(req->cmd->common.dptr.sgl.length));
+ le32_to_cpu(req->cmd->common.dptr.sgl.length),
+ le16_to_cpu(req->cqe->status));
nvmet_tcp_handle_req_failure(queue, queue->cmd, req);
return 0;
@@ -1602,6 +1603,9 @@ static void nvmet_tcp_release_queue_work(struct work_struct *w)
/* stop accepting incoming data */
queue->rcv_state = NVMET_TCP_RECV_ERR;
+ if (queue->nvme_sq.tls_key)
+ key_put(queue->nvme_sq.tls_key);
+
nvmet_tcp_uninit_data_in_cmds(queue);
nvmet_sq_destroy(&queue->nvme_sq);
cancel_work_sync(&queue->io_work);
@@ -1807,6 +1811,23 @@ static void nvmet_tcp_tls_handshake_done(void *data, int status,
spin_unlock_bh(&queue->state_lock);
cancel_delayed_work_sync(&queue->tls_handshake_tmo_work);
+
+ if (!status) {
+ struct key *tls_key = nvme_tls_key_lookup(peerid);
+
+ if (IS_ERR(tls_key)) {
+ pr_warn("%s: queue %d failed to lookup key %x\n",
+ __func__, queue->idx, peerid);
+ spin_lock_bh(&queue->state_lock);
+ queue->state = NVMET_TCP_Q_FAILED;
+ spin_unlock_bh(&queue->state_lock);
+ status = PTR_ERR(tls_key);
+ } else {
+ pr_debug("%s: queue %d using TLS PSK %x\n",
+ __func__, queue->idx, peerid);
+ queue->nvme_sq.tls_key = tls_key;
+ }
+ }
if (status)
nvmet_tcp_schedule_release_queue(queue);
else
--
2.35.3
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 16/16] nvmet-tcp: support secure channel concatenation
2024-07-17 9:10 ` [PATCH 16/16] nvmet-tcp: support secure channel concatenation Hannes Reinecke
@ 2024-07-17 22:36 ` Sagi Grimberg
2024-07-18 7:34 ` Hannes Reinecke
0 siblings, 1 reply; 37+ messages in thread
From: Sagi Grimberg @ 2024-07-17 22:36 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 17/07/2024 12:10, Hannes Reinecke wrote:
> Evaluate the SC_C flag during DH-CHAP-HMAC negotiation and insert
> the generated PSK once negotiation has finished.
Will look in details at the patch, but first a question,
IIRC TLS enabled ports should only allow host to connect
over TLS. How does this change now?
Plus, what does the discovery service tell hosts about such
discovery log entries?
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 16/16] nvmet-tcp: support secure channel concatenation
2024-07-17 22:36 ` Sagi Grimberg
@ 2024-07-18 7:34 ` Hannes Reinecke
0 siblings, 0 replies; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-18 7:34 UTC (permalink / raw)
To: Sagi Grimberg, Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 7/18/24 00:36, Sagi Grimberg wrote:
>
>
> On 17/07/2024 12:10, Hannes Reinecke wrote:
>> Evaluate the SC_C flag during DH-CHAP-HMAC negotiation and insert
>> the generated PSK once negotiation has finished.
>
> Will look in details at the patch, but first a question,
> IIRC TLS enabled ports should only allow host to connect
> over TLS. How does this change now?
>
It didn't. We always had the possibility to set 'treq' to
'not required', which then will allow for either.
(In case you wondered: that was implemented with the last
patch to my TLS series, implementing a 'peek' on icreq to
figure out whether TLS should be started or not.)
> Plus, what does the discovery service tell hosts about such
> discovery log entries?
That's what the 'treq' bits are for; 'required' means you
have to use TLS, 'not required' means you _may_ use TLS
(ie both TLS and none-TLS connections are allowed).
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCHv5 00/16] nvme: implement secure concatenation
2024-07-17 9:10 [PATCHv5 00/16] nvme: implement secure concatenation Hannes Reinecke
` (15 preceding siblings ...)
2024-07-17 9:10 ` [PATCH 16/16] nvmet-tcp: support secure channel concatenation Hannes Reinecke
@ 2024-07-17 21:38 ` Sagi Grimberg
2024-07-18 6:44 ` Hannes Reinecke
16 siblings, 1 reply; 37+ messages in thread
From: Sagi Grimberg @ 2024-07-17 21:38 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 17/07/2024 12:10, Hannes Reinecke wrote:
> Hi all,
>
> here's my attempt to implement secure concatenation for NVMe-of TCP
> as outlined in TP8018.
> Secure concatenation means that a TLS PSK is generated from the key
> material negotiated by the DH-HMAC-CHAP protocol, and the TLS PSK
> is then used for a subsequent TLS connection.
> The difference between the original definition of secure concatenation
> and the method outlined in TP8018 is that with TP8018 the connection
> is reset after DH-HMAC-CHAP negotiation, and a new connection is setup
> with the generated TLS PSK.
>
> To implement that Sagi came up with the idea to directly reset the
> admin queue once the DH-CHAP negotiation has completed; that way
> it will be transparent to the upper layers and we don't have to
> worry about exposing queues which should not be used.
I'm glad it worked out. Happy to see that this set is getting in shape!
>
> As usual, comments and reviews are welcome.
>
> Patchset can be found at
> git.kernel.org:/pub/scm/linux/kernel/git/hare/nvme.git
> branch secure-concat.v5
>
> Changes to v4:
> - Rework reset admin queue functionality based on an idea
> from Sagi (thanks!)
> - kbuild robot fixes
> - Fixup dhchap negotiation with non-empty C2 value
Can we split fixes from this patch set? (unless they are only
relevant only to the code introduced here).
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCHv5 00/16] nvme: implement secure concatenation
2024-07-17 21:38 ` [PATCHv5 00/16] nvme: implement secure concatenation Sagi Grimberg
@ 2024-07-18 6:44 ` Hannes Reinecke
0 siblings, 0 replies; 37+ messages in thread
From: Hannes Reinecke @ 2024-07-18 6:44 UTC (permalink / raw)
To: Sagi Grimberg, Hannes Reinecke; +Cc: Christoph Hellwig, Keith Busch, linux-nvme
On 7/17/24 23:38, Sagi Grimberg wrote:
>
>
> On 17/07/2024 12:10, Hannes Reinecke wrote:
>> Hi all,
>>
>> here's my attempt to implement secure concatenation for NVMe-of TCP
>> as outlined in TP8018.
>> Secure concatenation means that a TLS PSK is generated from the key
>> material negotiated by the DH-HMAC-CHAP protocol, and the TLS PSK
>> is then used for a subsequent TLS connection.
>> The difference between the original definition of secure concatenation
>> and the method outlined in TP8018 is that with TP8018 the connection
>> is reset after DH-HMAC-CHAP negotiation, and a new connection is setup
>> with the generated TLS PSK.
>>
>> To implement that Sagi came up with the idea to directly reset the
>> admin queue once the DH-CHAP negotiation has completed; that way
>> it will be transparent to the upper layers and we don't have to
>> worry about exposing queues which should not be used.
>
> I'm glad it worked out. Happy to see that this set is getting in shape!
>
>>
>> As usual, comments and reviews are welcome.
>>
>> Patchset can be found at
>> git.kernel.org:/pub/scm/linux/kernel/git/hare/nvme.git
>> branch secure-concat.v5
>>
>> Changes to v4:
>> - Rework reset admin queue functionality based on an idea
>> from Sagi (thanks!)
>> - kbuild robot fixes
>> - Fixup dhchap negotiation with non-empty C2 value
>
> Can we split fixes from this patch set? (unless they are only
> relevant only to the code introduced here).
Yes, thought about it myself already.
Will be doing that for the next round.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 37+ messages in thread