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

[Sagi Grimberg]

On 19/07/2024 11:38, 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 on an unencrypted connection
> (ie with TLS configured, but not enabled), and then the queue is reset to
> run over TLS (ie TLS configured _and_ enabled).
> So to differentiate between those two states store the generated
> key in opts->tls_key (as we're using the same TLS key for all queues),
> the key serial of the resulting TLS handshake in ctrl->tls_pskid
> (to signal that TLS on the admin queue is enabled), and a simple
> flag for the queues to indicated that TLS has been enabled.
>
> Signed-off-by: Hannes Reinecke <hare at kernel.org>
> Reviewed-by: Christoph Hellwig <hch at lst.de>
> ---
>   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 e78ef31eeef0..663fa14162ab 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4675,7 +4675,6 @@ static void nvme_free_ctrl(struct device *dev)
>   
>   	if (!subsys || ctrl->instance != subsys->instance)
>   		ida_free(&nvme_instance_ida, ctrl->instance);
> -	key_put(ctrl->tls_key);
>   	nvme_free_cels(ctrl);
>   	nvme_mpath_uninit(ctrl);
>   	cleanup_srcu_struct(&ctrl->srcu);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 9ac587e64166..185fb986a5cc 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -373,7 +373,7 @@ struct nvme_ctrl {
>   	struct nvme_dhchap_key *ctrl_key;
>   	u16 transaction;
>   #endif
> -	struct key *tls_key;
> +	key_serial_t tls_pskid;
>   
>   	/* Power saving configuration */
>   	u64 ps_max_latency_us;
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index ba05faaac562..72675b59a7a7 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -670,9 +670,9 @@ static ssize_t tls_key_show(struct device *dev,
>   {
>   	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>   
> -	if (!ctrl->tls_key)
> +	if (!ctrl->tls_pskid)
>   		return 0;
> -	return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
> +	return sysfs_emit(buf, "%08x", ctrl->tls_pskid);
>   }
>   static DEVICE_ATTR_RO(tls_key);
>   #endif
> 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;
> +}

One suggestion.

Can we call it:
nvme_tcp_queue_tls() ? It will first clarify that this is a queue level 
setting,
and will disambiguate (at least for me) the difference from 
nvme_tcp_tls_configured().