Re: [PATCH RFC v2 4/4] bpf: Mark bpf_iter__sockmap::sk as PTR_TRUSTED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: unlocked socket destroy via bpf_sock_destroy() in iter/sockmap
--

commit 97a776f78529c30ceec62254569454f51205e10f
Author: Michal Luczaj <mhal@rbox.co>

bpf: Mark bpf_iter__sockmap::sk as PTR_TRUSTED

This commit marks the sk field in struct bpf_iter__sockmap as PTR_TRUSTED so
BPF helpers and kfuncs can accept the socket pointer provided by iter/sockmap
programs.

Link: https://msgid.link/DM92JCFA4X4U.30MTDH0KCPTJ@gmail.com

> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 057e2f3c0265e..2a2a073728189 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -1989,7 +1989,7 @@ static struct bpf_iter_reg sock_map_iter_reg = {
>  		{ offsetof(struct bpf_iter__sockmap, key),
>  		  PTR_TO_BUF | PTR_MAYBE_NULL | MEM_RDONLY },
>  		{ offsetof(struct bpf_iter__sockmap, sk),
> -		  PTR_TO_BTF_ID_OR_NULL },
> +		  PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },

[Severity: Critical]
Does marking bpf_iter__sockmap::sk with PTR_TRUSTED allow iter/sockmap
programs to call bpf_sock_destroy() on an unlocked socket?

In net/core/filter.c, tracing_iter_filter() permits bpf_sock_destroy() for
any BPF_TRACE_ITER program as long as the caller supplies a trusted
struct sock_common pointer:

net/core/filter.c:tracing_iter_filter() {
	if (btf_id_set8_contains(&bpf_sk_iter_kfunc_ids, kfunc_id) &&
	    prog->expected_attach_type != BPF_TRACE_ITER)
		return -EACCES;
	return 0;
}

With ctx->sk marked as PTR_TRUSTED in sock_map_iter_reg, a NULL check on
ctx->sk yields a trusted struct sock pointer, which matches struct sock_common
at offset zero and is accepted by the verifier for bpf_sock_destroy().

However, unlike bpf_iter_tcp_seq_show() and bpf_iter_udp_seq_show() which
acquire lock_sock(sk) before running the BPF program, sock_map_seq_show() and
sock_hash_seq_show() in net/core/sock_map.c only hold rcu_read_lock():

sock_map_seq_show() / sock_hash_seq_show()
  -> bpf_iter_run_prog()
       -> bpf_sock_destroy()
            -> sk->sk_prot->diag_destroy(sk, ECONNABORTED)
                 -> tcp_abort() / udp_abort()

Because bpf_iter_run_prog() sets current->bpf_ctx, has_current_bpf_ctx()
returns true inside tcp_abort() and udp_abort(), causing both functions to
skip lock_sock(sk) under the assumption that the BPF context already holds the
socket lock:

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

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

For UDP sockets in a sockmap or sockhash, udp_abort() will run
__udp_disconnect() and sk->sk_prot->rehash(sk) with no socket lock held at
all.

For TCP_LISTEN sockets in a sockmap or sockhash, tcp_abort() will execute
tcp_set_state(sk, TCP_CLOSE) -> inet_put_port(sk) and inet_csk_listen_stop(sk)
before bh_lock_sock(sk) without holding lock_sock(sk), racing with concurrent
close() or abort paths.

For established TCP sockets, tcp_abort() will only take bh_lock_sock(sk)
without checking sock_owned_by_user(sk), running tcp_write_queue_purge(sk) and
tcp_done(sk) concurrently with process-context holders of lock_sock(sk). If
this races with __tcp_close() after sock_orphan(sk) sets SOCK_DEAD, both
tcp_done() and __tcp_close() can call inet_csk_destroy_sock(sk), leading to a
double sock_put(sk) and use-after-free.

Should sock_map_seq_show() and sock_hash_seq_show() acquire lock_sock(sk), or
should bpf_sock_destroy() be restricted from running in iter/sockmap?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-sockmap-update-limit-iter-v2-0-e96a63c43b39@rbox.co?part=4