Re: [PATCH net-next v4 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 3 potential issue(s) to consider:
- [High] tls: KeyUpdate bypassed in tls_sw_sendmsg_locked() after empty open_rec allocation
- [High] tls: KeyUpdate state races when sk_stream_wait_memory() drops lock_sock()
- [Medium] tls: tls_tx_key_update() ignores fatal send errors and loses EPIPE
--

commit 60932b2dae024fb3dd906a58ae73a60d9d48188e
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 Application Data
record when a socket with a registered TX secret reaches 2^24 records or
when the peer requests a KeyUpdate. It also adds the TLS_TX_KEY_UPDATE
socket option to trigger a KeyUpdate immediately and optionally request
one from the peer.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 027c32edaf850..4b2e10000facf 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -805,6 +808,16 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
>  			goto send_end;
>  		}
>  
> +		/* Check before each new record: one call can write many
> +		 * records, and a KeyUpdate reply must go before the next
> +		 * one. The KeyUpdate itself is a handshake record.
> +		 */
> +		if (!ctx->open_rec && record_type == TLS_RECORD_TYPE_DATA) {
> +			ret = tls_tx_maybe_update_key(sk, msg->msg_flags);
> +			if (ret)
> +				goto send_end;
> +		}
> +
>  		if (ctx->open_rec)
>  			rec = ctx->open_rec;
>  		else

[Severity: High]
Can a pending KeyUpdate or the record limit check be bypassed here if an
earlier send attempt left an empty ctx->open_rec allocated?

In net/tls/tls_sw.c, tls_sw_sendmsg_locked() checks
tls_tx_maybe_update_key() only when !ctx->open_rec, and then immediately
allocates ctx->open_rec via tls_get_rec(sk) before checking
sk_stream_memory_free(sk) or calling tls_alloc_encrypted_msg():

net/tls/tls_sw.c:tls_sw_sendmsg_locked() {
    ...
		if (ctx->open_rec)
			rec = ctx->open_rec;
		else
			rec = ctx->open_rec = tls_get_rec(sk);
    ...
		if (!sk_stream_memory_free(sk))
			goto wait_for_sndbuf;
    ...
wait_for_sndbuf:
		sk_set_nospace(sk);
wait_for_memory:
		ret = sk_stream_wait_memory(sk, &timeo);
		if (ret) {
trim_sgl:
			if (ctx->open_rec)
				tls_trim_both_msgs(sk, orig_size);
			goto send_end;
		}
}

When the socket send buffer is full and sk_stream_wait_memory() returns an
error with orig_size == 0 (such as -EAGAIN on a non-blocking socket or
expired SO_SNDTIMEO, or -ERESTARTSYS/-EINTR on a signal),
tls_trim_both_msgs(sk, 0) trims msg_plaintext and msg_encrypted to 0 bytes
but leaves ctx->open_rec non-NULL and empty.

This happens directly when a KeyUpdate is due (either at
TLS_KEYUPDATE_TX_RECORDS or when ctx->tx_key_update_reply is true) and
tls_tx_maybe_update_key() calls tls_tx_key_update() ->
tls_sw_sendmsg_locked() while !sk_stream_memory_free(sk), or when
setsockopt(TLS_TX_KEY_UPDATE) is interrupted by a signal in
sk_stream_wait_memory(), or when a non-blocking sendmsg() returns -EAGAIN
at wait_for_sndbuf before a peer KeyUpdate sets ctx->tx_key_update_reply.

When the caller retries sendmsg() once write space is available,
ctx->open_rec is already non-NULL, so the !ctx->open_rec check is false.
Wouldn't this skip tls_tx_maybe_update_key() (and the -EKEYEXPIRED check at
TLS_KEYUPDATE_MAX) and encrypt the Application Data record under the old
key, as well as cause do_tls_setsockopt_tx_payload_len() to fail with
-EBUSY after an interrupted TLS_TX_KEY_UPDATE?

