linux-kernel - Re: [PATCH v3 7/8] nvmet-tcp: Support KeyUpdate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAKmqyKNupEy8dj4B9a8NWymVJTQs_mpeQynLSdCPrO3rBYnByA@mail.gmail.com>
Date: Fri, 17 Oct 2025 11:53:04 +1000
From: Alistair Francis <alistair23@...il.com>
To: Hannes Reinecke <hare@...e.de>
Cc: chuck.lever@...cle.com, hare@...nel.org, 
	kernel-tls-handshake@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, 
	linux-nvme@...ts.infradead.org, linux-nfs@...r.kernel.org, kbusch@...nel.org, 
	axboe@...nel.dk, hch@....de, sagi@...mberg.me, kch@...dia.com, 
	Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH v3 7/8] nvmet-tcp: Support KeyUpdate

On Mon, Oct 6, 2025 at 4:48 PM Hannes Reinecke <hare@...e.de> wrote:
>
> On 10/3/25 06:31, alistair23@...il.com wrote:
> > From: Alistair Francis <alistair.francis@....com>
> >
> > If the nvmet_tcp_try_recv() function return EKEYEXPIRED or if we receive
> > a KeyUpdate handshake type then the underlying TLS keys need to be
> > updated.
> >
> > If the NVMe Host (TLS client) initiates a KeyUpdate this patch will
> > allow the NVMe layer to process the KeyUpdate request and forward the
> > request to userspace. Userspace must then update the key to keep the
> > connection alive.
> >
> > This patch allows us to handle the NVMe host sending a KeyUpdate
> > request without aborting the connection. At this time we don't support
> > initiating a KeyUpdate.
> >
> > Link: https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.3
> > Signed-off-by: Alistair Francis <alistair.francis@....com>
> > ---
> > v3:
> >   - Use a write lock for sk_user_data
> >   - Fix build with CONFIG_NVME_TARGET_TCP_TLS disabled
> >   - Remove unused variable
> > v2:
> >   - Use a helper function for KeyUpdates
> >   - Ensure keep alive timer is stopped
> >   - Wait for TLS KeyUpdate to complete
> >
> >   drivers/nvme/target/tcp.c | 90 ++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 85 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
> > index bee0355195f5..fd59dd3ca632 100644
> > --- a/drivers/nvme/target/tcp.c
> > +++ b/drivers/nvme/target/tcp.c
> > @@ -175,6 +175,7 @@ struct nvmet_tcp_queue {
> >
> >       /* TLS state */
> >       key_serial_t            tls_pskid;
> > +     key_serial_t            user_session_id;
> >       struct delayed_work     tls_handshake_tmo_work;
> >
> >       unsigned long           poll_end;
> > @@ -186,6 +187,8 @@ struct nvmet_tcp_queue {
> >       struct sockaddr_storage sockaddr_peer;
> >       struct work_struct      release_work;
> >
> > +     struct completion       tls_complete;
> > +
> >       int                     idx;
> >       struct list_head        queue_list;
> >
> > @@ -836,6 +839,11 @@ static int nvmet_tcp_try_send_one(struct nvmet_tcp_queue *queue,
> >       return 1;
> >   }
> >
> > +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> > +static int nvmet_tcp_try_peek_pdu(struct nvmet_tcp_queue *queue);
> > +static void nvmet_tcp_tls_handshake_timeout(struct work_struct *w);
> > +#endif
> > +
> >   static int nvmet_tcp_try_send(struct nvmet_tcp_queue *queue,
> >               int budget, int *sends)
> >   {
>
> And we need this why?
>
> > @@ -844,6 +852,13 @@ static int nvmet_tcp_try_send(struct nvmet_tcp_queue *queue,
> >       for (i = 0; i < budget; i++) {
> >               ret = nvmet_tcp_try_send_one(queue, i == budget - 1);
> >               if (unlikely(ret < 0)) {
> > +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> > +                     if (ret == -EKEYEXPIRED &&
> > +                             queue->state != NVMET_TCP_Q_DISCONNECTING &&
> > +                             queue->state != NVMET_TCP_Q_TLS_HANDSHAKE) {
> > +                                     goto done;
> > +                     }
> > +#endif
> >                       nvmet_tcp_socket_error(queue, ret);
> >                       goto done;
> >               } else if (ret == 0) {
>
> See my comment to the host patches. Handling an incoming KeyUpdate is
> vastly different than initiating a KeyUpdate. _and_ the network stack
> will only ever return -EKEYEXPIRED on receive.
> So please split the patches in handling an incoming KeyUpdate and
> initiating a KeyUpdate.

Ok, removed from both.

>
> > @@ -1110,6 +1125,45 @@ static inline bool nvmet_tcp_pdu_valid(u8 type)
> >       return false;
> >   }
> >
> > +#ifdef CONFIG_NVME_TARGET_TCP_TLS
> > +static int update_tls_keys(struct nvmet_tcp_queue *queue)
> > +{
> > +     int ret;
> > +
> > +     cancel_work(&queue->io_work);
> > +     queue->state = NVMET_TCP_Q_TLS_HANDSHAKE;
> > +
> > +     /* Restore the default callbacks before starting upcall */
> > +     write_lock_bh(&queue->sock->sk->sk_callback_lock);
> > +     queue->sock->sk->sk_data_ready =  queue->data_ready;
> > +     queue->sock->sk->sk_state_change = queue->state_change;
> > +     queue->sock->sk->sk_write_space = queue->write_space;
> > +     queue->sock->sk->sk_user_data = NULL;
> > +     write_unlock_bh(&queue->sock->sk->sk_callback_lock);
> > +
> We do have a function for this ...
>
> > +     nvmet_stop_keep_alive_timer(queue->nvme_sq.ctrl);
> > +
> > +     INIT_DELAYED_WORK(&queue->tls_handshake_tmo_work,
> > +                       nvmet_tcp_tls_handshake_timeout);
> > +
> > +     ret = nvmet_tcp_tls_handshake(queue, HANDSHAKE_KEY_UPDATE_TYPE_RECEIVED);
> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = wait_for_completion_interruptible_timeout(&queue->tls_complete, 10 * HZ);
> > +
> > +     if (ret <= 0) {
> > +             tls_handshake_cancel(queue->sock->sk);
> > +             return ret;
> > +     }
> > +
> > +     queue->state = NVMET_TCP_Q_LIVE;
> > +
> > +     return ret;
> > +}
> > +#endif
> > +
> >   static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
> >               struct msghdr *msg, char *cbuf)
> >   {
> > @@ -1135,6 +1189,9 @@ static int nvmet_tcp_tls_record_ok(struct nvmet_tcp_queue *queue,
> >                       ret = -EAGAIN;
> >               }
> >               break;
> > +     case TLS_RECORD_TYPE_HANDSHAKE:
> > +             ret = -EAGAIN;
> > +             break;
>
> Shouldn't this be rather -EKEYEXPIRED?

It shouldn't be. The TLS layer returns -EKEYEXPIRED and we update the
keys. TLS_RECORD_TYPE_HANDSHAKE occurs after the KeyUpdate when the
NVMe layer reads the KeyUpdate message, but we have already acted on
the KeyUpdate from the -EKEYEXPIRED returned by the TLS layer.
Basically the TLS layer handles decoding the KeyUpdate (already in
mainline) and returning -EKEYEXPIRED which kicks off the KeyUpdate.
This is just us clearing the message from the TLS buffer.

Alistair