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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] tls: tls_tx_key_update() ignores fatal send errors and loses
  EPIPE
--

commit cc9936b6fa0f39f256d5df2f7358e9099a857211
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 handshake record automatically when a
socket with a registered TX secret reaches 2^24 records or when replying to a
peer's update_requested flag. It also adds the TLS_TX_KEY_UPDATE socket option
to allow applications to trigger a KeyUpdate on demand.

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

[ ... ]

> @@ -1005,6 +1021,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)
> +{

[ ... ]

> +	/* 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;
> +
> +	/* 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);

[Severity: Medium]
As reported by Sashiko on v5 [1], what happens if transmitting the KeyUpdate
record in tls_sw_sendmsg_locked() fails with a fatal TCP error such as -EPIPE
(for example on a write-shutdown socket) or -ECONNRESET?

Inside tls_sw_sendmsg_locked(), the 5-byte KeyUpdate body is copied into
msg_pl and copied is incremented to 5 before pushing the record:

  tls_sw_sendmsg_locked()
    sk_msg_memcopy_from_iter()
    copied += try_to_copy;
    bpf_exec_tx_verdict()
      tls_push_record()
        tls_tx_records()
          tls_push_sg()
            tcp_sendmsg_locked() -> returns -EPIPE or -ECONNRESET
          tls_err_abort(sk, rc) -> sets sk->sk_err

In bpf_exec_tx_verdict(), *copied is only rolled back when sk->sk_err is
EBADMSG:

	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;

Then at the end of tls_sw_sendmsg_locked(), sk_stream_error() is called
before returning:

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 because
copied > 0 and returns 5.

Because tls_tx_key_update() only checks for ret < 0, won't it treat the failed
transmission as a success, rotate the TX key via tls_keyupdate_rekey(sk, 1),
increment LINUX_MIB_TLSTXREKEYOK and tls_ctx->tx_key_updates, and return 0
while the unsent KeyUpdate record remains queued on ctx->tx_list (and with
sk->sk_err cleared to 0 in the -EPIPE case)?

[1] https://lore.kernel.org/r/sashiko-outbox-198141@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-keyupdate-rfc-v6-0-99b695f9eb4f@gmail.com?part=5