Re: [PATCH bpf v4] bpf, sockmap: Fix cork use-after-free in tcp_bpf_sendmsg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent sends on the same socket with an active redirect verdict cause a Use-After-Free on the target redirect socket (`sk_redir`).
- [High] `memcpy` is called with identical source and destination pointers when appending to an existing cork.
- [High] A failed allocation of `psock->cork` leaves `psock->cork_bytes` greater than zero, leading to a memory leak of the stack-allocated `tmp` message and silent data loss on subsequent `sendmsg()` calls.
--

--- Patch [1]: [PATCH bpf v4] bpf, sockmap: Fix cork use-after-free in tcp_bpf_sendmsg() ---
commit df6d10c569d9a8f6bacb9c15795f057eb688c1ef
Author: Chengfeng Ye <nicoyip.dev@gmail.com>
Subject: bpf, sockmap: Fix cork use-after-free in tcp_bpf_sendmsg()

tcp_bpf_sendmsg() keeps msg_tx across sk_stream_wait_memory(), which drops
and reacquires the socket lock. This patch fixes a use-after-free caused
by an unsafe comparison between msg_tx and psock->cork, resolving it by
checking directly against the stack-local tmp message.

Link: https://lore.kernel.org/bpf/87fr18lmzo.fsf%40cloudflare.com/
Link: https://lore.kernel.org/netdev/20260719161630.2901208-1-nicoyip.dev%40gmail.com/

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 8e905b50deadb..a30475afb6f89 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could this leak the tmp
message and cause silent data loss if an earlier cork allocation fails?

In tcp_bpf_send_verdict(), psock->cork_bytes is updated before allocating
psock->cork:

net/ipv4/tcp_bpf.c:tcp_bpf_send_verdict() {
    ...
	if (msg->cork_bytes &&
	    msg->cork_bytes > msg->sg.size && !enospc) {
		psock->cork_bytes = msg->cork_bytes - msg->sg.size;
		if (!psock->cork) {
			psock->cork = kzalloc_obj(*psock->cork, ...);
			if (!psock->cork) {
				sk_msg_free(sk, msg);
				*copied = 0;
				return -ENOMEM;
			}
    ...
}

If the allocation fails, psock->cork_bytes remains greater than zero.

Then, on a subsequent sendmsg(), tcp_bpf_sendmsg() populates the stack-local
tmp message, sees the non-zero psock->cork_bytes, and jumps directly to
out_err:

net/ipv4/tcp_bpf.c:tcp_bpf_sendmsg() {
    ...
		if (psock->cork_bytes) {
			if (size > psock->cork_bytes)
				psock->cork_bytes = 0;
			else
				psock->cork_bytes -= size;
			if (psock->cork_bytes && !enospc)
				goto out_err;
    ...
}

Because it bypasses the error path cleanup below, doesn't this permanently
leak the tmp message's page fragments and silently drop the data?


[Severity: High]
This is a pre-existing issue, but does appending to a cork execute undefined
behavior by calling memcpy() with identical pointers?

In tcp_bpf_sendmsg(), if psock->cork is already allocated, msg_tx is set to
point to it and passed to tcp_bpf_send_verdict() as the msg argument. If the
BPF program requests further corking, the code does this:

net/ipv4/tcp_bpf.c:tcp_bpf_send_verdict() {
    ...
		if (!psock->cork) {
			psock->cork = kzalloc_obj(*psock->cork, ...);
            ...
		}
		memcpy(psock->cork, msg, sizeof(*msg));
		return 0;
    ...
}

Since msg is exactly psock->cork here, does this result in a memcpy() with
identical source and destination pointers, potentially causing a kernel
panic with FORTIFY_SOURCE or KASAN?


[Severity: High]
This is a pre-existing issue, but is there a use-after-free on sk_redir
when multiple threads send on the same socket concurrently?

When a BPF program returns __SK_REDIRECT with apply_bytes > 0,
sk_psock_msg_verdict() takes a single reference on the target socket via
sock_hold(psock->sk_redir).

If multiple threads process the remaining apply_bytes for this socket, they
all read psock->sk_redir and release the socket lock to call
tcp_bpf_sendmsg_redir():

net/ipv4/tcp_bpf.c:tcp_bpf_send_verdict() {
    ...
	case __SK_REDIRECT:
		redir_ingress = psock->redir_ingress;
		sk_redir = psock->sk_redir;
		sk_msg_apply_bytes(psock, tosend);
		if (!psock->apply_bytes) {
			/* Clean up before releasing the sock lock. */
			eval = psock->eval;
			psock->eval = __SK_NONE;
			psock->sk_redir = NULL;
		}
        ...
		release_sock(sk);

		origsize = msg->sg.size;
		ret = tcp_bpf_sendmsg_redir(sk_redir, redir_ingress, ...);
		sent = origsize - msg->sg.size;

		if (eval == __SK_REDIRECT)
			sock_put(sk_redir);
    ...
}

The thread that exhausts apply_bytes clears psock->sk_redir and calls
sock_put(sk_redir) to drop the single reference. Since it doesn't wait for
other concurrent threads to finish, could the target socket be freed while
they are still executing tcp_bpf_sendmsg_redir(sk_redir)?

[ ... ]

> @@ -604,7 +604,7 @@ static int tcp_bpf_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  wait_for_memory:
>  		err = sk_stream_wait_memory(sk, &timeo);
>  		if (err) {
> -			if (msg_tx && msg_tx != psock->cork)
> +			if (msg_tx == &tmp)
>  				sk_msg_free(sk, msg_tx);
>  			goto out_err;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724103856.3399001-1-nicoyip.dev@gmail.com?part=1