Re: [PATCH 02/16] nvme-tcp: sanitize TLS key handling

[Hannes Reinecke <hare@suse.de>]

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