Re: [PATCH net v2 1/5] tls: Break the connection after a burst of empty data records

[netdev-bot+sashiko@kernel.org]

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

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

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] ctx->empty_run counts only consecutive empty DATA records.
- [Low] Documentation/networking/tls.rst (line 123) still says only
  '``EBADMSG`` is returned if decryption failed for any other reason.'…
- [Low] In tls_sw_recvmsg() the new check 'if (is_peek && chunk)'
  (net/tls/tls_sw.c:1963) applies to every record type, not only…

--

tls: Break the connection after a burst of empty data records

This patch adds a per-reader counter of consecutive empty TLS data
records. Once more than 128 are decrypted without the reader sleeping or
returning to its caller, it aborts the strparser with EBADMSG. It also
makes tls_sw_recvmsg() decrypt empty records synchronously, and consume
them rather than queue them on rx_list when peeking.

Link: https://lore.kernel.org/all/20261019-tls-follow-on-v6-0-ab01c4ecca0c@kernel.org/

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 23eb94d9da6ee..65e578c8c6124 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -1582,6 +1584,19 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
>  	rxm->full_len -= prot->overhead_size;
>  	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
>  
> +	/* A peer may send empty data records, e.g. as cover traffic, but
> +	 * a burst of them traps the reader in the kernel. Treat more than
> +	 * 128 in a row, without the reader sleeping or returning to the
> +	 * caller in between, as a protocol violation. Stop the strparser
> +	 * before the next record gets parsed, so the reader finds the
> +	 * error in tls_rx_rec_wait().
> +	 */
> +	if (rxm->full_len ||
> +	    tls_msg(darg->skb)->control != TLS_RECORD_TYPE_DATA)
> +		ctx->empty_run = 0;

[Severity: Medium]
Can a peer get around the cap by mixing in non-empty records? Any record
with a non-zero rxm->full_len resets ctx->empty_run. So a peer that sends
repeated groups like this never triggers the abort:

  [128 empty data records][1 one-byte data record]

With that pattern, each byte delivered costs the reader 129 decryptions,
but the loop bounds only advance by one byte per group. In
tls_sw_recvmsg() the loop is:

  while (len && (decrypted + copied < target || tls_strp_msg_ready(ctx))) {

The sk_flush_backlog() calls in tls_read_flush_backlog() and
tls_rx_rec_wait() keep feeding in new records, and signals are only
checked when the reader sleeps. One recvmsg() with a 1 MiB buffer could
process around 135M records. With plain one-byte records it would be
about 1M.

tls_sw_read_sock() loops on desc->count. svc_tcp_recvfrom() starts with:

  .count		= serv->sv_max_mesg + sizeof(rpc_fraghdr),

and only lowers it by the bytes consumed. That gives an NFS-over-TLS
client the same ~129x amplification while the socket lock is held.
nvme-tcp uses rd_desc.count = 1 and never lowers it, so for nvme-tcp the
interleaved stream looks as unbounded as before.

The commit message says empty records "advance neither loop bound" and
keep the reader "in the kernel for as long as they arrive". With
interleaving, empty records are still 128/129 of the work, and the bounds
barely advance.

Would a per-call budget (empty records per reader call, or per byte
delivered), or a signal/need_resched check, be needed to close this?

> +	else if (unlikely(++ctx->empty_run > 128))
> +		tls_strp_abort_strp(&ctx->strp, -EBADMSG);

[Severity: Low]
Should Documentation/networking/tls.rst be updated for this new error
case? It currently says only:

  EBADMSG is returned if decryption failed for any other reason.

After this change, a record that decrypts and authenticates fine can also
cause EBADMSG. On the 129th consecutive empty data record,
tls_rx_one_record() calls tls_strp_abort_strp(). That sets sk->sk_err and
stops the strparser for good. Every later recvmsg(), splice() or
read_sock caller then gets EBADMSG from tls_rx_rec_wait().

Reusing EBADMSG matches existing practice, since tls_rx_msg_size()
already returns it when data_len < cipher_overhead. However, the
128-record limit and the fact that it breaks the connection permanently
are not documented anywhere that userspace TLS libraries would find them.

> +
>  	return tls_check_pending_rekey(sk, tls_ctx, darg->skb);
>  }

[ ... ]

> @@ -1939,7 +1960,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  			if (err < 0)
>  				goto put_on_rx_list_err;
>  
> -			if (is_peek) {
> +			if (is_peek && chunk) {
>  				peeked += chunk;
>  				goto put_on_rx_list;
>  			}

[Severity: Low]
Does this also consume empty non-data records under MSG_PEEK? The chunk
check applies to every record type, not just TLS_RECORD_TYPE_DATA.

Take an empty alert or handshake record that is read with MSG_PEEK.
full_len is 0 after decryption, which can happen with TLS 1.2 when
data_len == cipher_overhead, or with a TLS 1.3 inner type on an empty
body. Then:

  chunk is 0, so "async && chunk" is false
  skb_copy_datagram_msg() copies 0 bytes
  "is_peek && chunk" is false
  partially_consumed is false

The code then falls through to consume_skb(skb) and frees the record.

Before this patch, a peek always queued the skb on rx_list. The next
non-peek recvmsg() returned it from process_rx_list() along with its
TLS_GET_RECORD_TYPE cmsg. Now the next real read skips straight to the
following record.

The pile-up described in the commit message doesn't seem to apply to
non-data records. If one is already on rx_list, process_rx_list() sets
control != TLS_RECORD_TYPE_DATA and tls_sw_recvmsg() jumps to end, so
repeated peeks can't stack them up. The counter in tls_rx_one_record()
also only counts data records.

Would limiting the new condition to TLS_RECORD_TYPE_DATA records keep
the old peek behaviour for control records?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111185522.2859004-1-kuba%40kernel.org