Re: [PATCH net 1/2] tls: don't over-fill the plaintext sk_msg ring in tls_sw_sendmsg_splice()

[Sabrina Dubroca <sd@queasysnail.net>]

2026-07-26, 19:55:55 +0900, chanyoung wrote:
> tls_sw_sendmsg_splice() appends pages to the open record's plaintext
> sk_msg ring with sk_msg_page_add(), which performs no fullness check of
> its own, and the loop only tests sk_msg_full() at the bottom of its
> do-while.
> 
> If the ring is already full when the function is entered, the first

This should never happen. We need to fix whatever path leads to that
invalid condition.

As you write in the cover letter:

    The ring is left full and unpushed across a syscall by the copy path,
    which does not set full_record when the fragment it adds is the one that
    exactly fills the ring.

That's what we should fix.

Which I think would be:


diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index d4afc90fd796..d2e399be8ef6 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -832,6 +832,14 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
 		if (!sk_stream_memory_free(sk))
 			goto wait_for_sndbuf;
 
+		/* open record may be full if we couldn't push it in the last sendmsg call */
+		if (sk_msg_full(msg_pl)) {
+			full_record = true;
+			sk_msg_trim(sk, msg_en,
+				    msg_pl->sg.size + prot->overhead_size);
+			goto copied;
+		}
+
 alloc_encrypted:
 		ret = tls_alloc_encrypted_msg(sk, required_size);
 		if (ret) {
@@ -921,6 +929,12 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
 						       msg_pl, try_to_copy);
 			if (ret < 0)
 				goto trim_sgl;
+
+			if (sk_msg_full(msg_pl)) {
+				full_record = true;
+				sk_msg_trim(sk, msg_en,
+					    msg_pl->sg.size + prot->overhead_size);
+			}
 		}
 
 		/* Open records defined only if successfully copied, otherwise



> sk_msg_page_add() writes the reserved slot and sk_msg_iter_next() wraps
> sg.end around to sg.start.  sk_msg_iter_dist() then returns 0, so
> sk_msg_full() reports the ring as empty, the loop keeps running, and each
> further add overwrites a live entry without putting its page reference
> while sg.size keeps growing.  sg.size is then larger than the data
> reachable by walking the logical [sg.start, sg.end) ring.
> 
> tls_push_record() marks the end of the scatterlist at the logical last
> entry but passes the inflated msg_pl->sg.size to tls_do_encryption() as
> cryptlen, so the AEAD scatterwalk runs past the end-marked entry and
> dereferences the NULL returned by sg_next():

TBH that also seems a bit dumb on the scatterwalk/crypto side. Users
of the crypto library shouldn't pass data with inconsistent sg and
data size, but I don't think this should crash the kernel.

> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000008
>   CPU: 1 UID: 1000 PID: 204 Comm: exploit Not tainted 7.2.0-rc4+ #1 PREEMPTLAZY
>   RIP: 0010:memcpy_from_scatterwalk+0x32/0xc0
>   Call Trace:
>    <TASK>
>    skcipher_walk_next+0x1d1/0x2c0
>    gcm_encrypt_aesni_avx+0x1e9/0x220
>    bpf_exec_tx_verdict+0x3bb/0x860
>    tls_sw_sendmsg+0xa1a/0xca0
>    __sys_sendto+0x1da/0x1f0
>    do_syscall_64+0xdc/0x520
>    entry_SYSCALL_64_after_hwframe+0x76/0x7e
>    </TASK>
> 
> An unprivileged user can reach this on a plain loopback TCP socket with
> the "tls" ULP attached.  A full but unpushed plaintext ring survives
> across a syscall through the copy path: sk_msg_clone() returns 0 rather
> than -ENOSPC for the frag that makes the ring exactly full, because its
> guard is "if (i == src->sg.end && len)" and len reaches 0 as that frag is
> added, so full_record is never set and MSG_MORE keeps eor clear.  Since
> record_room is a byte count, a frag-exhausted ring that holds only a few
> hundred bytes still admits the next splice(), which then re-enters
> tls_sw_sendmsg_splice() on a full ring.
> 
> The caller already handles a ring that becomes full during the splice by
> testing sk_msg_full() afterwards and setting full_record to push the
> record, so the loop condition only needs to be evaluated before the first
> sk_msg_page_add() rather than after it.  Turn the do-while into a while
> loop: when the ring is full on entry the function returns without adding
> anything, the caller pushes the record, and the next iteration of the
> caller's loop starts from a fresh, empty ring.

Please make your LLM (much) less verbose.

-- 
Sabrina