Re: [PATCH net-next 1/4] ipv6: fix transport header of forwarded GSO packets

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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] ipv6: unvalidated csum_start in ip6_forward() moves
  transport_header backwards
- [Medium] In ip6_forward() (net/ipv6/ip6_output.c), the new else-branch
  sets transport_header to whatever ipv6_skip_exthdr() returns.
- [Low] The commit fixes a user-visible bug: forwarded IPv6 GSO flows
  with destination-options or routing headers stall on Packet Too Big at…

--

ipv6: fix transport header of forwarded GSO packets

This patch resets transport_header in ip6_forward() for GSO packets before
the MTU check. CHECKSUM_PARTIAL packets without encapsulation take it from
csum_start, and other packets with extension headers walk the chain with
ipv6_skip_exthdr(). This keeps skb_gso_validate_network_len() from reading
a bogus TCP header length, which caused Packet Too Big stalls and
SKB_BAD_GSO drops.

> skb_gso_validate_network_len() then reads a bogus TCP header length. An
> overestimate makes ip6_forward() answer with Packet Too Big at the
> current MTU, and the flow stalls. Since commit 7fb4c1967011 ("net: pull
> headers in qdisc_pkt_len_segs_init()"), a bogus doff below 5 also makes
> __dev_queue_xmit() drop the packet as SKB_BAD_GSO.

[ ... ]

> Signed-off-by: Matt Vollrath <tactii@gmail.com>

[Severity: Low]
Should this patch carry a Fixes: tag and a stable Cc? The only pointer to
where the bug came from, fe6cc55f3a9a ("net: ip, ipv6: handle gso skbs in
forwarding path"), is in the notes below the --- separator, and git am
strips those.

Stable trees that pick up 7fb4c1967011 would start dropping these
forwarded packets as SKB_BAD_GSO. Nothing in the commit points the stable
tooling at this fix.

Would a trailer like this in the commit body work, along with a mention of
7fb4c1967011?

Fixes: fe6cc55f3a9a ("net: ip, ipv6: handle gso skbs in forwarding path")

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a6..60ac56afbd06b 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -662,6 +662,23 @@ int ip6_forward(struct sk_buff *skb)
>  
>  	__IP6_INC_STATS(net, ip6_dst_idev(dst), IPSTATS_MIB_OUTFORWDATAGRAMS);
>  
> +	if (skb_is_gso(skb)) {
> +		if (skb->ip_summed == CHECKSUM_PARTIAL && !skb->encapsulation) {
> +			skb_set_transport_header(skb,
> +						 skb_checksum_start_offset(skb));
> +		} else if (ipv6_ext_hdr(hdr->nexthdr)) {
> +			u8 nexthdr = hdr->nexthdr;
> +			__be16 frag_off;
> +			int thoff;
> +
> +			thoff = ipv6_skip_exthdr(skb, skb_network_offset(skb) +
> +						 sizeof(*hdr), &nexthdr,
> +						 &frag_off);
> +			if (thoff >= 0)
> +				skb_set_transport_header(skb, thoff);

[Severity: High]
Can the CHECKSUM_PARTIAL branch move transport_header backwards, into
the fixed IPv6 header or even in front of skb->data?

csum_start is taken as is here. Nothing checks that
skb_checksum_start_offset(skb) is at least skb_network_offset(skb) +
sizeof(*hdr). Before this patch, the forwarding path kept the
transport_header set by ip6_rcv_core(), which never pointed inside the
fixed header.

For packets injected through tun/tap, macvtap or af_packet with
VIRTIO_NET_HDR_F_NEEDS_CSUM, csum_start comes from userspace. The check
in __virtio_net_hdr_to_skb() runs while skb->data still points at the
Ethernet header:

include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
    ...
		if (!skb_partial_csum_set(skb, start, off))
			return -EINVAL;
		if (skb_transport_offset(skb) < nh_min_len)
			return -EINVAL;
    ...
}

nh_min_len is sizeof(struct ipv6hdr) for VIRTIO_NET_HDR_GSO_TCPV6, so a
csum_start of 40 is accepted. After eth_type_trans() pulls the 14 byte
Ethernet header, that is offset 26 from the network header, in the
middle of the source address. A routed TCPV6 GSO packet built this way
takes this branch and gets transport_header set inside the IPv6
header. From there:

ip6_forward()
  ip6_pkt_too_big()
    skb_gso_validate_network_len()
      skb_gso_network_seglen()
        skb_gso_transport_seglen()
          thlen = tcp_hdrlen(skb);

This reads doff out of the address bytes. The network header length
comes out 14 bytes short and thlen is garbage, so the MTU checks here
and in ip6_finish_output_gso() use the wrong segment length.
qdisc_pkt_len_segs_init() later reads the same bogus doff.

It gets worse if the offset goes negative. bpf_prog_run_generic_xdp()
calls __skb_pull() after bpf_xdp_adjust_head() without adjusting
csum_start, so ip_summed can stay CHECKSUM_PARTIAL with csum_start
behind skb->data. A negative transport offset wraps the unsigned
arithmetic in skb_gso_network_seglen(), which lets the packet past the
MTU checks. Then, in qdisc_pkt_len_segs_init():

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
		hdr_len = skb_transport_offset(skb);
    ...
		th = (const struct tcphdr *)(skb->data + hdr_len);
    ...
}

hdr_len is an unsigned int. With a transport offset between -20 and -1
the length passed to pskb_may_pull() wraps to a small value that
passes, and th ends up about 4 GiB past skb->data on 64-bit. Reading
th->doff there would oops.

Would it be enough to use csum_start only when
skb_checksum_start_offset(skb) >= skb_network_offset(skb) +
sizeof(*hdr), and fall back to the ipv6_skip_exthdr() walk otherwise?

[Severity: Medium]
Can this make tcp_hdrlen() read past the linear data of the skb?

For each extension header, ipv6_skip_exthdr() reads only the 2-byte
generic header through skb_header_pointer(). After the last one it adds
hdrlen without a bounds check, and for destination or routing options
hdrlen can be up to 2048 bytes:

net/ipv6/exthdrs_core.c:ipv6_skip_exthdr() {
    ...
		nexthdr = hp->nexthdr;
		start += hdrlen;
    ...
}

So thoff can point into the page frags or past skb->len. The only check
here is thoff >= 0.

Right after this, the MTU check reads the TCP header directly at the new
offset:

ip6_forward()
  ip6_pkt_too_big()
    skb_gso_validate_network_len()
      skb_gso_network_seglen()
        skb_gso_transport_seglen()
          thlen = tcp_hdrlen(skb);

tcp_hdrlen() reads tcp_hdr(skb)->doff with no pskb_may_pull() and no
skb_header_pointer().

The else branch runs for TCP GSO skbs that are neither CHECKSUM_PARTIAL
nor encapsulated. Two examples are fraglist GRO (CHECKSUM_UNNECESSARY) and
virtio_net_hdr GSO packets without NEEDS_CSUM.

On ingress, only the extension header chain that was actually parsed gets
linearized. Suppose the chain is rewritten before ip6_forward(), for
example by nftables payload mangling in PREROUTING or by a tc BPF
store_bytes. Then the new thoff can land far past skb_headlen(), possibly
past skb->end. Both of those need CAP_NET_ADMIN, which an unprivileged
user namespace has inside its own netns.

Before this patch, transport_header on this path pointed just past the
fixed header, or past a hop-by-hop header that ipv6_parse_hopopts() had
already pulled. Either way the read stayed inside the linear data.

Would a pskb_may_pull(skb, thoff + sizeof(struct tcphdr)) check for TCP
GSO cover this, keeping the old transport_header if the pull fails?

> +		}
> +	}
> +
>  	mtu = ip6_dst_mtu_maybe_forward(dst, true);
>  	if (mtu < IPV6_MIN_MTU)
>  		mtu = IPV6_MIN_MTU;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105201309.3182307-1-tactii%40gmail.com