Re: [PATCH bpf] bpf: reject mini-sockets in bpf_sock_destroy()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch causes a functional regression by completely disabling bpf_sock_destroy() for TCP mini-sockets (TIME_WAIT and NEW_SYN_RECV).

Pre-existing issues:
- [High] The helpers bpf_tcp_check_syncookie and bpf_tcp_gen_syncookie suffer from the same type confusion and out-of-bounds read as bpf_sock_destroy, but remain unfixed.
--

commit cf46da8728e7f1fca4584ccdb089c82b7fd3061f
Author: Xiang Mei (Microsoft) <xmei5@asu.edu>

bpf: reject mini-sockets in bpf_sock_destroy()

The patch tries to fix a KASAN slab-out-of-bounds read in
bpf_sock_destroy() by checking sk_fullsock(sk) before accessing
sk->sk_protocol. It casts sock_common to sock and early exits if it is
a mini-socket.

Fixes: 4ddbcb886268 ("bpf: Add bpf_sock_destroy kfunc")
Reported-by: AutonomousCodeSecurity@microsoft.com
Signed-off-by: Xiang Mei (Microsoft) <xmei5@asu.edu>

> diff --git a/net/core/filter.c b/net/core/filter.c
> index b446aa8be5c37..3e70c8ab5aab1 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12665,6 +12665,9 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
>  {
>  	struct sock *sk = (struct sock *)sock;
>  
> +	if (!sk_fullsock(sk))
> +		return -EOPNOTSUPP;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
Does this unconditionally reject mini-sockets?

The commit message states that mini-sockets have no diag_destroy() handler,
but as documented in the original commit 4ddbcb886268, request sockets are
explicitly supported because their protocol diag_destroy handler
(tcp_abort) correctly handles TCP_TIME_WAIT and TCP_NEW_SYN_RECV states.

If we unconditionally reject non-full sockets, wouldn't this break BPF
programs that rely on bpf_sock_destroy() to forcefully terminate pending
connections or TIME_WAIT sockets?

[Severity: High]
This is a pre-existing issue, but does this same type confusion exist
in bpf_tcp_check_syncookie() and bpf_tcp_gen_syncookie()?

Both of those helpers take ARG_PTR_TO_BTF_ID_SOCK_COMMON and cast it to
a full struct sock *. They then immediately dereference sk->sk_protocol
without an sk_fullsock() check:

net/core/filter.c:bpf_tcp_check_syncookie() {
    ...
	if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
		return -EINVAL;
    ...
}

If an XDP or TC BPF program passes a tw_sock or req_sock from
bpf_skc_lookup_tcp(), won't this result in the same KASAN
slab-out-of-bounds read since sk_protocol resides outside the bounds
of struct sock_common?

> +
>  	/* The locking semantics that allow for synchronous execution of the
>  	 * destroy handlers are only supported for TCP and UDP.
>  	 * Supporting protocols will need to acquire sock lock in the BPF context

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702224519.800135-1-xmei5@asu.edu?part=1