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

[PATCH ipsec] 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 the cloned
frag_list member, producing a linear frag_list child skb allocated from
slab with head_frag == 0.

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_sublist_rcv from ip_list_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

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 is still a clone sharing skb_shinfo with the outer TCP skb held by
strparser. Instead, call skb_gso_reset(skb) at skip_cow in esp_input() and
esp6_input(), where the skb is guaranteed to be uncloned and its
skb_shinfo is private.

Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Cc: stable@vger.kernel.org
Signed-off-by: Guoshuai Yang <yangguoshuai@gmail.com>
---
 net/ipv4/esp4.c | 3 +++
 net/ipv6/esp6.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index e76db58..416638c 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -900,6 +900,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)
+		skb_gso_reset(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..02cfc85 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -943,6 +943,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)
+		skb_gso_reset(skb);
+
 	ret = -ENOMEM;
 	tmp = esp_alloc_tmp(aead, nfrags, seqhilen);
 	if (!tmp)
-- 
2.54.0 (Apple Git-157)

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

[netdev-bot+sashiko]

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

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

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 fix clears the outer-TCP GSO metadata only on the head
  skb.

--

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

This patch clears the stale outer TCP GSO metadata (gso_size, gso_segs,
gso_type) on espintcp packets. It does this at the skip_cow label in
esp_input() and esp6_input(), where the skb is no longer cloned. The goal
is to stop skb_segment() from hitting BUG_ON(!list_skb->head_frag) when a
decrypted inner packet with a strparser-built frag_list is forwarded.

> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index e76db5817e78e..416638c990825 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -900,6 +900,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)
> +		skb_gso_reset(skb);
> +

[Severity: Medium]

Does this reset only reach the head skb? skb_gso_reset() writes to
skb_shinfo(skb) only, and the frag_list members seem to keep the outer
TCP GSO state.

In the case the commit message describes, strparser chains a cloned
spillover member onto frag_list. skb_cow_data() then replaces that member
with skb_copy(), and skb_copy_header() copies the GSO fields over from
the shared outer TCP shinfo:

net/core/skbuff.c:skb_copy_header() {
    ...
	skb_shinfo(new)->gso_size = skb_shinfo(old)->gso_size;
	skb_shinfo(new)->gso_segs = skb_shinfo(old)->gso_segs;
	skb_shinfo(new)->gso_type = skb_shinfo(old)->gso_type;
}

So after decryption the member still has, for example, SKB_GSO_TCPV4 with
gso_size set to the outer MSS.

The head is no longer GSO after this patch. Take a forwarded inner IPv4
packet with DF clear that is larger than the egress MTU. It passes
ip_forward(), since ip_exceeds_mtu() is false when DF is clear. It then
goes through ip_fragment() instead of ip_finish_output_gso():

__ip_finish_output()
  ip_fragment()
    ip_do_fragment()            /* frag_list fast path */
      ip_fraglist_prepare()     /* CHECKSUM_NONE, gso_* left as is */
...
validate_xmit_skb()
  netif_needs_gso()             /* true for each such fragment */
    skb_gso_segment()
      inet_gso_segment()
        tcp4_gso_segment()

The fast path is taken when the head payload is a multiple of 8 and fits
the mtu, each member fits the mtu, there is enough headroom, and nothing
is shared or cloned.

Take an inner TCP packet over an IPv4 outer stream. tcp4_gso_segment()
accepts the stale SKB_GSO_TCPV4, clears th->check, and writes a
pseudo-header checksum into what is really fragment payload.
tcp_gso_segment() then either returns -EINVAL and the fragment is dropped
(payload <= stale MSS), or splits the payload into garbage segments.

With an IPv6 outer stream (stale SKB_GSO_TCPV6) or an inner UDP packet,
the gso_type check fails and the fragment is dropped with -EINVAL. The
same applies to esp6_input(). An inner IPv4 packet arriving over an IPv6
espintcp SA can reach ip_do_fragment() with stale SKB_GSO_TCPV6 on its
members.

In either case, would the forwarded datagram fail to reassemble at the
destination? This path does not seem able to reach the skb_segment()
BUG_ON from the commit message, because the members are standalone linear
skbs by then. But the outer TCP GSO state is still set on them, so the
reset described in the subject is only partly done.

Could the GSO fields also be cleared on each skb_walk_frags() member at
skip_cow, in both esp_input() and esp6_input()? The members should be
private there, because skb_cow_data() copies every cloned or nonlinear
member.

>  	err = -ENOMEM;
>  	tmp = esp_alloc_tmp(aead, nfrags, seqhilen);
>  	if (!tmp)

[ ... ]

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