Re: [PATCH v2] bpf, sockmap: Fix cork ownership in tcp_bpf_sendmsg()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unconditionally freeing the stack-allocated temporary message (`&tmp`) on the common exit path causes a Use-After-Free of corked pages.
- [High] Setting `*copied = 0` on failed redirects of a corked message wipes out the success count of independent messages sent earlier in the same syscall.
--

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

bpf, sockmap: Fix cork ownership in tcp_bpf_sendmsg()

tcp_bpf_sendmsg() keeps msg_tx across sk_stream_wait_memory(), which drops
and reacquires the socket lock. This commit makes temporary ownership explicit
by freeing only &tmp at the common exit, resets cork_bytes when allocating
the persistent cork fails, propagates iterator errors, and clears the copied
count when corked data is discarded.

Link: https://lore.kernel.org/netdev/20260719161630.2901208-1-nicoyip.dev@gmail.com/ [v1]

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6d..ce06bc1cff2b6 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c

[ ... ]

> @@ -480,14 +482,15 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  		if (unlikely(ret < 0)) {
>  			int free = sk_msg_free(sk, msg);
>  
> -			if (!cork)
> +			if (cork)
> +				*copied = 0;
> +			else
>  				*copied -= free;

[Severity: High]
Does setting *copied to 0 here erase the accumulated byte count of any
independent messages that were successfully transmitted earlier in the same
sendmsg syscall?

In tcp_bpf_sendmsg(), the copied variable tracks the total bytes successfully
sent across multiple iterations of the while loop. Because copied is zeroed
when a corked message fails redirect, tcp_bpf_sendmsg() returns the error
code instead of the number of bytes successfully sent prior to the failure.

Will this cause userspace to assume the entire buffer failed to send and retry
sending the first parts, leading to silent data duplication on the network?

>  		}
>  		if (cork) {
>  			sk_msg_free(sk, msg);

[ ... ]

> @@ -588,13 +592,12 @@ static int tcp_bpf_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
>  wait_for_memory:
>  		err = sk_stream_wait_memory(sk, &timeo);
> -		if (err) {
> -			if (msg_tx && msg_tx != psock->cork)
> -				sk_msg_free(sk, msg_tx);
> +		if (err)
>  			goto out_err;
> -		}
>  	}
>  out_err:
> +	if (msg_tx == &tmp)
> +		sk_msg_free(sk, msg_tx);

[Severity: Critical]
Does unconditionally freeing the stack-allocated temporary message (&tmp) here
cause a use-after-free of corked pages?

When a new cork is initiated on the final iteration of tcp_bpf_sendmsg(),
msg_tx points to the local stack variable tmp. tcp_bpf_send_verdict()
decides to cork it, allocates psock->cork, and performs a shallow copy of
tmp via memcpy(). It returns 0 without clearing tmp.sg.size or tmp.sg.data.

When the while loop finishes, execution falls through to out_err. Because
msg_tx is still &tmp and its state was never cleared after the shallow copy,
sk_msg_free() iterates over the scatterlist and frees all elements.

Doesn't this leave psock->cork holding dangling pointers to these freed pages,
meaning a subsequent send or socket close will trigger a use-after-free?

>  	if (err < 0)
>  		err = sk_stream_error(sk, msg->msg_flags, err);

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