[PATCH net v3] xfrm: esp: reset outer TCP GSO metadata on espintcp input

[PATCH net v3] xfrm: esp: reset outer TCP GSO metadata on espintcp input

[Guoshuai Yang]

When ESP packets are received over TCP (RFC 8229 espintcp), strparser
slices individual length-prefixed ESP records from the outer TCP stream by
cloning the incoming TCP skb(s) in __strp_recv().

If the outer TCP skb was GRO-coalesced before reaching strparser, its
skb_shinfo carries the outer TCP stream's GSO metadata (gso_size, gso_segs,
and gso_type). When an ESP record spans two outer TCP segments, strparser
chains the spillover clone onto skb_shinfo(head)->frag_list.

Subsequently in esp_input() and esp6_input(), skb_cow_data() unshares the
head skb via pskb_expand_head() (preserving the outer TCP gso_size and
gso_type in the new private skb_shinfo) and calls skb_copy() on each cloned
frag_list member, which copies the outer TCP GSO fields via
skb_copy_header() onto a linear frag_list child skb allocated from slab
with head_frag == 0.

Furthermore, if an outer TCP GRO aggregate carried SKB_GSO_FRAGLIST (e.g.,
when rx-gro-list is enabled) and was chained onto
skb_shinfo(head)->frag_list by __strp_recv(), skb_cow_data() invokes
skb_copy() / skb_copy_expand() on that frag_list member and hits
WARN_ON_ONCE(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST), failing with
-ENOMEM before decryption even begins.

After ESP decryption and header removal, the resulting single inner packet
still has skb_is_gso(skb) == true with the stale outer TCP gso_size, while
skb_headlen(head_skb) is now smaller than that gso_size due to stripping
the outer framing and ESP header. When this packet passes through
gro_cells_receive() (which flushes frag_list packets via GRO_NORMAL
without resetting gso_size) and is forwarded, validate_xmit_skb() invokes
tcp_gso_segment() -> skb_segment(). Because head_skb is shorter than the
stale gso_size and SKB_GSO_DODGY is not set, skb_segment() attempts to map
the slab-backed frag_list member as a scatter-gather page fragment and hits
BUG_ON(!list_skb->head_frag):

  Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
  PC is at skb_segment+0x574/0xd20
  LR is at skb_segment+0x884/0xd20
  Call trace:
   skb_segment from tcp_gso_segment
   tcp_gso_segment from inet_gso_segment
   inet_gso_segment from skb_mac_gso_segment
   skb_mac_gso_segment from __skb_gso_segment
   __skb_gso_segment from validate_xmit_skb
   validate_xmit_skb from __dev_queue_xmit
   __dev_queue_xmit from ip_finish_output2
   ip_finish_output2 from ip_output
   ip_output from ip_sublist_rcv_finish
   ip_sublist_rcv_finish from ip_sublist_rcv
   ip_list_rcv from __netif_receive_skb_list_core
   __netif_receive_skb_list_core from __netif_receive_skb_list
   __netif_receive_skb_list from gro_normal_list
   gro_normal_list from gro_cell_poll

Similarly, if a decrypted inner packet with a frag_list later takes the
ip_do_fragment() frag_list fast path, each detached frag_list member is
transmitted as a standalone IP fragment and would still carry the stale
outer TCP GSO state when reaching validate_xmit_skb(). And even when an
ESP record does not have a frag_list, leaving the stale outer TCP gso_size
on the decrypted inner skb prevents dev_gro_receive() from initializing
gso_size to the inner packet's skb_gro_len().

We cannot reset GSO state earlier in espintcp_rcv() / handle_esp() because
the skb may still be a clone sharing skb_shinfo with the outer TCP skb held
by strparser. Instead, in esp_input() and esp6_input():
1. Prior to skb_cow_data(), unshare/unclone and reset GSO metadata on any
   frag_list member carrying SKB_GSO_FRAGLIST so skb_copy() /
   skb_copy_expand() inside skb_cow_data() does not hit WARN_ON_ONCE() and
   drop the packet.
