Re: [PATCH RFC net-next 4/5] tls: send a KeyUpdate at the record limit and on request

[sashiko-bot@kernel.org]

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@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