[PATCH RFC net-next 4/5] tls: send a KeyUpdate at the record limit and on request
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Mon Oct 26 17:37:08 PDT 2026
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] tls: missing MSG_NOSIGNAL in tls_tx_key_update() causes unexpected SIGPIPE
- [Medium] tls: stale KeyUpdate state after manual rekey in do_tls_setsockopt_conf()
- [Medium] tls: swallowed TCP send errors and cleared sk_err in tls_tx_key_update()
- [Medium] tls: lock_sock() dropped in tls_tx_key_update() races with setsockopt and rx
--
commit 56c21ecdb1fcce3eea994287aff8609b92227c81
Author: Martin Belanger <martin.belanger at dell.com>
tls: send a KeyUpdate at the record limit and on request
This commit sends a TLS 1.3 KeyUpdate before the next record when the TX key
reaches 2^24 records or when the peer has requested a KeyUpdate. It also adds
the TLS_TX_KEY_UPDATE socket option to trigger an immediate KeyUpdate and
optionally request one from the peer.
> diff --git a/net/tls/tls_keyupdate.c b/net/tls/tls_keyupdate.c
> index 543bf57599c7b..851d5b54b336d 100644
> --- a/net/tls/tls_keyupdate.c
> +++ b/net/tls/tls_keyupdate.c
> @@ -146,5 +146,10 @@ int tls_keyupdate_rx(struct sock *sk, struct sk_buff *skb)
> }
> TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXREKEYOK);
>
> + /* Any KeyUpdate from the peer answers our update_requested */
> + ctx->tx_key_update_asked = false;
> + if (msg[4] == TLS_KEY_UPDATE_REQUESTED)
> + ctx->tx_key_update_reply = true;
> +
> return 0;
> }
[Severity: Medium]
What happens to ctx->tx_key_update_asked and ctx->tx_key_update_reply if
userspace later performs a manual rekey via setsockopt(TLS_TX) or
setsockopt(TLS_RX)?
In net/tls/tls_main.c, do_tls_setsockopt_conf() clears the registered
secrets when updating an existing key:
if (update) {
/* The registered secret no longer matches the key. The RX
* secret also needs the TX one, see do_tls_setsockopt_secret().
*/
if (tx)
memzero_explicit(&ctx->tx_secret,
sizeof(ctx->tx_secret));
memzero_explicit(&ctx->rx_secret, sizeof(ctx->rx_secret));
return 0;
}
If userspace calls setsockopt(TLS_TX_KEY_UPDATE) with request = 1 (setting
ctx->tx_key_update_asked = true) and then manually rekeys via TLS_TX or
TLS_RX, ctx->rx_secret is zeroed so the peer's KeyUpdate is handled in
userspace rather than tls_keyupdate_rx(). If userspace subsequently registers
new traffic secrets, ctx->tx_key_update_asked remains true and future
TLS_TX_KEY_UPDATE requests with value = 1 will permanently fail with -EBUSY
in tls_sw_key_update(). Similarly, a stale ctx->tx_key_update_reply = true
across a manual TX rekey and secret registration would cause the next
tls_sw_sendmsg() to emit an unintended extra KeyUpdate.
Should do_tls_setsockopt_conf() also clear ctx->tx_key_update_asked (and
ctx->tx_key_update_reply when tx is true)?
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index a314fa638dc36..4acedddfb01e8 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
[ ... ]
> @@ -1005,6 +1006,150 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
> return copied > 0 ? copied : ret;
> }
>
> +/* RFC 9846 section 5.5 allows 2^24.5 full-size records per AES-GCM key.
> + * Send the KeyUpdate a little earlier.
> + */
> +#define TLS_KEYUPDATE_TX_RECORDS BIT_ULL(24)
> +
> +/* Section 4.7.3: the epoch must not exceed 2^48-1. The application
> + * traffic secrets start at epoch 3.
> + */
> +#define TLS_KEYUPDATE_MAX (BIT_ULL(48) - 4)
> +
> +/* Send a KeyUpdate with the current key, then install the next key.
> + * Called with tx_lock held and the socket locked.
> + */
> +static int tls_tx_key_update(struct sock *sk, u8 request, int flags)
> +{
> + u8 body[TLS_KEYUPDATE_MSG_SIZE] = {
> + TLS_HANDSHAKE_KEYUPDATE, 0, 0, 1, request
> + };
> + struct tls_context *tls_ctx = tls_get_ctx(sk);
> + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> + u8 record_type = TLS_RECORD_TYPE_HANDSHAKE;
> + u8 buf[CMSG_SPACE(sizeof(record_type))];
> + struct msghdr msg = {};
> + struct cmsghdr *cmsg;
> + struct kvec iov;
> + int ret;
> +
> + memset(buf, 0, sizeof(buf));
> + msg.msg_control = buf;
> + msg.msg_controllen = sizeof(buf);
> + msg.msg_flags = flags & MSG_DONTWAIT;
[Severity: Medium]
Could masking msg.msg_flags with only MSG_DONTWAIT here cause unexpected
SIGPIPE delivery?
When tls_sw_sendmsg() is called with MSG_NOSIGNAL and a KeyUpdate is due,
tls_tx_key_update_if_due() passes msg->msg_flags into tls_tx_key_update(),
which strips MSG_NOSIGNAL before calling tls_sw_sendmsg_locked(). Likewise,
tls_sw_key_update() called from do_tls_setsockopt_key_update() passes
flags = 0.
If the connection has failed with -EPIPE (for example when SEND_SHUTDOWN is
set), tls_sw_sendmsg_locked() passes msg.msg_flags to tcp_sendmsg_locked()
and sk_stream_error():
net/core/stream.c:sk_stream_error() {
if (err == -EPIPE)
err = sock_error(sk) ? : -EPIPE;
if (err == -EPIPE && !(flags & MSG_NOSIGNAL))
send_sig(SIGPIPE, current, 0);
return err;
}
This sends SIGPIPE to the calling task during setsockopt(TLS_TX_KEY_UPDATE)
or during a sendmsg() call that explicitly requested MSG_NOSIGNAL. Should
tls_tx_key_update() preserve MSG_NOSIGNAL from flags in the sendmsg() path
and set MSG_NOSIGNAL in the setsockopt() path?
> +
> + cmsg = CMSG_FIRSTHDR(&msg);
> + cmsg->cmsg_level = SOL_TLS;
> + cmsg->cmsg_type = TLS_SET_RECORD_TYPE;
> + cmsg->cmsg_len = CMSG_LEN(sizeof(record_type));
> + memcpy(CMSG_DATA(cmsg), &record_type, sizeof(record_type));
> +
> + iov.iov_base = body;
> + iov.iov_len = sizeof(body);
> + iov_iter_kvec(&msg.msg_iter, ITER_SOURCE, &iov, 1, sizeof(body));
> +
> + /* A control record closes the open record first, so all data
> + * written so far goes out with the current key, ahead of the
> + * KeyUpdate.
> + */
> + ret = tls_sw_sendmsg_locked(sk, &msg, sizeof(body));
> + if (ret < 0)
> + return ret;
[Severity: Medium]
Can tls_sw_sendmsg_locked() return sizeof(body) even when transmitting the
KeyUpdate record fails at the TCP layer?
Inside tls_sw_sendmsg_locked(), once the 5-byte KeyUpdate payload is copied
into msg_pl, copied is incremented to 5 before bpf_exec_tx_verdict() ->
tls_push_record() is called. If synchronous encryption in tls_do_encryption()
succeeds and moves the record to ctx->tx_list, but tls_tx_records() ->
tls_push_sg() -> tcp_sendmsg_locked() fails (for example with -EPIPE because
SEND_SHUTDOWN is set while sk->sk_err was initially 0), tls_tx_records()
calls tls_err_abort(sk, -EPIPE) and returns -EPIPE.
Because bpf_exec_tx_verdict() only rolls back copied when
sk->sk_err == EBADMSG, copied remains 5. At the end of
tls_sw_sendmsg_locked():
end:
ret = sk_stream_error(sk, msg->msg_flags, ret);
return copied > 0 ? copied : ret;
sk_stream_error() consumes and clears sk->sk_err back to 0 via
sock_error(sk), and tls_sw_sendmsg_locked() returns 5 instead of -EPIPE.
tls_tx_key_update() then sees ret == 5, rotates the TX key via
tls_keyupdate_rekey(sk, 1), increments LINUX_MIB_TLSTXREKEYOK and
tx_key_updates, and returns 0 on a broken socket with sk->sk_err cleared.
> +
> + /* Nothing may be encrypted with the next key before the KeyUpdate
> + * is encrypted with the current one.
> + */
> + ret = tls_encrypt_async_wait(ctx);
> + if (!ret)
> + ret = tls_keyupdate_rekey(sk, 1);
> + if (ret) {
> + /* The peer expects the next key: the connection is lost */
> + TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXREKEYERROR);
> + tls_err_abort(sk, ret);
> + return ret;
> + }
[Severity: Medium]
Is KeyUpdate state protected if tls_sw_sendmsg_locked() drops the socket lock
while waiting for send buffer memory?
Both tls_tx_key_update_if_due() and tls_sw_key_update() check
ctx->tx_secret.len, ctx->rx_secret.len, ctx->tx_key_update_asked, and
ctx->tx_key_update_reply under lock_sock(sk) and ctx->tx_lock, then call
tls_tx_key_update(). Inside tls_tx_key_update(), tls_sw_sendmsg_locked() (or
tcp_sendmsg_locked() via tls_tx_records() -> tls_push_sg()) can call
sk_stream_wait_memory() when the send buffer is full, which temporarily drops
lock_sock(sk) while keeping only ctx->tx_lock held.
However, the setsockopt handlers for TLS_TX / TLS_RX
(do_tls_setsockopt_conf()) and TLS_TX_TRAFFIC_SECRET / TLS_RX_TRAFFIC_SECRET
(do_tls_setsockopt_secret()), as well as the RX path (tls_sw_recvmsg() ->
tls_keyupdate_rx()), only acquire lock_sock(sk) and do not take ctx->tx_lock.
While tls_tx_key_update() is sleeping in sk_stream_wait_memory():
1. A concurrent setsockopt(TLS_TX) in do_tls_setsockopt_conf() can replace
the TX key and zero ctx->tx_secret and ctx->rx_secret. When
tls_tx_key_update() wakes up, it finishes sending the KeyUpdate under the
already-replaced key and then calls tls_keyupdate_rekey(sk, 1), which sees
!ctx->tx_secret.len, returns -EINVAL, and aborts the connection via
tls_err_abort(sk, -EINVAL).
2. A concurrent setsockopt(TLS_RX) can zero ctx->rx_secret after
tls_sw_key_update(sk, 1) has already verified ctx->rx_secret.len, leaving
ctx->tx_key_update_asked = true with no registered RX secret to ever clear
it.
3. A concurrent recvmsg() in tls_keyupdate_rx() can set
ctx->tx_key_update_reply = true while tls_sw_key_update(sk, 1) is sleeping
before sending KeyUpdate(update_requested), so tls_tx_key_update() does
not clear tx_key_update_reply.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261025-keyupdate-rfc-v1-0-60b7756eccbd@gmail.com?part=4
More information about the Linux-nvme
mailing list