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

[Jiayuan Chen <jiayuan.chen@linux.dev>]

on 9/4/26 6:19 PM, sashiko-bot@kernel.org write:
> 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.


Hallucination


> - [High] Calling cond_resched() while holding rcu_read_lock() via bpf_sock_destroy() on TCP_LISTEN sockets.


It's True


> --
[...]
> [ ... ]
>> @@ -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?

No.


I try load "SEC("iter/sockmap")" but verifier rejected it.

Only the tcp/udp iters mark sk as PTR_TRUSTED and bpf_sock_destroy() 
needs a trusted arg.


> 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?


Yes, reproduced with a listener that still has an unaccepted child.

Let me think how to fix it...