Re: [PATCH net 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: 3 · Low: 0

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] The empty-record cap triggers on any backlog of empty records,
  not only on a fast burst.
- [Medium] In tls_sw_recvmsg() the MSG_PEEK path now frees zero-length
  records of every content type, not only empty application_data records.
- [Medium] The new cap 'breaks the connection' through
  tls_strp_abort_strp(&ctx->strp, -EBADMSG).

--

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

This patch adds a per-read counter of consecutive empty application_data
records. When the counter passes 128, it stops the strparser with
-EBADMSG. It also makes recvmsg() decrypt empty records synchronously and
consume them, even with MSG_PEEK, instead of queuing them on rx_list.

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 771e02bff5199..adf3b26b8a75f 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;
> +	else if (unlikely(++ctx->empty_run > 128))
> +		tls_strp_abort_strp(&ctx->strp, -EBADMSG);

[Severity: Medium]
Can this cap also trip on a slow, legitimate sender when the local
application just stops reading for a while?

ctx->empty_run is reset in only two places. One is
tls_rx_reader_acquire(), at the start of each read call. The other is
tls_rx_rec_wait(), just before the reader sleeps:

net/tls/tls_sw.c:tls_rx_rec_wait() {
    ...
		if (!timeo)
			return -EAGAIN;

		ctx->empty_run = 0;
		released = true;
    ...
}

Suppose records pile up in sk_receive_queue while the application is
idle. The next recvmsg() finds every queued record already parsable, so
tls_rx_rec_wait() returns 1 without reaching that reset.
tls_rx_one_record() then counts every queued empty data record within
that single call.

For example, take a TLS 1.3 peer that sends padding-only cover traffic at
one record per second, which RFC 8446 allows. After about two minutes
without a read, more than 128 empty records would be queued. The next
read would get EBADMSG, and the strparser would stay stopped.

The commit message and the new comment describe this as a reaction to "a
burst" from a peer that is "either broken or malicious". Is the idle
reader backlog case meant to be covered as well? Also, no MIB counter is
incremented when this triggers.

[Severity: Medium]
Does this leave the socket in a state where only the first read learns
that the connection is broken?

tls_strp_abort_strp() sets strp->stopped, writes sk->sk_err once and
calls sk_error_report(). It does not set RCV_SHUTDOWN or SOCK_DONE, and
it records no persistent TLS error.

In the burst case nothing has been copied yet, so tls_rx_rec_wait() runs
with has_copied == false and returns sock_error(sk), which clears
sk_err:

	if (sk->sk_err) {
		if (has_copied)
			return -READ_ONCE(sk->sk_err);
		return sock_error(sk);
	}

After that, tls_strp_check_rcv() returns early because strp->stopped is
set, so msg_ready never becomes true again:

net/tls/tls_strp.c:tls_strp_check_rcv() {
	if (unlikely(strp->stopped) || strp->msg_ready)
		return;
    ...
}

A later blocking recvmsg() or splice() would then sleep in
sk_wait_event() on tls_strp_msg_ready(ctx). It would wake only on
SO_RCVTIMEO, a signal, or a FIN/RST from the peer, even if the peer keeps
sending data. A nonblocking reader would get -EAGAIN forever.

tls_sk_poll() also clears EPOLLIN, because rx_list is empty and
tls_strp_msg_ready() is false:

net/tls/tls_main.c:tls_sk_poll() {
    ...
	if ((skb_queue_empty_lockless(&ctx->rx_list) &&
	     !tls_strp_msg_ready(ctx)) ||
	    READ_ONCE(ctx->key_update_pending))
		mask &= ~(EPOLLIN | EPOLLRDNORM);
    ...
}

EPOLLERR also goes away once sk_err has been consumed. Could an
application that reads SO_ERROR on EPOLLERR before calling recv(), or
that retries after EBADMSG, end up hanging?

This seems to differ from the subject line and from this part of the
commit message:

  "The strparser is stopped before it parses the next record, so the
  reader gets the error from tls_rx_rec_wait() and no later record is
  delivered."

The existing read_failure abort in tls_rx_msg_size() has the same
weakness. This patch, though, adds a new trigger that legal records can
reach, and its stated behaviour depends on this mechanism. Does the abort
need a persistent error or EOF state, so that later reads and poll() see
the failure?

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

[ ... ]

> @@ -1918,7 +1936,10 @@ int tls_sw_recvmsg(struct sock *sk,
>  
>  			DEBUG_NET_WARN_ON_ONCE(darg.skb == ctx->strp.anchor);
>  
> -			if (async) {
> +			/* Consume empty records, even when peeking, so that
> +			 * they cannot pile up on rx_list.
> +			 */
> +			if (async && chunk) {
>  				/* TLS 1.2-only, to_decrypt must be text len */
>  				chunk = min_t(int, to_decrypt, len);
>  				async_copy_bytes += chunk;
> @@ -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: Medium]
Does this also consume zero-length records that are not application data
when MSG_PEEK is set?

tls_rx_msg_size() accepts a zero-length non-data record (alert,
handshake, or any other type). In TLS 1.2 that means data_len ==
cipher_overhead. In TLS 1.3 it means the plaintext is only the inner
content type byte.

Such a record can only be the first one in a recvmsg() call. So
darg.async and async are both false, and chunk == rxm->full_len == 0.

Both of the new checks are then false:

	if (async && chunk) {
	...
	if (is_peek && chunk) {

so execution reaches consume_skb(skb) even though the caller passed
MSG_PEEK.

Before this patch, the record went onto rx_list. The next non-peek
recvmsg() then returned it, with its TLS_GET_RECORD_TYPE cmsg, through
process_rx_list(). Now the peek returns the cmsg and frees the record,
and the next real read returns the following record instead.

Non-data records don't seem to be part of the pile-up problem.
tls_rx_one_record() resets empty_run for them, and the loop already
breaks after a non-data record:

		if (control != TLS_RECORD_TYPE_DATA)
			break;

Should the consume path be limited to data records? For example:

	if (is_peek && (chunk || control != TLS_RECORD_TYPE_DATA))

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