Re: [PATCH net 1/2] net: tls: preserve split open record on async encrypt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Error masking in bpf_exec_tx_verdict() leads to zero-copy Use-After-Free
- [Critical] Use-After-Free during __SK_REDIRECT socket lock drop
--

commit d94934a96df0cedb5ae4fee1bc9264e363a4efb9
Author: Christopher Lusk <clusk@northecho.dev>

net: tls: preserve split open record on async encrypt

When a BPF sk_msg verdict reduces the apply_bytes size below the open
record, the record is split. This patch ensures the remainder record
remains rooted when the initial encryption completes asynchronously.

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 2590e855f6a5a..5682849e102c7 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c

[ ... ]

> @@ -861,6 +869,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
>  	struct sock *sk_redir;
>  	struct tls_rec *rec;
>  	bool enospc, policy, redir_ingress;
> +	bool async = false;
>  	int err = 0, send;
>  	u32 delta = 0;

[ ... ]

> @@ -910,6 +919,10 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
>  	switch (psock->eval) {
>  	case __SK_PASS:
>  		err = tls_push_record(sk, flags, record_type);
> +		if (err == -EINPROGRESS) {
> +			async = true;
> +			err = 0;
> +		}
>  		if (err && err != -EINPROGRESS && sk->sk_err == EBADMSG) {
>  			*copied -= sk_msg_free(sk, msg);
>  			tls_free_open_rec(sk);

[ ... ]

> @@ -979,7 +992,7 @@ static int bpf_exec_tx_verdict(struct sk_msg *msg, struct sock *sk,
>  	}
>   out_err:
>  	sk_psock_put(sk, psock);
> -	return err;
> +	return err ?: (async ? -EINPROGRESS : 0);
>  }

If async is true due to -EINPROGRESS from a previous fragment, but a
subsequent fragment encounters an error (for example, if __SK_DROP returns
-EACCES, or if tls_push_record() returns -ENOMEM), does this return
statement mask the -EINPROGRESS status?

If the error code overrides -EINPROGRESS, the caller tls_sw_sendmsg() might
not increment its num_async counter. When the send ends, it could skip
calling tls_encrypt_async_wait(). If MSG_ZEROCOPY was used, could this
allow the function to return to userspace while the hardware crypto engine
is still reading the pages, leading to a use-after-free?

Additionally, while looking at this function, there appears to be a
pre-existing issue when handling __SK_REDIRECT:

net/tls/tls_sw.c:bpf_exec_tx_verdict() {
    ...
    case __SK_REDIRECT:
        ...
        release_sock(sk);
        err = tcp_bpf_sendmsg_redir(sk_redir, redir_ingress,
                                    &msg_redir, send, flags);
        lock_sock(sk);
        if (err < 0) {
            sk_mem_uncharge(sk, msg->sg.size);
    ...
}

The msg argument points to the plaintext buffer inside ctx->open_rec.
When the socket lock is dropped via release_sock(sk) before calling
tcp_bpf_sendmsg_redir(), could a concurrent thread acquire the lock and
transmit or free ctx->open_rec?

If that happens, when the original thread reacquires the lock and accesses
msg->sg.size, does this result in a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515151556.189841-1-clusk@northecho.dev?part=1