2. At skip_cow, where the head skb and all frag_list members are guaranteed
   to be uncloned with private skb_shinfo, call skb_gso_reset() on both the
   head skb and each skb_walk_frags() member.

Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Cc: stable@vger.kernel.org
Signed-off-by: Guoshuai Yang <yangguoshuai@gmail.com>
---
v3:
  - Unshare/unclone and reset GSO metadata on any frag_list member carrying
    SKB_GSO_FRAGLIST before skb_cow_data() so skb_copy() / skb_copy_expand()
    does not hit WARN_ON_ONCE(SKB_GSO_FRAGLIST) and return -ENOMEM.
  - Move the shared espintcp GSO reset helpers into include/net/esp.h.
  - Link to v2: https://lore.kernel.org/netdev/20261020030450.7170-1-yangguoshuai@gmail.com/
v2:
  - Also call skb_gso_reset() on each skb_walk_frags() member at skip_cow
    in esp_input() and esp6_input() so detached frag_list members on the
    ip_do_fragment() fast path do not retain stale outer TCP GSO state.
  - Specify target tree 'net' in subject prefix.
  - Link to v1: https://lore.kernel.org/netdev/20261018005218.53267-1-yangguoshuai@gmail.com/
 include/net/esp.h | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 net/ipv4/esp4.c   |  9 +++++++++
 net/ipv6/esp6.c   |  9 +++++++++
 3 files changed, 69 insertions(+)

diff --git a/include/net/esp.h b/include/net/esp.h
index 3229507..09ed682 100644
--- a/include/net/esp.h
+++ b/include/net/esp.h
@@ -28,6 +28,57 @@ static inline void esp_output_fill_trailer(u8 *tail, int tfclen, int plen, __u8
 	tail[plen - 1] = proto;
 }
 
+static inline int esp_reset_espintcp_fraglist_gso(struct sk_buff *skb)
+{
+	struct sk_buff **fragp;
+	struct sk_buff *frag;
+
+	skb_walk_frags(skb, frag) {
+		if (skb_shinfo(frag)->gso_type & SKB_GSO_FRAGLIST)
+			break;
+	}
+	if (!frag)
+		return 0;
+
+	if (skb_unclone(skb, GFP_ATOMIC))
+		return -ENOMEM;
+
+	for (fragp = &skb_shinfo(skb)->frag_list; (frag = *fragp);
+	     fragp = &frag->next) {
+		if (!(skb_shinfo(frag)->gso_type & SKB_GSO_FRAGLIST))
+			continue;
+
+		if (skb_shared(frag)) {
+			struct sk_buff *nfrag;
+
+			nfrag = skb_clone(frag, GFP_ATOMIC);
+			if (!nfrag)
+				return -ENOMEM;
+
+			nfrag->next = frag->next;
+			consume_skb(frag);
+			frag = nfrag;
+			*fragp = frag;
+		}
+
+		if (skb_unclone(frag, GFP_ATOMIC))
+			return -ENOMEM;
+
+		skb_gso_reset(frag);
+	}
+
+	return 0;
+}
+
+static inline void esp_reset_espintcp_gso(struct sk_buff *skb)
+{
+	struct sk_buff *iter;
+
+	skb_gso_reset(skb);
+	skb_walk_frags(skb, iter)
+		skb_gso_reset(iter);
+}
+
 struct esp_info {
 	struct	ip_esp_hdr *esph;
 	__be64	seqno;
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index e76db58..92980cd 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -893,6 +893,12 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 		}
 	}
 
+	if (x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP) {
+		err = esp_reset_espintcp_fraglist_gso(skb);
+		if (err)
+			goto out;
+	}
+
 	err = skb_cow_data(skb, 0, &trailer);
 	if (err < 0)
 		goto out;
