[PATCH 11/17] nvme-tcp: request secure channel concatenation
Sagi Grimberg
sagi at grimberg.me
Sun Apr 21 04:14:40 PDT 2024
On 09/04/2024 0:48, Hannes Reinecke wrote:
> On 4/8/24 23:09, Sagi Grimberg wrote:
>>
>>
>> On 08/04/2024 8:32, Hannes Reinecke wrote:
>>> On 4/7/24 23:41, Sagi Grimberg wrote:
>>>>
>>>>
>>>> On 18/03/2024 17:03, Hannes Reinecke wrote:
>>>>> From: Hannes Reinecke <hare 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.
Why do you store a generated key in opts? That is supposed to hold what
the user passed.
> I am aware that the controller structure has a 'tls_key' entry, too, but
> this holds the key being negotiated from the TLS handshake, so we cannot
> store it there.
I suggest to introduce a new tls_generated_key ctrl member. Lets not
overload
opts->tls_key which is designed to hold the a user driven tls_key
(unless I'm misunderstanding
what you are saying?).
> For reference, the schedule is:
> ctrl->opts->tls_key: key to be used for the TLS handshake, overriding
> the default selection scheme
This is what the user passes in?
> ctrl->tls_key: negotiated key from TLS handshake on the admin channel.
> Defines the key to be used for the I/O channels; I/O channels do
> _not_ use the default selection scheme
> queue->tls_pskid: serial number of the negotiated keys from the TLS
> handshake.
what do you mean by "negotiated keys"? plural?
> I do agree that we could use a flag instead of the per-queue tls pskid,
> but then we'll lose the information _which_ key had been used for
> the TLS handshake.
Isn't it stored in the ctrl?
> Keys will be regenerated after each DH-CHAP
> negotiation, and the spec allows for running DH-CHAP negotiation
> on the encrypted connection.
> (Not that it's currently implemented, but we might want to eventually).
> In that cases we would need to know which key is currently active
> on the TLS connection. That information is pretty much opaque as
> the in-kernel crypto is using derived key information, and we cannot
> get the original key information via any other means.
> So I'd rather keep it.
Can you give a concrete example of what info you are missing?
I'm not following...
More information about the Linux-nvme
mailing list