[PATCH 08/17] nvme-tcp: sanitize TLS key handling
Sagi Grimberg
sagi at grimberg.me
Sun Apr 7 14:15:33 PDT 2024
On 18/03/2024 17:03, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare at suse.de>
>
> 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).
1. configured/enabled confuse (me at least) in this context.
2. What does "queue is reset" mean?
> 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.
I have some recollection of asking about it, but can you explain why is
a tls_pskid now stored per-queue? What do we lose if tls_pskid moves from
nvme_tcp_queue to nvme_tcp_ctrl ?
> Signed-off-by: Hannes Reinecke <hare at suse.de>
> ---
> drivers/nvme/host/core.c | 1 -
> drivers/nvme/host/sysfs.c | 2 +-
> drivers/nvme/host/tcp.c | 54 +++++++++++++++++++++++++++++----------
> 3 files changed, 42 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 2baf5786a92f..9b601655f423 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4567,7 +4567,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);
> nvme_auth_stop(ctrl);
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index ec581608f16c..1f9e57fbfee3 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -671,7 +671,7 @@ static ssize_t tls_key_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> - struct key *key = ctrl->tls_key;
> + struct key *key = ctrl->opts->tls_key;
>
> if (!key)
> return 0;
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 4a58886e1354..7018dc0dd026 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -163,6 +163,7 @@ struct nvme_tcp_queue {
> __le32 recv_ddgst;
> struct completion tls_complete;
> int tls_err;
> + key_serial_t tls_pskid;
> struct page_frag_cache pf_cache;
>
> void (*state_change)(struct sock *);
> @@ -205,7 +206,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_pskid != 0);
> +}
> +
> +static inline bool nvme_tcp_tls_configured(struct nvme_ctrl *ctrl)
> {
> if (!IS_ENABLED(CONFIG_NVME_TCP_TLS))
> return 0;
> @@ -1418,7 +1427,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);
> }
> @@ -1430,7 +1439,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) {
> @@ -1581,7 +1590,11 @@ static void nvme_tcp_tls_done(void *data, int status, key_serial_t pskid)
> key_put(tls_key);
> queue->tls_err = -EKEYREVOKED;
> } else {
> - ctrl->ctrl.tls_key = tls_key;
> + queue->tls_pskid = key_serial(tls_key);
> + if (qid == 0)
> + ctrl->ctrl.tls_key = tls_key;
> + else
> + key_put(tls_key);
> queue->tls_err = 0;
> }
>
> @@ -1762,7 +1775,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;
> @@ -1823,6 +1836,12 @@ 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, so clear the PSK ID */
> + if (queue->tls_pskid) {
> + dev_dbg(nctrl->device, "queue %d clear TLS PSK %08x\n",
> + qid, queue->tls_pskid);
> + queue->tls_pskid = 0;
> + }
> mutex_unlock(&queue->queue_lock);
> }
>
> @@ -1919,16 +1938,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;
> + }
> }
> }
>
> @@ -1949,15 +1969,17 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
>
> static int __nvme_tcp_alloc_io_queues(struct nvme_ctrl *ctrl)
> {
> + struct nvme_tcp_ctrl *tcp_ctrl = to_tcp_ctrl(ctrl);
> + key_serial_t pskid = ctrl->tls_key ? key_serial(ctrl->tls_key) : 0;
> int i, ret;
>
> - if (nvme_tcp_tls(ctrl) && !ctrl->tls_key) {
> + if (nvme_tcp_tls_configured(ctrl) && !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));
> + ret = nvme_tcp_alloc_queue(ctrl, i, pskid);
> if (ret)
> goto out_free_queues;
> }
> @@ -2138,6 +2160,12 @@ 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_key) {
> + dev_dbg(ctrl->device, "Wipe negotiated TLS_PSK %08x\n",
> + key_serial(ctrl->tls_key));
> + key_put(ctrl->tls_key);
> + ctrl->tls_key = NULL;
> + }
> }
>
> static void nvme_tcp_teardown_io_queues(struct nvme_ctrl *ctrl,
More information about the Linux-nvme
mailing list