Skip to contentSkip to conversation

tls: make sure to abort the stream if headers are bogus

PATCH Applied Jakub Kicinski 2 participants 3 messages Sep 17, 2025 – Sep 18, 2025

Subsystems (4)
Networking Network drivers Kselftest Networking TLS
lore
Share & tools
lore

Jakub Kicinski

author maintainer
#0 lore raw

PATCH 1/2 tls: make sure to abort the stream if headers are bogus

Normally we wait for the socket to buffer up the whole record before we service it. If the socket has a tiny buffer, however, we read out the data sooner, to prevent connection stalls. Make sure that we abort the connection when we find out late that the record is actually invalid. Retrying the parsing is fine in itself but since we copy some more data each time before we parse we can overflow the allocated skb space.

Constructing a scenario in which we're under pressure without enough data in the socket to parse the length upfront is quite hard. syzbot figured out a way to do this by serving us the header in small OOB sends, and then filling in the recvbuf with a large normal send.

Make sure that tls_rx_msg_size() aborts strp, if we reach an invalid record there's really no way to recover.

Reported-by Lee Jones <[email protected]>
Fixes 84c61fe1a75b ("tls: rx: do not use the standard strparser")
Reviewed-by Sabrina Dubroca <[email protected]>
Signed-off-by Jakub Kicinski <[email protected]>
---
 net/tls/tls.h      |  1 +
 net/tls/tls_strp.c | 14 +++++++++-----
 net/tls/tls_sw.c   |  3 +--
 3 files changed, 11 insertions(+), 7 deletions(-)
net/tls/tls.h +1 -0
index 4e077068e6d9..e4c42731ce39 100644
@@ -141,6 +141,7 @@ void update_sk_prot(struct sock *sk, struct tls_context *ctx);
 
 int wait_on_pending_writer(struct sock *sk, long *timeo);
 void tls_err_abort(struct sock *sk, int err);
+void tls_strp_abort_strp(struct tls_strparser *strp, int err);
 
 int init_prot_info(struct tls_prot_info *prot,
 		   const struct tls_crypto_info *crypto_info,
net/tls/tls_strp.c +9 -5
index d71643b494a1..98e12f0ff57e 100644
@@ -13,7 +13,7 @@
 
 static struct workqueue_struct *tls_strp_wq;
 
-static void tls_strp_abort_strp(struct tls_strparser *strp, int err)
+void tls_strp_abort_strp(struct tls_strparser *strp, int err)
 {
 	if (strp->stopped)
 		return;

@@ -211,11 +211,17 @@ static int tls_strp_copyin_frag(struct tls_strparser *strp, struct sk_buff *skb,
 				struct sk_buff *in_skb, unsigned int offset,
 				size_t in_len)
 {
+	unsigned int nfrag = skb->len / PAGE_SIZE;
 	size_t len, chunk;
 	skb_frag_t *frag;
 	int sz;
 
-	frag = &skb_shinfo(skb)->frags[skb->len / PAGE_SIZE];
+	if (unlikely(nfrag >= skb_shinfo(skb)->nr_frags)) {
+		DEBUG_NET_WARN_ON_ONCE(1);
+		return -EMSGSIZE;
+	}
+
+	frag = &skb_shinfo(skb)->frags[nfrag];
 
 	len = in_len;
 	/* First make sure we got the header */

@@ -520,10 +526,8 @@ static int tls_strp_read_sock(struct tls_strparser *strp)
 	tls_strp_load_anchor_with_queue(strp, inq);
 	if (!strp->stm.full_len) {
 		sz = tls_rx_msg_size(strp, strp->anchor);
-		if (sz < 0) {
-			tls_strp_abort_strp(strp, sz);
+		if (sz < 0)
 			return sz;
-		}
 
 		strp->stm.full_len = sz;
net/tls/tls_sw.c +1 -2
index bac65d0d4e3e..daac9fd4be7e 100644
@@ -2474,8 +2474,7 @@ int tls_rx_msg_size(struct tls_strparser *strp, struct sk_buff *skb)
 	return data_len + TLS_HEADER_SIZE;
 
 read_failure:
-	tls_err_abort(strp->sk, ret);
-
+	tls_strp_abort_strp(strp, ret);
 	return ret;
 }
Signature · 1 line
2.51.0
author maintainer
#1 lore raw

PATCH 2/2 selftests: tls: test skb copy under mem pressure and OOB

Add a test which triggers mem pressure via OOB writes.

Reviewed-by Sabrina Dubroca <[email protected]>
Signed-off-by Jakub Kicinski <[email protected]>
---
 tools/testing/selftests/net/tls.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
tools/testing/selftests/net/tls.c +16 -0
index cd67b0ae75a7..e788b84551ca 100644
@@ -2775,6 +2775,22 @@ TEST_F(tls_err, poll_partial_rec_async)
 	}
 }
 
+/* Use OOB+large send to trigger copy mode due to memory pressure.
+ * OOB causes a short read.
+ */
+TEST_F(tls_err, oob_pressure)
+{
+	char buf[1<<16];
+	int i;
+
+	memrnd(buf, sizeof(buf));
+
+	EXPECT_EQ(send(self->fd2, buf, 5, MSG_OOB), 5);
+	EXPECT_EQ(send(self->fd2, buf, sizeof(buf), 0), sizeof(buf));
+	for (i = 0; i < 64; i++)
+		EXPECT_EQ(send(self->fd2, buf, 5, MSG_OOB), 5);
+}
+
 TEST(non_established) {
 	struct tls12_crypto_info_aes_gcm_256 tls12;
 	struct sockaddr_in addr;
Signature · 1 line
2.51.0
CI Applied
#2 lore raw
In reply to Jakub Kicinski
This series was applied to netdev/net.git (main) by Paolo Abeni <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:

Quoted 9 lines · On Tue, 16 Sep 2025 17:28:13 -0700 you wrote:

Normally we wait for the socket to buffer up the whole record before we service it. If the socket has a tiny buffer, however, we read out the data sooner, to prevent connection stalls. Make sure that we abort the connection when we find out late that the record is actually invalid. Retrying the parsing is fine in itself but since we copy some more data each time before we parse we can overflow the allocated skb space.

[...]

Here is the summary with links:
- [net,1/2] tls: make sure to abort the stream if headers are bogus

    https://git.kernel.org/netdev/net/c/0aeb54ac4cd5
  - [net,2/2] selftests: tls: test skb copy under mem pressure and OOB
    https://git.kernel.org/netdev/net/c/4c05c7ed880f

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html