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

[bot+bpf-ci]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 2a84f9d01131..cae43b999162 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12913,8 +12913,9 @@ __bpf_kfunc_start_defs();
>   * @sock: Pointer to socket to be destroyed
>   *
>   * Return:
> - * On error, may return EPROTONOSUPPORT, EINVAL.
> - * EPROTONOSUPPORT if protocol specific destroy handler is not supported.
> + * On error, may return EOPNOTSUPP, or whatever the protocol specific
> + * destroy handler returns.
> + * EOPNOTSUPP if protocol specific destroy handler is not supported.
>   * 0 otherwise
>   */
>  __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
> @@ -12926,8 +12927,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);

This isn't a bug, but would it help to keep the TCP/UDP rationale comment
next to the protocol check, and add a short note that sk_protocol is only
valid on full socks? The block comment explaining the locking semantics
and TCP/UDP restriction now sits two statements away from the protocol
test it documents, and the newly added sk_fullsock() guard doesn't have
any nearby note explaining why it's there. The reason (sk_protocol is a
struct sock member not present in sock_common) is only in the changelog.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34471854292