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

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

On 7/3/26 7:05 AM, sashiko-bot@kernel.org wrote:
> 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?

It's true. Please also include a selftest to reproduce the issue and, 
another one to cover the non-full socket.


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