Re: [PATCH net 1/1] net: Don't deliver IPv6 packets to IPv4 sockets

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

Pre-existing issues:
- [Critical] Missing `sk_family` validation in `inet_steal_sock()` allows type confusion with `AF_UNIX` sockets, leading to arbitrary code execution.
- [Critical] Missing `sk_protocol` validation in `inet_steal_sock()` and `inet6_steal_sock()` allows cross-protocol type confusion between TCP and UDP sockets.
--

--- Patch [1]: [PATCH net 1/1] net: Don't deliver IPv6 packets to IPv4 sockets ---
commit dbdb2f67dbf7c27de3851c77618430e20ab40c0b
Author: Shihuang Liu <[email protected]>

net: Don't deliver IPv6 packets to IPv4 sockets

bpf_sk_assign() allows a TC ingress program to attach an arbitrary
hashed socket to an skb, without checking that the socket family
matches the packet's network layer. As a result, an IPv6 UDP packet
can be assigned to an AF_INET UDP socket: udpv6_rcv() steals the
socket via inet6_steal_sock(), which also lacks a family check, and
queues the IPv6 skb on the AF_INET socket.

> diff --git a/include/net/inet6_hashtables.h b/include/net/inet6_hashtables.h
> index 2cc5d416bbb5a..b39e59efac8d7 100644
> --- a/include/net/inet6_hashtables.h
> +++ b/include/net/inet6_hashtables.h
> @@ -115,6 +115,12 @@ struct sock *inet6_steal_sock(struct net *net, struct sk_buff *skb, int doff,
>  	if (!sk)
>  		return NULL;
>  
> +	if (unlikely(sk->sk_family != AF_INET6)) {
> +		if (*refcounted)
> +			sock_gen_put(sk);
> +		return NULL;
> +	}
> +

[Severity: Critical]
This is a pre-existing issue, but missing sk_family validation in
inet_steal_sock() allows type confusion with AF_UNIX sockets.

Since inet_steal_sock() completely lacks the sk_family != AF_INET check that
was added here for IPv6, bpf_sk_assign() can attach an AF_UNIX socket to an
IPv4 packet. Because sk_protocol for an AF_UNIX socket is 0, it is returned
to the IPv4 UDP receive path.

Could this allow an attacker to dereference the IP options payload as a file
pointer list in unix_detach_fds(), leading to arbitrary code execution?

net/unix/af_unix.c:__unix_dgram_recvmsg() {
    ...
    if (!(flags & MSG_PEEK)) {
        if (UNIXCB(skb).fp)
            unix_detach_fds(&scm, skb);
    ...
}

Should we add an if (unlikely(sk->sk_family != AF_INET)) check to
inet_steal_sock() in include/net/inet_hashtables.h to prevent this?

>  	if (!prefetched || !sk_fullsock(sk))
>  		return sk;
>  

[Severity: Critical]
This is a pre-existing issue, but missing sk_protocol validation in
inet6_steal_sock() and inet_steal_sock() allows cross-protocol type confusion
between TCP and UDP sockets.

Because bpf_sk_assign() does not verify that a selected socket's protocol
matches the packet's transport protocol, inet6_steal_sock() and
inet_steal_sock() can return a TCP socket to the UDP receive path or vice
versa.

Could this lead to out-of-bounds heap corruption? For example, if a UDP
socket is assigned to a TCP packet, wouldn't tcp_v4_rcv() write to fields
like tp->segs_in which reside far past the end of the smaller udp_sock
allocation?

net/ipv4/tcp_ipv4.c:tcp_v4_rcv() {
    ...
    tcp_segs_in(tcp_sk(sk), skb);
    ...
}

Similarly, if a TCP socket is assigned to a UDP packet, attacker-controllable
fields in tcp_sock overlap with sensitive function pointers in udp_sock,
potentially allowing control-flow hijacking.

Would it be safer to ensure that sk->sk_protocol matches the packet being
processed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1