Re: [PATCH bpf v2 03/15] bpf, sockmap: Don't let an SK_PASS verdict cover the next message

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] Use-after-free of psock->cork.
- [High] The fix is incomplete when the verdict has a byte budget
  (psock->apply_bytes != 0).
- [Medium] The patch resets eval early, which lets one sender's bytes be
  charged against another sender's verdict and end that verdict early.

--

bpf, sockmap: Don't let an SK_PASS verdict cover the next message

This commit changes the __SK_PASS branch of tcp_bpf_send_verdict(). When
psock->apply_bytes is 0, it now resets psock->eval to __SK_NONE before
tcp_bpf_push(). When the push fails, it subtracts the bytes that went out
from apply_bytes and ends the verdict if apply_bytes reaches 0.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index d90bcc911b36b..b016aa419fb2d 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -457,8 +457,22 @@ static int tcp_bpf_send_verdict(struct sock *sk, struct sk_psock *psock,
>  
>  	switch (psock->eval) {
>  	case __SK_PASS:
> +		/* The push may sleep without the socket lock, and it may fail */
> +		if (!psock->apply_bytes)
> +			psock->eval = __SK_NONE;

[Severity: High]

When psock->apply_bytes is not 0, eval stays __SK_PASS during the push.
The budget is still debited only after tcp_bpf_push() returns. Can a
second sender on the same socket still get past the prog here?

This call chain releases the socket lock:

tcp_bpf_push()->tcp_sendmsg_locked()->sk_stream_wait_memory()->sk_wait_event()

While sender A sleeps there, sender B can enter tcp_bpf_send_verdict(). B
sees eval == __SK_PASS, so the prog doesn't run, and B computes its tosend
from the budget before A's bytes are taken out:

	tosend = msg->sg.size;
	if (psock->apply_bytes && psock->apply_bytes < tosend)
		tosend = psock->apply_bytes;

A's bytes are subtracted only later, by sk_msg_apply_bytes(psock, tosend)
on success or by the new sk_msg_apply_bytes(psock, origsize -
msg->sg.size) on failure.

If both pushes succeed, a verdict that bpf_msg_apply_bytes() limited to N
bytes covers up to 2N bytes. If A's message alone uses up the budget, B's
message is the next message, and it passes without running the prog.

The commit message says:

    When apply_bytes is not 0, eval stays __SK_PASS during the push, and
    the prog does not run for another message, as today. The verdict must
    still cover the bytes that a failed push does not send. But such a
    message also lowers apply_bytes.

But B's message is sized against the budget before A's in-flight bytes
are removed from it. Would it be better to reserve the budget before the
push? That means debiting tosend, and ending the verdict when the budget
reaches 0, before calling tcp_bpf_push(), instead of subtracting after a
failure.

The __SK_PASS branch also looks unchanged at the end of the series.

> +		origsize = msg->sg.size;
>  		ret = tcp_bpf_push(sk, msg, tosend, flags, true);

[Severity: Critical]

Can another sender free msg while this push sleeps?

msg can be psock->cork here. tcp_bpf_sendmsg() picks it up with:

		if (psock->cork) {
			msg_tx = psock->cork;

The __SK_REDIRECT branch detaches psock->cork before release_sock(). The
__SK_PASS branch leaves it attached during tcp_bpf_push(). tcp_bpf_push()
keeps sge and page pointing into the cork's sg ring across
tcp_sendmsg_locked(), and that call can sleep in sk_stream_wait_memory()
without the socket lock.

With this patch and apply_bytes == 0 (the default), eval is __SK_NONE
during that sleep. Sender B can take the lock, reuse the same psock->cork,
append to it, and run the prog on A's partly sent cork. If the prog
returns SK_REDIRECT, B's __SK_REDIRECT branch does:

		if (psock->cork) {
			cork = true;
			psock->cork = NULL;
		}
		...
		if (cork) {
			sk_msg_free(sk, msg);
			kfree(msg);

When A resumes, tcp_bpf_push() writes into the freed struct sk_msg:

		msg->sg.size -= ret;
		sge->offset += ret;
		sge->length -= ret;

It may also retry with, or call put_page() on, a page that was already
released. If the push failed, the new failure path reads msg->sg.size and
calls sk_msg_free(sk, msg) on the freed object.

If B's prog returns SK_DROP instead, sk_msg_free() in the __SK_DROP branch
drops page references that A is still using.

The shared-cork problem with two PASS senders already existed. The kfree()
variant used to need a third sender to end a byte-limited verdict. The
early reset makes it reachable with only two senders.

The later commit "bpf, sockmap: Check that the redirected message is the
cork" also says that "a cork is still not safe for two senders on one
socket". The __SK_PASS branch is unchanged at the end of the series.

Is it safe to reset eval here while psock->cork is still attached and
being pushed?

>  		if (unlikely(ret)) {
> +			/* A part of it may have gone out */
> +			sk_msg_apply_bytes(psock, origsize - msg->sg.size);
> +			/* Another sender can use up the rest during the push */
> +			if (!psock->apply_bytes) {
> +				psock->eval = __SK_NONE;
> +				if (psock->sk_redir) {
> +					sock_put(psock->sk_redir);
> +					psock->sk_redir = NULL;
> +				}
> +			}

[Severity: Medium]

With the early reset, can sender A's bytes be charged against a verdict
that another sender installed?

When apply_bytes is 0 on entry, A's tosend is its whole message. eval is
now __SK_NONE while A sleeps in tcp_sendmsg_locked(). Sender B can take
the socket lock and run the prog, which installs B's verdict:

net/core/skmsg.c:sk_psock_msg_verdict() {
    ...
	psock->apply_bytes = msg->apply_bytes;
	if (ret == __SK_REDIRECT) {
    ...
		psock->sk_redir = msg->sk_redir;
		sock_hold(psock->sk_redir);
    ...
}

If B's apply_bytes is larger than B's message, B's verdict (PASS, DROP or
REDIRECT) stays active with some budget left. When A resumes, it
subtracts its bytes from B's budget, even though A's own verdict had no
byte limit. On success that happens through the unchanged
sk_msg_apply_bytes(psock, tosend). On failure it happens through the new
call above.

If that brings apply_bytes to 0, either this new block or the common block
at the end of tcp_bpf_send_verdict() sets eval to __SK_NONE and drops
psock->sk_redir:

	if (likely(!ret)) {
		if (!psock->apply_bytes) {
			psock->eval =  __SK_NONE;
			if (psock->sk_redir) {
				sock_put(psock->sk_redir);
				psock->sk_redir = NULL;
			}
		}

That ends B's verdict before it has covered the bytes B's prog asked for.
Later bytes that should have been dropped or redirected get a fresh prog
run instead.

Before this patch, eval stayed __SK_PASS during A's push, so this needed a
third sender. Could A record locally whether its verdict had a byte limit
on entry? Then it would touch psock->apply_bytes, eval and sk_redir only
when that is true and the verdict is still its own.

>  			*copied -= sk_msg_free(sk, msg);
>  			break;
>  		}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102072112.4160195-1-alexei.starovoitov%40gmail.com