Re: [PATCH bpf v2 1/2] bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] bpf_sock_destroy bypasses socket locking for iterators that do not pre-lock the socket, leading to data corruption and kernel panics.
- [High] Calling cond_resched() while holding rcu_read_lock() via bpf_sock_destroy() on TCP_LISTEN sockets.
--

commit 1a9df13e7482333dfb4a7ad04982dba6254ff808
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

bpf: Fix out-of-bounds read of sk_protocol in bpf_sock_destroy()

sk_protocol lives in struct sock, not in struct sock_common. A timewait
or request sock handed to bpf_sock_destroy() by the tcp iterator is
neither, so reading sk->sk_protocol runs past the object.

Closes: https://lore.kernel.org/bpf/20260702224519.800135-1-xmei5@asu.edu/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 61940e7535523..a41cc60a401a5 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> @@ -12925,8 +12926,12 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
>  	 * Supporting protocols will need to acquire sock lock in the BPF context
>  	 * prior to invoking this kfunc.
>  	 */
> -	if (!sk->sk_prot->diag_destroy || (sk->sk_protocol != IPPROTO_TCP &&
> -					   sk->sk_protocol != IPPROTO_UDP))
> +	if (!sk->sk_prot->diag_destroy)
> +		return -EOPNOTSUPP;
> +
> +	if (sk_fullsock(sk) &&
> +	    sk->sk_protocol != IPPROTO_TCP &&
> +	    sk->sk_protocol != IPPROTO_UDP)
>  		return -EOPNOTSUPP;
>  
>  	return sk->sk_prot->diag_destroy(sk, ECONNABORTED);

[Severity: High]
This is a pre-existing issue, but does this invoke the protocol's destroy
handler without proper locking?

The bpf_sock_destroy() kfunc can be called from iterators. It calls the
protocol's diag_destroy() handler:

net/core/filter.c:bpf_sock_destroy() {
    ...
    return sk->sk_prot->diag_destroy(sk, ECONNABORTED);
}

For TCP sockets, this resolves to tcp_abort(), which skips acquiring the
socket lock because has_current_bpf_ctx() is always true inside a BPF
program:

net/ipv4/tcp.c:tcp_abort() {
    ...
    if (!has_current_bpf_ctx())
        lock_sock(sk);
    ...
}

While some iterators pre-lock the socket, others (like sockmap iterators) do
not. Does this result in tcp_abort() running lockless and potentially
corrupting socket state?

Additionally, if a raw socket with IPPROTO_TCP is passed, raw_abort() is
invoked instead, which unconditionally calls lock_sock():

net/ipv4/raw.c:raw_abort() {
    ...
    lock_sock(sk);
    ...
}

Since iterators run under rcu_read_lock(), will this sleep in an atomic
context and trigger a BUG?

[Severity: High]
This is also a pre-existing issue, but can this call cond_resched() while
holding an RCU read lock?

When bpf_sock_destroy() processes a TCP socket in the TCP_LISTEN state,
it calls tcp_abort(), which then calls inet_csk_listen_stop():

net/ipv4/tcp.c:tcp_abort() {
    ...
    inet_csk_listen_stop(sk);
    ...
}

Inside inet_csk_listen_stop(), there is a loop that explicitly yields:

net/ipv4/inet_connection_sock.c:inet_csk_listen_stop() {
    ...
    cond_resched();
    ...
}

Since bpf_iter_run_prog() runs iterators under rcu_read_lock_dont_migrate(),
will calling cond_resched() from this atomic context trigger a "sleeping
function called from invalid context" BUG?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904094954.63447-1-jiayuan.chen@linux.dev?part=1