@@ -900,6 +906,9 @@ static int esp_input(struct xfrm_state *x, struct sk_buff *skb)
 	nfrags = err;
 
 skip_cow:
+	if (x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP)
+		esp_reset_espintcp_gso(skb);
+
 	err = -ENOMEM;
 	tmp = esp_alloc_tmp(aead, nfrags, seqhilen);
 	if (!tmp)
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index b1c9b36..65470d3 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -936,6 +936,12 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 		}
 	}
 
+	if (x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP) {
+		ret = esp_reset_espintcp_fraglist_gso(skb);
+		if (ret)
+			goto out;
+	}
+
 	nfrags = skb_cow_data(skb, 0, &trailer);
 	if (nfrags < 0) {
 		ret = -EINVAL;
@@ -943,6 +949,9 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
 	}
 
 skip_cow:
+	if (x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP)
+		esp_reset_espintcp_gso(skb);
+
 	ret = -ENOMEM;
 	tmp = esp_alloc_tmp(aead, nfrags, seqhilen);
 	if (!tmp)
-- 
2.54.0 (Apple Git-157)

Re: [PATCH net v3] xfrm: esp: reset outer TCP GSO metadata on espintcp input

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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).

- [Medium] The commit message presents a crash as the current failure
  mode: skb_segment() 'hits BUG_ON(!list_skb->head_frag)', followed by…
