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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 3 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Race in the kernel KeyUpdate path: its state can change while
  the socket lock is dropped.
- [Medium] The record limit is wrong for AES-128-CCM.
- [Medium] setsockopt(SOL_TLS, TLS_TX_KEY_UPDATE) can block indefinitely
  and ignores O_NONBLOCK.
- [Low] Documentation/networking/tls.rst does not match the
  implementation in two places.

--

tls: send a KeyUpdate at the record limit and on request

With this commit, the kernel sends a TLS 1.3 KeyUpdate before the next
Application Data record in two cases: when the TX key has protected 2^24
records (except ChaCha20-Poly1305), or when the peer sent
update_requested. It also adds the TLS_TX_KEY_UPDATE socket option to
send a KeyUpdate on demand and, optionally, ask the peer to update too.

> diff --git a/Documentation/networking/tls.rst b/Documentation/networking/tls.rst
> index 2e011bd1ce4ae..69752a6d67df4 100644
> --- a/Documentation/networking/tls.rst
> +++ b/Documentation/networking/tls.rst
> @@ -326,6 +326,23 @@ direction is then left to the application again.
>  
>  The secrets cannot be read back.
>  
> +With a registered TX secret, the kernel sends a KeyUpdate before the key
> +reaches its record limit (RFC 9846, section 5.5): after 2^24 records, except
> +for ChaCha20-Poly1305, which has no limit. It also sends one before the next
> +record when a received KeyUpdate asks for it. With a registered RX secret,
> +the kernel processes a received KeyUpdate and does not pass the record to
> +the application. A peer that sends more than 8 KeyUpdates in one second ends
> +the connection.
> +
> +TLS_TX_KEY_UPDATE
> +~~~~~~~~~~~~~~~~~
> +
> +TLS 1.3 only. Send a KeyUpdate now and install the next TX key. Needs a
> +registered TX secret. The value is an ``int``: 0 to send a KeyUpdate, 1 to
> +also ask the peer to update its key. Asking the peer needs a registered RX
> +secret, and fails with ``EBUSY`` until the peer has answered the previous
> +request.

[Severity: Low]
This isn't a bug, but two parts of this text don't seem to match the code.

The text says the reply is sent "before the next record". In
tls_sw_sendmsg_locked(), the check only runs for data records:

    if ((!ctx->open_rec || !ctx->open_rec->msg_plaintext.sg.size) &&
        record_type == TLS_RECORD_TYPE_DATA) {
            ret = tls_tx_maybe_update_key(sk, msg->msg_flags);

Suppose tx_key_update_reply is set and the application sends an alert or
handshake record with TLS_SET_RECORD_TYPE. That record goes out before
the reply. This matches RFC 9846 section 4.7.3 and the commit message
("before the next Application Data record"). Should the documentation
say the same?

For TLS_TX_KEY_UPDATE, the text says "Send a KeyUpdate now and install
the next TX key". If the value is 1 and a peer request is pending,
tls_sw_key_update() calls tls_tx_key_update() twice:

    if (request && ctx->tx_key_update_reply) {
            ret = tls_tx_key_update(sk, TLS_KEY_UPDATE_NOT_REQUESTED,
                                    MSG_NOSIGNAL);
            ...
    }

    ret = tls_tx_key_update(sk, request ? TLS_KEY_UPDATE_REQUESTED : ...

That sends two KeyUpdates and moves the TX key forward two generations in
one call. Could the documentation mention this, and also the EKEYEXPIRED
return?

[ ... ]

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

[Severity: Medium]
The comment bases 2^24 on the AES-GCM figure. However,
tls_tx_maybe_update_key() applies it to every cipher except
ChaCha20-Poly1305:

    limit = ctx->prot_info.cipher_type != TLS_CIPHER_CHACHA20_POLY1305 &&
            get_unaligned_be64(ctx->tx.rec_seq) >= TLS_KEYUPDATE_TX_RECORDS;

tls_traffic_secret_len() accepts TLS_CIPHER_AES_CCM_128. An AES-128-CCM
socket can therefore register a TX secret and reach this path.

Is 2^24 the right limit for AES-128-CCM? RFC 9147 Appendix B uses the
CCM confidentiality bound (2l * q)^2 / 2^n. With l = 2^10 blocks and
n = 128, that gives about 2^23 full-size records for the 2^-60 target
used by TLS 1.3. At 2^24 records the attacker's advantage is closer to
2^-58.

Should CCM get a lower threshold? If not, could the comment explain why
the GCM figure also covers CCM?

[ ... ]

> +	/* 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);
> +	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]
Can the TX key state change between the caller's tx_secret check and
this tls_keyupdate_rekey() call?

The nested tls_sw_sendmsg_locked() can drop the socket lock while
tx_lock is still held. For example, when the send buffer is full:

    wait_for_sndbuf:
            sk_set_nospace(sk);
    wait_for_memory:
            ret = sk_stream_wait_memory(sk, &timeo);

Pushing the record through tcp_sendmsg_locked() can drop it as well.

In do_tls_setsockopt(), TLS_TX and TLS_TX_TRAFFIC_SECRET only take
lock_sock(), not tx_lock. A TLS_TX rekey from another thread can
therefore run inside this window. do_tls_setsockopt_conf() then does:

    if (tx) {
            memzero_explicit(&ctx->tx_secret,
                             sizeof(ctx->tx_secret));
            ctx->tx_key_update_reply = false;
    }

When this thread resumes, tls_keyupdate_rekey(sk, 1) sees
secret->len == 0 and returns -EINVAL. tls_err_abort() then aborts the
connection.

If the wait happened before the KeyUpdate record was copied, the
KeyUpdate is also encrypted with the key user space just installed. The
peer then receives a KeyUpdate that user space doesn't know about.

tls.rst documents handing KeyUpdate back to the application with TLS_TX
as supported. The application can't tell when sendmsg() will insert an
automatic KeyUpdate.

Should TLS_TX and TLS_TX_TRAFFIC_SECRET take tx_lock, as
TLS_TX_KEY_UPDATE and TLS_TX_MAX_PAYLOAD_LEN do?

[ ... ]

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

[Severity: Medium]
Can setsockopt(SOL_TLS, TLS_TX_KEY_UPDATE) block indefinitely on a
non-blocking socket?

Both calls here pass only MSG_NOSIGNAL, and tls_tx_key_update() keeps
only these flags:

    msg.msg_flags = flags & (MSG_DONTWAIT | MSG_NOSIGNAL);

MSG_DONTWAIT is therefore never set on this path, and
tls_sw_sendmsg_locked() uses sock_sndtimeo(sk, 0). That is sk_sndtimeo,
which defaults to MAX_SCHEDULE_TIMEOUT.

If the send buffer is full, for example because the peer stopped
reading, the call sleeps in sk_stream_wait_memory() with tx_lock held.
It waits until space frees up, an error or shutdown happens, or a signal
arrives. sendmsg() turns O_NONBLOCK into MSG_DONTWAIT, but setsockopt()
has no such conversion.

tls.rst doesn't mention this blocking. It also lists only EBUSY. It
leaves out EINVAL (no TX secret, no RX secret with value 1, or not
TLS_SW), EKEYEXPIRED, and send errors such as EAGAIN after SO_SNDTIMEO,
EPIPE, or EINTR.

With value 1 and a pending reply, the first KeyUpdate can succeed and
the second fail. The call then returns an error even though one
KeyUpdate has gone out and the key has moved forward. Should this be
documented, or handled differently?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105-keyupdate-rfc-v6-0-99b695f9eb4f%40gmail.com