Re: [PATCH net] udp: check frag_list IP family before using skb_segment_list()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix in net/ipv4/udp_offload.c:__udp_gso_segment() is
  incomplete.
- [Low] The comment added in net/ipv4/udp_offload.c:__udp_gso_segment()
  (lines 517-521) says the code makes sure "the frag_list skbs still…

--

udp: check frag_list IP family before using skb_segment_list()

This patch adds a check to __udp_gso_segment(). The fraglist fast path,
__udp_gso_segment_list(), is now taken only when the IP version of the
first frag_list skb matches the family being segmented. On a mismatch
the code falls back to __skb_linearize() and skb_segment(), so that
__udpv6_gso_segment_list_csum() does not treat an IPv4 header as a
struct ipv6hdr.

> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index cf07c3c6611a2..802365b4780fd 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -514,9 +514,14 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  	}
>  
>  	if (skb_shinfo(gso_skb)->gso_type & SKB_GSO_FRAGLIST) {
> -		 /* Detect modified geometry and pass those to skb_segment. */
> +		/* Detect modified geometry and pass those to skb_segment.
> +		 * SKB_GSO_UDP_L4 is used by both IPv4 and IPv6: also make sure
> +		 * the frag_list skbs still have the IP family we segment for,
> +		 * as their network headers are not rebuilt.
> +		 */

[Severity: Low]
This isn't a bug, but the comment says the code makes sure "the frag_list
skbs still have the IP family we segment for". The condition below only
looks at skb_shinfo(gso_skb)->frag_list, which is the first member.

The idea that every member has the same family as the first one is only
stated in the changelog. Could the comment say that only the first
frag_list skb is checked, and why that is enough?

>  		if ((skb_pagelen(gso_skb) - sizeof(*uh) == skb_shinfo(gso_skb)->gso_size) &&
> -		    !(skb_shinfo(gso_skb)->gso_type & SKB_GSO_DODGY))
> +		    !(skb_shinfo(gso_skb)->gso_type & SKB_GSO_DODGY) &&
> +		    ip_hdr(skb_shinfo(gso_skb)->frag_list)->version == (is_ipv6 ? 6 : 4))
>  			return __udp_gso_segment_list(gso_skb, features, is_ipv6);

[Severity: High]
Is it enough to check only the first frag_list skb here? The changelog
says "All skbs in the list belong to the same flow, so they share the
same IP family". On devices with hard_header_len == 0, though, GRO seems
able to append an IPv4 skb to an IPv6 head.

In gro_list_prepare(), maclen comes from skb->dev->hard_header_len. For
tun in IFF_TUN mode and for wireguard this is 0, so the memcmp() compares
nothing. skb->protocol is never compared:

net/core/gro.c:gro_list_prepare() {
    unsigned int maclen = skb->dev->hard_header_len;
    ...
		else if (!diffs)
			diffs = memcmp(skb_mac_header(p),
				       skb_mac_header(skb),
				       maclen);
    ...
}

inet_gro_receive() then checks only the version of the new skb. When p
is an IPv6 skb, it reads p at IPv4 offsets and never checks
iph2->version:

net/ipv4/af_inet.c:inet_gro_receive() {
    ...
		iph2 = (struct iphdr *)(p->data + off);
    ...
		if ((iph->protocol ^ iph2->protocol) |
		    ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
		    ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
    ...
}

On an IPv6 head these bytes are saddr[1] and saddr[4..11], and the
sender chooses them. The reverse direction is blocked, because
ipv6_gro_receive() masks first_word with 0xF00FFFFF, and that mask
covers the version nibble.

udp_gro_receive_segment() checks ports, is_flist, ip_summed/csum_level
and gro_receive_network_flush(). It then calls skb_gro_receive_list(p,
skb), which does not check the family.

So an IPv6 head A could end up with frag_list = [B (IPv6), C (IPv4)].
The new check passes because B is version 6. skb_segment_list() keeps
C's own 20 byte IPv4 header and only adjusts the transport header.

If the head's addresses or ports were changed, for example by NAT66, tc
pedit or BPF, the early return in __udpv6_gso_segment_list_csum() is not
taken. The loop then treats C as IPv6:

net/ipv4/udp_offload.c:__udpv6_gso_segment_list_csum() {
    ...
	while ((seg = seg->next)) {
		uh2 = udp_hdr(seg);
		iph2 = ipv6_hdr(seg);

		__udpv6_gso_segment_csum(seg, &iph2->saddr, &iph->saddr,
					 &uh2->source, uh->source);
		__udpv6_gso_segment_csum(seg, &iph2->daddr, &iph->daddr,
					 &uh2->dest, uh->dest);
	}
    ...
}

The *oldip = *newip stores in __udpv6_gso_segment_csum() write offsets
8-23 and 24-39 of C's network header. pskb_may_pull(skb,
skb_gro_offset(skb)) only guarantees 28 bytes are linear, up to the end
of the UDP header. That allows a write of up to 12 bytes past the linear
part, into tailroom or skb_shared_info.

With rx-gro-list enabled, a tun device in an unprivileged user namespace
appears to be enough to reach this path. Remote peers on wireguard or
point-to-point tunnels may also be able to send such packets.

Isn't this the same write past the linear part that the patch is meant
to fix? Would it be better to check every frag_list member here? Another
option is to block cross-family merges in GRO, for example by checking
p's IP version in inet_gro_receive().

>  
>  		ret = __skb_linearize(gso_skb);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106141727.2303218-1-edumazet%40kernel.org