- [Medium] esp_reset_espintcp_fraglist_gso() (include/net/esp.h:31-71)
  replaces a shared frag_list member by writing `*fragp = frag` (line…
- [Low] The commit message names 'rx-gro-list is enabled' as the example
  trigger for the SKB_GSO_FRAGLIST case.

--

xfrm: esp: reset outer TCP GSO metadata on espintcp input

This patch clears stale outer TCP GSO metadata from espintcp ESP records
in esp_input() and esp6_input(). Before skb_cow_data(), a new helper
unclones the head and any frag_list members that carry SKB_GSO_FRAGLIST.
At skip_cow, skb_gso_reset() is called on the head and on every
frag_list member.

> Furthermore, if an outer TCP GRO aggregate carried SKB_GSO_FRAGLIST (e.g.,
> when rx-gro-list is enabled) and was chained onto
> skb_shinfo(head)->frag_list by __strp_recv(), skb_cow_data() invokes
> skb_copy() / skb_copy_expand() on that frag_list member and hits
> WARN_ON_ONCE(skb_shinfo(skb)->gso_type & SKB_GSO_FRAGLIST), failing with
> -ENOMEM before decryption even begins.

[Severity: Low]
Is enabling rx-gro-list enough on its own to reach this case?

tcp4_check_fraglist_gro() looks up an established socket in the netns of
the GRO device. It enables fraglist GRO only when no socket is found:

net/ipv4/tcp_offload.c:tcp4_check_fraglist_gro() {
    ...
	net = dev_net_rcu(skb->dev);
	sk = __inet_lookup_established(net, iph->saddr, th->source,
				       iph->daddr, ntohs(th->dest),
				       iif, sdif);
	NAPI_GRO_CB(skb)->is_flist = !sk && !skb_is_gso(skb);
    ...
}

tcp6_check_fraglist_gro() does the same.

An espintcp flow always ends at a locally established TCP socket. If that
socket is in the same netns as the GRO device, is_flist stays 0. In that
case only plain SKB_GSO_TCPV4/V6 state reaches esp_input(), and the
skip_cow reset handles it without the new pre-pass.

It looks like esp_reset_espintcp_fraglist_gso() only matters when GRO runs
in a different netns from the espintcp socket and the aggregate arrives
unsegmented. One example is a host NIC forwarding into a container over
veth, since VETH_FEATURES includes NETIF_F_GSO_SOFTWARE and so
NETIF_F_GSO_FRAGLIST.

Could the commit message describe that topology? Could it also say that
this half of the fix depends on TCP fraglist GRO? That feature is much
newer than e27cca96cd68 from the Fixes: tag, so on stable trees without it
the pre-pass does nothing.

> stale gso_size and SKB_GSO_DODGY is not set, skb_segment() attempts to map
> the slab-backed frag_list member as a scatter-gather page fragment and hits
> BUG_ON(!list_skb->head_frag):
>
>   Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
>   PC is at skb_segment+0x574/0xd20

[Severity: Medium]
Is there still a BUG_ON() at this point? In the tree this patch targets,
skb_segment() handles the condition like this:

net/core/skbuff.c:skb_segment() {
    ...
				} else {
					if (unlikely(!list_skb->head_frag)) {
						DEBUG_NET_WARN_ON_ONCE(1);
						err = -EINVAL;
						goto err;
					}
    ...
}

On current kernels, segmentation seems to fail and validate_xmit_skb()
drops the packet. A warning appears only with CONFIG_DEBUG_NET, and there
is no oops.

Could the commit message say which kernel the trace came from? Could it
also describe the behavior on the kernels this patch targets?

> diff --git a/include/net/esp.h b/include/net/esp.h
> index 322950727dd0a..09ed682209113 100644
> --- a/include/net/esp.h
> +++ b/include/net/esp.h
> @@ -28,6 +28,57 @@ static inline void esp_output_fill_trailer(u8 *tail, int tfclen, int plen, __u8
>  	tail[plen - 1] = proto;
>  }
>
> +static inline int esp_reset_espintcp_fraglist_gso(struct sk_buff *skb)
> +{

[ ... ]

> +	if (skb_unclone(skb, GFP_ATOMIC))
> +		return -ENOMEM;
> +
> +	for (fragp = &skb_shinfo(skb)->frag_list; (frag = *fragp);
> +	     fragp = &frag->next) {
> +		if (!(skb_shinfo(frag)->gso_type & SKB_GSO_FRAGLIST))
> +			continue;
> +
> +		if (skb_shared(frag)) {
> +			struct sk_buff *nfrag;
> +
> +			nfrag = skb_clone(frag, GFP_ATOMIC);
> +			if (!nfrag)
> +				return -ENOMEM;
> +
> +			nfrag->next = frag->next;
> +			consume_skb(frag);
> +			frag = nfrag;
> +			*fragp = frag;

[Severity: Medium]
Can fragp point at the ->next field of a member that is still shared when
this store happens?

When the head is cloned, the skb_unclone(skb) call above goes through
pskb_expand_head(). That calls skb_clone_fraglist(), which only takes a
reference on each member:

net/core/skbuff.c:skb_clone_fraglist() {
	skb_walk_frags(skb, list)
		skb_get(list);
}

After that, the old shinfo and the new shinfo both walk the same ->next
pointers.

Take a chain A -> B where A has no SKB_GSO_FRAGLIST, B does, and both are
now shared. The loop skips A with continue. It then clones B into N, calls
consume_skb(B), and writes A->next = N.

A is shared, so the other owner's chain now points at N as well. B still
holds one reference but is no longer linked anywhere, so it leaks. N has
users == 1 but two chains reach it.

skb_cow_data() runs next. It sees that A is shared, copies A and
everything after it, and calls kfree_skb() on N, while the other owner's
A->next still points at N. Can that lead to a use-after-free or double
free of N?

The -ENOMEM returns after this point can leave the same cross-linked chain
behind.

This relink pattern looks like it comes from ___pskb_trim(). That function
handles skb_shared() on every member before skipping any, so it never
writes through a shared predecessor. Would it be safer to privatize every
member walked so far, including the skipped ones, or to copy from the first
shared member onward the way skb_cow_data() does?

I could not find a current producer that hands esp_input() a cloned head,
or a shared non-FRAGLIST member in front of a FRAGLIST member. strparser
unclones the head or allocates a fresh one before chaining. Still, the
skb_unclone(skb) and skb_shared() branches here exist only for that case.

[ ... ]

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