Re: [PATCH bpf v8 1/2] net: Validate protocol in skb_steal_sock() for BPF-assigned sockets

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch adds a protocol validation check to skb_steal_sock() to prevent mismatched sockets from causing type confusion. However, it restricts this check exclusively to 'prefetched' sockets. Sockets assigned via TCP early demux (tcp_v4_early_demux()) evaluate prefetched to false. An attacker can use a Netfilter rule at the NF_INET_LOCAL_IN hook to rewrite the IP protocol of an incoming TCP packet to IPPROTO_UDP after early demux has attached a struct tcp_sock. The packet will then be routed to udp_rcv(), which calls skb_steal_sock() and bypasses the check.
- [High] The protocol validation in skb_steal_sock() prevents mismatches like UDP sockets on TCP packets, but fails to prevent type confusion with RAW sockets. A RAW socket can be created with an arbitrary protocol (e.g., socket(AF_INET, SOCK_RAW, IPPROTO_TCP)). Because the patch only verifies sk->sk_protocol against the expected protocol without checking sk->sk_type, a malicious BPF program can assign a RAW socket to a TCP packet. The receive path will then dangerously cast the raw_sock to a tcp_sock, causing out-of-bounds access and kernel panics.
--

commit 5523558843e68f090fac91db8bdd58ed67db17e4
Author: Jiayuan Chen <jiayuan.chen@linux.dev>
net: Validate protocol in skb_steal_sock() for BPF-assigned sockets

This commit adds protocol validation in skb_steal_sock() to reject mismatched
sockets, preventing malicious BPF programs from causing kernel panics by
assigning incorrect socket types to packets. It updates various callers to
pass their expected protocol during the socket steal operation.

Link: https://lore.kernel.org/bpf/20260403015851.148209-1-jiayuan.chen@linux.dev/
Link: https://sashiko.dev/#/patchset/20260403015851.148209-1-jiayuan.chen%40linux.dev

> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
[ ... ]
> @@ -103,6 +105,18 @@ static inline struct sock *skb_steal_sock(struct sk_buff *skb,
>  
>  	*prefetched = skb_sk_is_prefetched(skb);
>  	if (*prefetched) {

[Severity: Critical]
This is a pre-existing issue, but does wrapping the validation inside the
prefetched check leave early demux sockets vulnerable?

Sockets assigned via TCP early demux evaluate prefetched to false because
their destructor is sock_edemux. An attacker could use a Netfilter rule at
the NF_INET_LOCAL_IN hook to rewrite the IP protocol of an incoming TCP
packet to IPPROTO_UDP after early demux has attached a tcp_sock.

The packet would then be routed to udp_rcv(), which calls skb_steal_sock()
and bypasses the validation entirely since prefetched is false. This could
allow the UDP receive path to operate on a tcp_sock, potentially leading to
memory corruption or arbitrary execution via overlapping fields like
encap_rcv. 

Should the protocol validation be moved outside of this check?

> +		/* A non-full socket here is either a reqsk or a
> +		 * timewait sock, both only contain sock_common and
> +		 * lack sk_protocol. Since both can only be TCP,
> +		 * use IPPROTO_TCP as the protocol.
> +		 */
> +		if (unlikely(((sk_fullsock(sk) ? sk->sk_protocol : IPPROTO_TCP) != protocol))) {

[Severity: High]
This is a pre-existing issue, but does checking only sk_protocol without
verifying sk_type allow type confusion with RAW sockets?

A RAW socket can be created with an arbitrary protocol, such as
socket(AF_INET, SOCK_RAW, IPPROTO_TCP). If a BPF program assigns this RAW
socket to a TCP packet, the sk_protocol check will pass, but the receive
path will dangerously cast the raw_sock to a tcp_sock.

Since raw_sock and tcp_sock have completely different layouts, could this
lead to out-of-bounds accesses and kernel panics when the TCP stack tries
to use it? Does sk_type also need to be validated here?

> +			skb_orphan(skb);
> +			*prefetched = false;
> +			*refcounted = false;
> +			return NULL;
> +		}
> +
>  #if IS_ENABLED(CONFIG_SYN_COOKIES)
>  		if (sk->sk_state == TCP_NEW_SYN_RECV && inet_reqsk(sk)->syncookie) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608125846.157004-1-jiayuan.chen@linux.dev?part=1