Re: [PATCH bpf v2 1/1] bpf: Check sk_state before sk_protocol in bpf_tcp_*_syncookie

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch completely misses an identical slab out-of-bounds read vulnerability in the `bpf_sock_destroy` kfunc when a mini-socket is passed.
--

--- Patch [1]: [PATCH bpf v2 1/1] bpf: Check sk_state before sk_protocol in bpf_tcp_*_syncookie ---
commit fe03c404200965b4022b50ca58915a717cbea479
Author: Luxiao Xu <rakukuip@gmail.com>

bpf: Check sk_state before sk_protocol in bpf_tcp_*_syncookie

This commit fixes a slab out-of-bounds read vulnerability in the
bpf_tcp_gen_syncookie and bpf_tcp_check_syncookie helpers. It ensures
sk_state is checked before accessing sk_protocol to avoid reading past
the memory boundary of mini-sockets.

Link: https://lore.kernel.org/all/ca8d8c570509b02355bb4bd4e56859f3e2564c9c.1785576172.git.rakukuip@gmail.com/

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 11bb0d236822a..16845987b2449 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7684,7 +7684,7 @@ BPF_CALL_5(bpf_tcp_check_syncookie, struct sock *, sk, void *, iph, u32, iph_len
>  		return -EINVAL;
>  
>  	/* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> -	if (sk->sk_protocol != IPPROTO_TCP || sk->sk_state != TCP_LISTEN)
> +	if (sk->sk_state != TCP_LISTEN || sk->sk_protocol != IPPROTO_TCP)
>  		return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but does the same slab
out-of-bounds read occur in bpf_sock_destroy()?

It looks like bpf_sock_destroy() takes a struct sock_common but casts it
unconditionally to struct sock before accessing sk->sk_protocol, which
could also be a mini-socket (like request_sock or inet_timewait_sock).

net/core/filter.c:bpf_sock_destroy() {
    ...
	if (!sk->sk_prot->diag_destroy || (sk->sk_protocol != IPPROTO_TCP &&
					   sk->sk_protocol != IPPROTO_UDP))
		return -EOPNOTSUPP;
    ...
}

Should we validate the socket type before dereferencing sk->sk_protocol
there too?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6218aa3534d0d2d3f448fde70a8dc2769d7a8201.1785823138.git.rakukuip@gmail.com?part=1