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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory leak of stack-allocated `tmp` scatterlist pages when aborting via `goto out_err`.
- [High] Lost error code from `sk_msg_memcopy_from_iter` causes sendmsg to incorrectly return 0 instead of propagating -EFAULT.
- [High] Silent data loss on `__SK_REDIRECT` failure for corked messages due to masked error.
- [Medium] Overlapping `memcpy()` when corking an already-corked message.
--

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

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 commit fixes a use-after-free where a
stale cork could be mistaken for the local temporary message and freed again.
It tests for tmp directly instead of comparing with psock->cork.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6d..b9f0366333653 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -589,7 +589,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);

[Severity: High]
This isn't a bug introduced by this patch, but can this leak page references
or memory for the stack-allocated tmp?

If kzalloc_obj() for psock->cork fails in tcp_bpf_send_verdict(), returning
-ENOMEM but leaving psock->cork_bytes > 0, and userspace retries sendmsg(),
msg_tx defaults to the stack-allocated &tmp.

Data is copied into tmp, allocating pages. The code then hits:
if (psock->cork_bytes && !enospc)
    goto out_err;

And the out_err block returns without freeing tmp. Unprivileged userspace
could repeatedly trigger this state after a single induced -ENOMEM, leaking
pages on subsequent small sendmsg() calls.

>  			goto out_err;

[Severity: High]
This is a pre-existing issue, but does this goto out_err drop the
error code from sk_msg_memcopy_from_iter()?

Earlier in tcp_bpf_sendmsg():
ret = sk_msg_memcopy_from_iter(sk, &msg->msg_iter, msg_tx, copy);
if (ret < 0) {
    sk_msg_trim(sk, msg_tx, osize);
    goto out_err;
}

The out_err block returns err (which is initialized to 0), rather than ret.
If a local user passes a bad pointer, it will get a 0 return value instead
of -EFAULT, which usually causes userspace blocking socket writes to spin
endlessly.

>  		}

[Severity: Medium]
This isn't a bug introduced by this patch, but does tcp_bpf_send_verdict()
trigger an overlapping memory copy?

If a BPF program evaluates an already-corked message and assigns a new
cork_bytes threshold that exceeds the current size, tcp_bpf_send_verdict()
executes:

memcpy(psock->cork, msg, sizeof(*msg));

If msg is identical to psock->cork, this results in an overlapping memory
copy onto itself. This can violate the C standard and trigger UBSAN panics.

[Severity: High]
This is a pre-existing issue, but does tcp_bpf_send_verdict() silently
drop data on __SK_REDIRECT failure for corked messages?

If a redirected message is corked and tcp_bpf_sendmsg_redir() fails, the
code frees the message:

if (cork) {
    sk_msg_free(sk, msg);
    kfree(msg);
    msg = NULL;
    ret = 0;
}

This intentionally skips subtracting the freed bytes from *copied and
overwrites ret = 0. This causes tcp_bpf_sendmsg() to assume success,
continue processing the user's buffer, and return a positive byte count
to userspace despite dropping the corked data.

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