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

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 comple=
tely 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 <[email protected]>

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.=
[email protected]/

> 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;
> =20
>  	/* sk_listener() allows TCP_NEW_SYN_RECV, which makes no sense here. */
> -	if (sk->sk_protocol !=3D IPPROTO_TCP || sk->sk_state !=3D TCP_LISTEN)
> +	if (sk->sk_state !=3D TCP_LISTEN || sk->sk_protocol !=3D 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 !=3D IPPROTO_TCP &&
					   sk->sk_protocol !=3D IPPROTO_UDP))
		return -EOPNOTSUPP;
    ...
}

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

[ ... ]

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/6218aa3534d0d2d3f44=
[email protected]?part=3D1