> @@ -1005,6 +1018,153 @@ 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;
> +	union {
> +		struct cmsghdr cmsg;
> +		u8 buf[CMSG_SPACE(sizeof(record_type))];
> +	} ctrl = {};
> +	struct msghdr msg = {};
> +	struct cmsghdr *cmsg;
> +	struct kvec iov;
> +	int ret;
> +
> +	msg.msg_control = &ctrl;
> +	msg.msg_controllen = sizeof(ctrl);
> +	msg.msg_flags = flags & (MSG_DONTWAIT | MSG_NOSIGNAL);
> +
> +	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]
As reported by Sashiko on RFC v3 [1], what happens in tls_tx_key_update()
in net/tls/tls_sw.c when transmitting the 5-byte KeyUpdate record fails in
tls_tx_records() with an error such as -EPIPE (after shutdown(SHUT_WR)) or
-ECONNRESET?

In tls_sw_sendmsg_locked(), the 5-byte kvec payload is copied into msg_pl
and copied is incremented to 5 before bpf_exec_tx_verdict() ->
tls_push_record() -> tls_tx_records() -> tls_push_sg() ->
tcp_sendmsg_locked() attempts to transmit the encrypted record:

net/tls/tls_sw.c:bpf_exec_tx_verdict() {
	err = tls_push_record(sk, flags, record_type);
	if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
		*copied -= sk_msg_free(sk, msg);
		tls_free_open_rec(sk);
		err = -sk->sk_err;
	}
	return err;
}

Because bpf_exec_tx_verdict() only rolls back *copied when sk->sk_err ==
EBADMSG, copied remains 5 when tls_tx_records() fails with -EPIPE or
-ECONNRESET. At the end of tls_sw_sendmsg_locked():

net/tls/tls_sw.c:tls_sw_sendmsg_locked() {
    ...
end:
	ret = sk_stream_error(sk, msg->msg_flags, ret);
	return copied > 0 ? copied : ret;
}

When ret is -EPIPE, sk_stream_error() calls sock_error(sk), which clears
sk->sk_err back to 0, and tls_sw_sendmsg_locked() then discards ret and
returns copied (5).

Because tls_tx_key_update() only checks ret < 0, doesn't it treat the
failed send as a success, rotate the TX key via tls_keyupdate_rekey(sk, 1),
update tx_key_updates and tx_key_update_reply / tx_key_update_asked, and
return 0 with the unsent KeyUpdate record still sitting on ctx->tx_list and
sk->sk_err cleared to 0?

