Re: [PATCH net 1/2] net: keep the TCP fixed-ID flags on the header they describe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] ipv6: use-after-free of hdr in ipv6_srh_rcv() after iptunnel_pull_offloads()
- [Medium] net: __skb_udp_tunnel_segment() loses remcsum NETIF_F_HW_CSUM when encap_level > 0
- [Medium] net: geneve_post_decap_hint() leaves fixed-ID flag on outer header after decap
--

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

net: keep the TCP fixed-ID flags on the header they describe

This commit introduces skb_gso_fixedid_encap() and skb_gso_fixedid_decap() to
move the SKB_GSO_TCP_FIXEDID and SKB_GSO_TCP_FIXEDID_INNER flags between
headers during tunnel encapsulation and decapsulation. It also preserves
SKB_GSO_CB(skb)->encap_level in gre_gso_segment() and
__skb_udp_tunnel_segment() so inner IPv4 headers are segmented using the inner
fixed-ID flag.

[ ... ]

> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 5cccf4c0e6913..c2ffda6029a06 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -665,6 +665,8 @@ static inline int iptunnel_pull_offloads(struct sk_buff *skb)
>  		err = skb_unclone(skb, GFP_ATOMIC);
>  		if (unlikely(err))
>  			return err;
> +		if (skb->encapsulation)
> +			skb_gso_fixedid_decap(skb_shinfo(skb));
>  		skb_shinfo(skb)->gso_type &= ~(NETIF_F_GSO_ENCAP_ALL >>
>  					       NETIF_F_GSO_SHIFT);
>  	}

[Severity: Medium]
When a double-encapsulated Geneve GSO packet carrying a GRO hint option is
decapsulated in geneve_udp_encap_recv(), iptunnel_pull_header() calls
iptunnel_pull_offloads(), which shifts SKB_GSO_TCP_FIXEDID_INNER into
SKB_GSO_TCP_FIXEDID and clears skb->encapsulation.

Immediately afterwards, geneve_udp_encap_recv() calls geneve_post_decap_hint()
to restore encapsulation state for the nested tunnel:

drivers/net/geneve.c:geneve_udp_encap_recv() {
    ...
	if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len, inner_proto,
				 !net_eq(geneve->net, dev_net(geneve->dev)))) {
		dev_dstats_rx_dropped(geneve->dev);
		goto drop;
	}
    ...
	if (geneve_post_decap_hint(sk, skb, sizeof(struct genevehdr) +
				   opts_len, &geneveh)) {
    ...
}

drivers/net/geneve.c:geneve_post_decap_hint() {
    ...
	skb->encapsulation = 1;
	skb_set_transport_header(skb, gro_hint->nested_tp_offset);
    ...
	if (uh->check) {
		...
		skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL_CSUM;
		...
	} else {
		skb_shinfo(skb)->gso_type |= SKB_GSO_UDP_TUNNEL;
	}
    ...
}

Doesn't this leave the fixed-ID flag for the innermost TCP/IPv4 header in
SKB_GSO_TCP_FIXEDID instead of SKB_GSO_TCP_FIXEDID_INNER while
skb->encapsulation is 1, causing subsequent segmentation or decapsulation of
the nested tunnel to apply the inner TCP fixed-ID state to the nested tunnel's
outer header and lose the fixed-ID state for the innermost IPv4 header?

[ ... ]

> 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]
In __skb_udp_tunnel_segment(), when SKB_GSO_TUNNEL_REMCSUM is active, features
is first masked with skb->dev->hw_enc_features and then updated to strip
NETIF_F_CSUM_MASK and set NETIF_F_HW_CSUM before calling gso_inner_segment():

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 remaining non-zero, both inet_gso_segment()
and ipv6_gso_segment() see encap as true and mask features with
skb->dev->hw_enc_features a second time:

net/ipv4/af_inet.c:inet_gso_segment() {
    ...
	encap = SKB_GSO_CB(skb)->encap_level > 0;
	if (encap)
		features &= skb->dev->hw_enc_features;
    ...
}

Won't this second masking clear NETIF_F_HW_CSUM on any device where
hw_enc_features does not include NETIF_F_HW_CSUM (such as devices without
hardware tunnel checksum offload or NICs that only advertise
NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM in hw_enc_features), forcing software
segmentation to compute unnecessary inner payload checksums and disabling
GSO_PARTIAL and frag_list segmentation for remcsum packets?

> 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
> @@ -53,6 +53,7 @@
>  #include <linux/ioam6_genl.h>
>  #include <net/ioam6.h>
>  #include <net/dst_metadata.h>
> +#include <net/ip_tunnels.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -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 points into skb->head via skb_transport_header(skb)
before entering the segments_left == 0 decapsulation block:

net/ipv6/exthdrs.c:ipv6_srh_rcv() {
	hdr = (struct ipv6_sr_hdr *)skb_transport_header(skb);
    ...
looped_back:
	if (hdr->segments_left == 0) {
		if (hdr->nexthdr == NEXTHDR_IPV6 || hdr->nexthdr == NEXTHDR_IPV4) {
			...
			if (iptunnel_pull_offloads(skb)) {
				kfree_skb(skb);
				return -1;
			}
			if (hdr->nexthdr == NEXTHDR_IPV4)
				skb->protocol = htons(ETH_P_IP);
    ...
}

When skb_is_gso(skb) && skb_cloned(skb) is true, iptunnel_pull_offloads()
calls skb_unclone(skb, GFP_ATOMIC) -> pskb_expand_head(), which reallocates
skb->head and frees the old buffer.

Doesn't reading hdr->nexthdr after iptunnel_pull_offloads(skb) cause a
use-after-free read from the old skb->head buffer?

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