[PATCH 11/17] nvme-tcp: request secure channel concatenation
Hannes Reinecke
hare at suse.de
Mon Apr 8 14:48:14 PDT 2024
On 4/8/24 23:09, Sagi Grimberg wrote:
>
>
> On 08/04/2024 8:32, Hannes Reinecke wrote:
>> On 4/7/24 23:41, Sagi Grimberg wrote:
>>>
>>>
>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>> From: Hannes Reinecke <hare at suse.de>
>>>>
>>>> Add a fabrics option 'concat' to request secure channel concatenation.
>>>
>>> maybe name it secconcat ? or secure_concat ?
>>>
>>>> When secure channel concatenation is enabled a 'generated PSK' is
>>>> inserted
>>>> into the keyring such that it's available after reset.
>>>
>>> Umm, I'm not sure what this means? Is this the revoke action? what if
>>> the user passes a keyring? I'm not entirely sure what is the
>>> interface semantics here.
>>>
>> No, this is not the 'revoke' action. This is the modification TP8018
>> brought in. For secure concatenation the initial connection is done
>> without encryption, DH-CHAP is run and generates the TLS PSK. Then
>> the connection is reset, and the new connection starts TLS with the
>> generated key.
>
> Yes, I misread "it's available" and "it's not available". Sorry.
>
>>
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>>>> ---
>>>> drivers/nvme/host/auth.c | 108
>>>> ++++++++++++++++++++++++++++++++++--
>>>> drivers/nvme/host/fabrics.c | 25 ++++++++-
>>>> drivers/nvme/host/fabrics.h | 3 +
>>>> drivers/nvme/host/sysfs.c | 2 +
>>>> drivers/nvme/host/tcp.c | 53 ++++++++++++++++--
>>>> include/linux/nvme.h | 7 +++
>>>> 6 files changed, 186 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>>>> index a264b3ae078b..a4033a8f9607 100644
>>>> --- a/drivers/nvme/host/auth.c
>>>> +++ b/drivers/nvme/host/auth.c
>>>> @@ -12,6 +12,7 @@
>>>> #include "nvme.h"
>>>> #include "fabrics.h"
>>>> #include <linux/nvme-auth.h>
>>>> +#include <linux/nvme-keyring.h>
>>>> #define CHAP_BUF_SIZE 4096
>>>> static struct kmem_cache *nvme_chap_buf_cache;
>>>> @@ -131,7 +132,12 @@ static int
>>>> nvme_auth_set_dhchap_negotiate_data(struct nvme_ctrl *ctrl,
>>>> data->auth_type = NVME_AUTH_COMMON_MESSAGES;
>>>> data->auth_id = NVME_AUTH_DHCHAP_MESSAGE_NEGOTIATE;
>>>> data->t_id = cpu_to_le16(chap->transaction);
>>>> - data->sc_c = 0; /* No secure channel concatenation */
>>>> + if (!ctrl->opts->concat || chap->qid != 0)
>>>> + data->sc_c = NVME_AUTH_SECP_NOSC;
>>>> + else if (ctrl->opts->tls_key)
>>>> + data->sc_c = NVME_AUTH_SECP_REPLACETLSPSK;
>>>> + else
>>>> + data->sc_c = NVME_AUTH_SECP_NEWTLSPSK;
>>>> data->napd = 1;
>>>> data->auth_protocol[0].dhchap.authid = NVME_AUTH_DHCHAP_AUTH_ID;
>>>> data->auth_protocol[0].dhchap.halen = 3;
>>>> @@ -311,8 +317,9 @@ static int
>>>> nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
>>>> data->hl = chap->hash_len;
>>>> data->dhvlen = cpu_to_le16(chap->host_key_len);
>>>> memcpy(data->rval, chap->response, chap->hash_len);
>>>> - if (ctrl->ctrl_key) {
>>>> + if (ctrl->ctrl_key)
>>>> chap->bi_directional = true;
>>>> + if (ctrl->ctrl_key || ctrl->opts->concat) {
>>>> get_random_bytes(chap->c2, chap->hash_len);
>>>> data->cvalid = 1;
>>>> memcpy(data->rval + chap->hash_len, chap->c2,
>>>> @@ -322,7 +329,10 @@ static int
>>>> nvme_auth_set_dhchap_reply_data(struct nvme_ctrl *ctrl,
>>>> } else {
>>>> memset(chap->c2, 0, chap->hash_len);
>>>> }
>>>> - chap->s2 = nvme_auth_get_seqnum();
>>>> + if (ctrl->opts->concat)
>>>> + chap->s2 = 0;
>>>> + else
>>>> + chap->s2 = nvme_auth_get_seqnum();
>>>> data->seqnum = cpu_to_le32(chap->s2);
>>>> if (chap->host_key_len) {
>>>> dev_dbg(ctrl->device, "%s: qid %d host public key %*ph\n",
>>>> @@ -677,6 +687,79 @@ static void nvme_auth_free_dhchap(struct
>>>> nvme_dhchap_queue_context *chap)
>>>> crypto_free_kpp(chap->dh_tfm);
>>>> }
>>>> +static int nvme_auth_secure_concat(struct nvme_ctrl *ctrl,
>>>> + struct nvme_dhchap_queue_context *chap)
>>>> +{
>>>> + u8 *psk, *digest, *tls_psk;
>>>> + struct key *tls_key;
>>>> + size_t psk_len;
>>>> + int ret = 0;
>>>> +
>>>> + if (!chap->sess_key) {
>>>> + dev_warn(ctrl->device,
>>>> + "%s: qid %d no session key negotiated\n",
>>>> + __func__, chap->qid);
>>>> + return -ENOKEY;
>>>> + }
>>>> +
>>>> + psk = nvme_auth_generate_psk(chap->hash_id, chap->sess_key,
>>>> + chap->sess_key_len,
>>>> + chap->c1, chap->c2,
>>>> + chap->hash_len, &psk_len);
>>>> + if (IS_ERR(psk)) {
>>>> + ret = PTR_ERR(psk);
>>>> + dev_warn(ctrl->device,
>>>> + "%s: qid %d failed to generate PSK, error %d\n",
>>>> + __func__, chap->qid, ret);
>>>> + return ret;
>>>> + }
>>>> + dev_dbg(ctrl->device,
>>>> + "%s: generated psk %*ph\n", __func__, (int)psk_len, psk);
>>>> +
>>>> + digest = nvme_auth_generate_digest(chap->hash_id, psk, psk_len,
>>>> + ctrl->opts->subsysnqn,
>>>> + ctrl->opts->host->nqn);
>>>> + if (IS_ERR(digest)) {
>>>> + ret = PTR_ERR(digest);
>>>> + dev_warn(ctrl->device,
>>>> + "%s: qid %d failed to generate digest, error %d\n",
>>>> + __func__, chap->qid, ret);
>>>> + goto out_free_psk;
>>>> + };
>>>> + dev_dbg(ctrl->device, "%s: generated digest %s\n",
>>>> + __func__, digest);
>>>> + tls_psk = nvme_auth_derive_tls_psk(chap->hash_id, psk, psk_len,
>>>> digest);
>>>> + if (IS_ERR(tls_psk)) {
>>>> + ret = PTR_ERR(tls_psk);
>>>> + dev_warn(ctrl->device,
>>>> + "%s: qid %d failed to derive TLS psk, error %d\n",
>>>> + __func__, chap->qid, ret);
>>>> + goto out_free_digest;
>>>> + };
>>>> +
>>>> + tls_key = nvme_tls_psk_refresh(ctrl->opts->keyring,
>>>> ctrl->opts->host->nqn,
>>>> + ctrl->opts->subsysnqn, chap->hash_id,
>>>> + true, tls_psk, psk_len, digest);
>>>> + if (IS_ERR(tls_key)) {
>>>> + ret = PTR_ERR(tls_key);
>>>> + dev_warn(ctrl->device,
>>>> + "%s: qid %d failed to insert generated key, error %d\n",
>>>> + __func__, chap->qid, ret);
>>>> + tls_key = NULL;
>>>> + kfree_sensitive(tls_psk);
>>>> + }
>>>> + if (ctrl->opts->tls_key) {
>>>> + key_revoke(ctrl->opts->tls_key);
>>>> + key_put(ctrl->opts->tls_key);
>>>> + }
>>>
>>> Question, is this for the case where the user passes both tls_key and
>>> concat? Is this something that we expect users to do? What is the
>>> benefit?
>
> Answer?
>
Once DH-HMAC-CHAP is run a new key is generated, and that key should be
used for the subsequent TLS connection. So I need to find a location
where to store the key, _and_ be sure that the key reference is kept
intact during controller reset (which I need to do to establish the TLS
connection).
So I store the generated key in 'opts->tls_key', as this structure is
kept around throughout the lifetime of a connection.
I am aware that the controller structure has a 'tls_key' entry, too, but
this holds the key being negotiated from the TLS handshake, so we cannot
store it there.
For reference, the schedule is:
ctrl->opts->tls_key: key to be used for the TLS handshake, overriding
the default selection scheme
ctrl->tls_key: negotiated key from TLS handshake on the admin channel.
Defines the key to be used for the I/O channels; I/O channels do
_not_ use the default selection scheme
queue->tls_pskid: serial number of the negotiated keys from the TLS
handshake.
I do agree that we could use a flag instead of the per-queue tls pskid,
but then we'll lose the information _which_ key had been used for
the TLS handshake. Keys will be regenerated after each DH-CHAP
negotiation, and the spec allows for running DH-CHAP negotiation
on the encrypted connection.
(Not that it's currently implemented, but we might want to eventually).
In that cases we would need to know which key is currently active
on the TLS connection. That information is pretty much opaque as
the in-kernel crypto is using derived key information, and we cannot
get the original key information via any other means.
So I'd rather keep it.
Cheers,
Hannes
More information about the Linux-nvme
mailing list