> +
> +	/* 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;
> +	}
> +
> +	TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXREKEYOK);
> +	tls_ctx->tx_key_updates++;
> +	if (request == TLS_KEY_UPDATE_NOT_REQUESTED)
> +		tls_ctx->tx_key_update_reply = false;
> +
> +	return 0;
> +}
> +
> +/* Send a KeyUpdate if one is due: in reply to the peer's
> + * update_requested (RFC 9846 section 4.7.3), or before the key reaches
> + * its record limit (section 5.5). ChaCha20-Poly1305 has no record limit:
> + * the sequence number would wrap first.
> + */
> +static int tls_tx_maybe_update_key(struct sock *sk, int flags)
> +{
> +	struct tls_context *ctx = tls_get_ctx(sk);
> +	bool limit;
> +
> +	if (likely(!ctx->tx_secret.len))
> +		return 0;
> +
> +	limit = ctx->prot_info.cipher_type != TLS_CIPHER_CHACHA20_POLY1305 &&
> +		get_unaligned_be64(ctx->tx.rec_seq) >= TLS_KEYUPDATE_TX_RECORDS;
> +	if (!limit && !ctx->tx_key_update_reply)
> +		return 0;
> +
> +	if (ctx->tx_key_updates >= TLS_KEYUPDATE_MAX) {
> +		/* Ignore update_requested. At the record limit the
> +		 * connection must end.
> +		 */
> +		ctx->tx_key_update_reply = false;
> +		return limit ? -EKEYEXPIRED : 0;
> +	}
> +
> +	return tls_tx_key_update(sk, TLS_KEY_UPDATE_NOT_REQUESTED, flags);
> +}
> +
> +/**
> + * tls_sw_key_update - send a KeyUpdate now, for TLS_TX_KEY_UPDATE
> + * @sk: socket
> + * @request: non-zero to also ask the peer to update its key
> + *
> + * Called with tx_lock held and the socket locked.
> + *
> + * Return: 0 on success, or a negative errno.
> + */
> +int tls_sw_key_update(struct sock *sk, int request)
> +{
> +	struct tls_context *ctx = tls_get_ctx(sk);
> +	int ret;
> +
> +	if (!ctx->tx_secret.len)
> +		return -EINVAL;
> +
> +	if (request) {
> +		/* Only the kernel sees the answer */
> +		if (!ctx->rx_secret.len)
> +			return -EINVAL;
> +		/* Section 4.7.3: wait for the answer to the previous one */
> +		if (ctx->tx_key_update_asked)
> +			return -EBUSY;
> +	}
> +
> +	if (ctx->tx_key_updates >= TLS_KEYUPDATE_MAX - 1)
> +		return -EKEYEXPIRED;
> +
> +	/* A reply must carry update_not_requested */
> +	if (request && ctx->tx_key_update_reply) {
> +		ret = tls_tx_key_update(sk, TLS_KEY_UPDATE_NOT_REQUESTED,
> +					MSG_NOSIGNAL);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = tls_tx_key_update(sk, request ? TLS_KEY_UPDATE_REQUESTED :
> +				TLS_KEY_UPDATE_NOT_REQUESTED, MSG_NOSIGNAL);
> +	if (!ret && request)
> +		ctx->tx_key_update_asked = true;
> +
> +	return ret;
> +}

[Severity: High]
As reported by Sashiko on RFC v3 [2], can the KeyUpdate state in struct
tls_context race when sk_stream_wait_memory() temporarily drops
lock_sock(sk)?

In net/tls/tls_main.c, do_tls_setsockopt_conf() and
do_tls_setsockopt_secret() only acquire lock_sock(sk) without ctx->tx_lock,
and in net/tls/tls_keyupdate.c, tls_keyupdate_rx() updates
ctx->tx_key_update_asked and ctx->tx_key_update_reply under lock_sock(sk).
Meanwhile, when the socket send buffer is full, tls_sw_sendmsg_locked()
(at wait_for_sndbuf / wait_for_memory) and tcp_sendmsg_locked() (via
tls_push_sg()) call sk_stream_wait_memory(), which releases lock_sock(sk)
while sleeping.

Could this lead to several races while a sender holding ctx->tx_lock sleeps
in sk_stream_wait_memory()?

1. In tls_tx_key_update(), if a concurrent setsockopt(SOL_TLS, TLS_TX) runs
do_tls_setsockopt_conf() while tls_sw_sendmsg_locked() is sleeping,
do_tls_setsockopt_conf() installs a new TX key and zeroes ctx->tx_secret.
When tls_tx_key_update() wakes up and finishes sending the KeyUpdate
record, tls_keyupdate_rekey(sk, 1) sees !ctx->tx_secret.len, returns
-EINVAL, and calls tls_err_abort(sk, -EINVAL), aborting the connection.

2. In tls_sw_key_update(sk, 1), if a concurrent setsockopt(SOL_TLS, TLS_RX)
runs do_tls_setsockopt_conf() while tls_tx_key_update() is sleeping,
do_tls_setsockopt_conf() zeroes ctx->rx_secret and clears
ctx->tx_key_update_asked = false. When tls_tx_key_update() returns 0,
tls_sw_key_update() unconditionally sets ctx->tx_key_update_asked = true
even though rx_secret is no longer registered, leaving tx_key_update_asked
permanently stuck at true.

3. If a concurrent recvmsg() runs tls_keyupdate_rx() while
tls_sw_key_update(sk, 1) is sleeping in sk_stream_wait_memory() before
copying the KeyUpdate message, or while an Application Data sendmsg() is
sleeping at wait_for_sndbuf in tls_sw_sendmsg_locked() after allocating
ctx->open_rec, ctx->tx_key_update_reply is set to true after the caller has
already passed the tx_key_update_reply check. Upon wakeup,
tls_sw_key_update(sk, 1) sends KeyUpdate(update_requested) or
tls_sw_sendmsg_locked() sends Application Data without first sending the
required KeyUpdate(update_not_requested) reply.

[1] https://lore.kernel.org/r/20261030-keyupdate-rfc-v3-4-8e1e32226e51@gmail.com
[2] https://lore.kernel.org/r/20261030-keyupdate-rfc-v3-4-8e1e32226e51@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-keyupdate-rfc-v4-0-f17419972b9f@gmail.com?part=4