Re: [PATCH net 6/7] net: tls: fix use-after-free in tls_sw_sendmsg_locked after bpf verdict

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Arithmetic underflow, state corruption, and silent data duplication in the iterator rollback path.
- [High] Use-After-Free when `bpf_exec_tx_verdict` returns `-ENOMEM`.
--

commit ba7ab075901b934710734a96c18f344dbc28cf04
Author: Jakub Kicinski <kuba@kernel.org>

net: tls: fix use-after-free in tls_sw_sendmsg_locked after bpf verdict

This patch fixes a use-after-free in tls_sw_sendmsg_locked() when
bpf_exec_tx_verdict() returns -ENOSPC. It updates msg_pl and msg_en after a
possible record split to avoid referencing a freed record.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 2c6a3c4a9fcc..4f8d656f39a3 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1157,6 +1157,13 @@ static int tls_sw_sendmsg_locked(struct sock *sk, struct msghdr *msg,
>  				else if (ret == -ENOMEM)
>  					goto wait_for_memory;

If bpf_exec_tx_verdict() splits the record, pushes it to tx_list, and then
encounters an -ENOMEM error while transmitting, it returns -ENOMEM.

This jumps to wait_for_memory without refetching msg_en. If
sk_stream_wait_memory() returns 0, the loop continues and evaluates:

	if (ctx->open_rec && msg_en->sg.size < required_size)
		goto alloc_encrypted;

Could this result in a use-after-free read when dereferencing msg_en->sg.size,
since msg_en still points to the freed original record?

>  				else if (ctx->open_rec && ret == -ENOSPC) {
> +					/* bpf_exec_tx_verdict() may have
> +					 * called tls_split_open_record(),
> +					 * freeing the old record. Re-fetch.
> +					 */
> +					rec = ctx->open_rec;
> +					msg_pl = &rec->msg_plaintext;
> +					msg_en = &rec->msg_encrypted;
>  					if (msg_pl->cork_bytes) {
>  						ret = 0;
>  						goto send_end;

If bpf_exec_tx_verdict() splits the record and then returns -ENOSPC (for
example, from a failed sk_msg_alloc on the remainder), the new msg_pl points
to the remainder record. If msg_pl->cork_bytes is 0, execution falls through
to rollback_iter:

rollback_iter:
	copied -= try_to_copy;
	sk_msg_sg_copy_clear(msg_pl, first);
	iov_iter_revert(&msg->msg_iter,
			msg_pl->sg.size - orig_size);

Since msg_pl is now the remainder record, its size might be smaller than
orig_size. Does the subtraction in iov_iter_revert() underflow and corrupt
the iterator state?

Also, sk_msg_sg_copy_clear(msg_pl, first) is called, but 'first' was captured
from the original record. Could this cause an out-of-bounds access on the
remainder's scatterlist?

Additionally, is it safe to roll back the iterator after the first chunk of
the split record was successfully transmitted? It seems this could lead to
the application re-sending already committed data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260429222944.2139041-1-kuba@kernel.org?part=6