Re: [PATCH net 2/2] selftests: net: check TCP IP IDs across tunnels after GRO and GSO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] ipv6: seg6: use-after-free of hdr in ipv6_srh_rcv() after iptunnel_pull_offloads()
- [Medium] selftests: net: unchecked tunnel() return status in run_decap()
- [Medium] udp: gso: inner hw_enc_features re-masking clobbers remcsum NETIF_F_HW_CSUM
--

commit e0a5534c2eabd4b1e53b2a69aaee94090aff3fec
Author: Julius Bairaktaris <julius@bairaktaris.de>

selftests: net: check TCP IP IDs across tunnels after GRO and GSO

This series updates GSO fixed-ID flag handling across tunnel encapsulation,
decapsulation, and segmentation paths, and adds a network selftest verifying
inner and outer IPv4 IDs across IPIP, GRE, and VXLAN tunnels.

> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index cf07c3c6611a2..edcabe1115a60 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -205,7 +205,6 @@ static struct sk_buff *__skb_udp_tunnel_segment(struct sk_buff *skb,
>  
>  	/* setup inner skb. */
>  	skb->encapsulation = 0;
> -	SKB_GSO_CB(skb)->encap_level = 0;
>  	__skb_pull(skb, tnl_hlen);
>  	skb_reset_mac_header(skb);
>  	skb_set_network_header(skb, skb_inner_network_offset(skb));

[Severity: Medium]
When __skb_udp_tunnel_segment() processes a packet with remote checksum
offload enabled, it masks features with skb->dev->hw_enc_features, strips
all checksum bits, and sets only the hardware checksum bit:

net/ipv4/udp_offload.c:__skb_udp_tunnel_segment() {
	...
	features &= skb->dev->hw_enc_features;
	if (need_csum)
		features &= ~NETIF_F_SCTP_CRC;

	if (remcsum) {
		features &= ~NETIF_F_CSUM_MASK;
		if (!need_csum || offload_csum)
			features |= NETIF_F_HW_CSUM;
	}

	/* segment inner packet. */
	segs = gso_inner_segment(skb, features);
	...
}

With SKB_GSO_CB(skb)->encap_level = 0 removed here, encap_level remains
greater than zero when gso_inner_segment() invokes inet_gso_segment() in
net/ipv4/af_inet.c or ipv6_gso_segment() in net/ipv6/ip6_offload.c:

	encap = SKB_GSO_CB(skb)->encap_level > 0;
	if (encap)
		features &= skb->dev->hw_enc_features;

Won't this second masking with skb->dev->hw_enc_features clear the
NETIF_F_HW_CSUM bit that __skb_udp_tunnel_segment() just set for remcsum on
any device where hw_enc_features does not include NETIF_F_HW_CSUM (such as
devices with hw_enc_features == 0 or devices only advertising protocol-
specific checksum flags), forcing skb_segment() into the software checksum
path?

> diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
> index 96c8402a27a54..126058772bc5a 100644
> --- a/net/ipv6/exthdrs.c
> +++ b/net/ipv6/exthdrs.c

[ ... ]

> @@ -406,7 +407,10 @@ static int ipv6_srh_rcv(struct sk_buff *skb, struct inet6_dev *idev)
>  
>  			skb_reset_network_header(skb);
>  			skb_reset_transport_header(skb);
> -			skb->encapsulation = 0;
> +			if (iptunnel_pull_offloads(skb)) {
> +				kfree_skb(skb);
> +				return -1;
> +			}
>  			if (hdr->nexthdr == NEXTHDR_IPV4)
>  				skb->protocol = htons(ETH_P_IP);
>  			__skb_tunnel_rx(skb, skb->dev, net);

[Severity: Critical]
In ipv6_srh_rcv(), hdr is initialized from skb_transport_header(skb) at the
start of the function:

net/ipv6/exthdrs.c:ipv6_srh_rcv() {
	...
	hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);

When hdr->segments_left == 0 on entry, the skb_cloned(skb) check later in
ipv6_srh_rcv() has not run. Calling iptunnel_pull_offloads(skb) on a cloned
GSO packet calls skb_unclone(skb, GFP_ATOMIC) -> pskb_expand_head(), which
allocates a new skb->head buffer and frees the old one:

include/net/ip_tunnels.h:iptunnel_pull_offloads() {
	if (skb_is_gso(skb)) {
		int err;

		err = skb_unclone(skb, GFP_ATOMIC);
		if (unlikely(err))
			return err;
	...
}

Can checking hdr->nexthdr after iptunnel_pull_offloads(skb) returns
dereference a dangling hdr pointer into the freed skb->head buffer?

> diff --git a/tools/testing/selftests/net/tcp_fixedid.sh b/tools/testing/selftests/net/tcp_fixedid.sh
> new file mode 100755
> index 0000000000000..88f9298174077
> --- /dev/null
> +++ b/tools/testing/selftests/net/tcp_fixedid.sh
> @@ -0,0 +1,291 @@

[ ... ]

> +# run_decap <fixed: inner|outer>: VXLAN from NSRC, decapsulated in NRTR
> +run_decap() {
> +	local fixed=$1 want=incr flags=-F
> +	local r0mac t0
> +
> +	setup || { log_test_skip "decap"; return; }
> +	ip -n "$NRTR" link add vx0 address "$VX_MAC" type vxlan id 42 \
> +		dstport 4789 local 10.1.0.2 nolearning ||
> +		{ log_test_skip "decap"; cleanup_all_ns; return; }
> +	ip -n "$NRTR" addr add 10.3.0.2/24 dev vx0
> +	ip -n "$NRTR" link set vx0 up
> +	tunnel "$NRTR" tun0 ipip 4 10.2.0.1 10.2.0.2
> +	route_into "$NRTR" tun0
> +	noseg "$NRTR" r1
> +
> +	if [ "$fixed" = inner ]; then
> +		want=fixed
> +		flags=-f
> +	fi
> +	r0mac=$(ip -n "$NRTR" -br link show r0 | awk '{ print $3 }')
> +	capture "$NMID" m1
> +	t0=$(tx_packets "$NRTR" tun0)
> +	ip netns exec "$NSRC" "$TX" -T -i s0 -M "$r0mac" -s 10.3.0.1 \
> +		-d "$INNER_DST" -n "$SEGS" -b "$BURSTS" \
> +		-V 10.1.0.1,10.1.0.2 -m "$VX_MAC" $flags
> +	wait "$CAP_PID"
> +	check "decap VXLAN with fixed $fixed IDs, into IPIP" \
> +		"$(tail -n 1 "$RESULT")" "$want" 1 \
> +		$(($(tx_packets "$NRTR" tun0) - t0))
> +	cleanup_all_ns
> +}

[Severity: Medium]
Unlike run_encap() and run_encap_decap(), run_decap() does not check the
return status of the tunnel helper when creating the ipip device tun0:

	tunnel "$NRTR" tun0 ipip 4 10.2.0.1 10.2.0.2

If the kernel supports vxlan but does not have ipip support enabled, won't
tun0 creation fail silently here, causing capture to wait for its full poll
timeout and tx_packets to output an empty string that triggers an arithmetic
syntax error in:

	$(($(tx_packets "$NRTR" tun0) - t0))

and fails the test instead of skipping it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792857378.git.julius@bairaktaris.de?part=2