[PATCH net v3 0/2] net: gro: fix frag0 and remcsum handling

[PATCH net v3 0/2] net: gro: fix frag0 and remcsum handling

[Eric Dumazet]

Two GRO fixes for paths using remote checksum offload (VXLAN, GUE)
and for NAPI_GRO_CB(skb)->frag0 after a pull.

Patch 1 skips skb_gro_remcsum_cleanup() once the skb was merged,
as grc->offset can no longer be trusted, except for fraglist members,
whose inner checksum is restored.

Patch 2 invalidates frag0 before a pull, which commit bd56a29c7a4e
("net: gro: change skb_gro_network_header()") stopped doing.

v3:
- Patch 1: restore the inner checksum of fraglist members instead of
  keeping the adjusted value, which was wrong for forwarded packets
  and for GUE. Only restore it when it is part of the headers
  pulled by skb_gro_receive_list(), as start and offset come from the
  packet. Mention FOU_F_REMCSUM_NOPARTIAL (Sashiko).
- Patch 2: only use frag0 in gro_try_pull_from_frag0() while frag0_len
  is not zero, to not corrupt headers after a failed pull (Sashiko).
- Explain how the issues were found (netdev bot).

v2: https://lore.kernel.org/netdev/20261030125955.3107383-1-edumazet@kernel.org/
- Patch 1: reword the comment and changelog: skb->data is not always
  moved (frag/head steal).
- Patch 2: invalidate frag0 before the pull, as __pskb_pull_tail()
  can fail after pskb_expand_head() freed the old head (Sashiko).
- Patch 2: add a Fixes: tag for UDP fraglist GRO, which pulls
  directly since before bd56a29c7a4e (Sashiko).
- CC Tom Herbert and stable.

v1: https://lore.kernel.org/netdev/20261028141325.2103814-1-edumazet@kernel.org/


Eric Dumazet (2):
  net: gro: do not undo remote checksum offload on merged packets
  net: gro: invalidate frag0 after a pull

 include/net/gro.h       | 27 +++++++++++++++++++++++++++
 net/core/gro.c          |  9 ++++++++-
 net/ipv4/esp4_offload.c |  1 +
 net/ipv4/udp_offload.c  |  1 +
 net/ipv6/esp6_offload.c |  1 +
 5 files changed, 38 insertions(+), 1 deletion(-)

-- 
2.53.0

[PATCH net v3 1/2] net: gro: do not undo remote checksum offload on merged packets

[Eric Dumazet]

skb_gro_flush_final_remcsum() calls skb_gro_remcsum_cleanup() to undo
the checksum adjustment done by skb_gro_remcsum_process() when the
packet was not consumed by GRO.

When the inner handler merged the packet (NAPI_GRO_CB(skb)->same_flow
is set), grc->offset can no longer be trusted:

- skb_gro_receive() either gives the frags or the head of the skb
  to the held packet, or uses __skb_pull(skb, offset) before adding
  the skb to the frag_list of the held packet.

- skb_gro_receive_list() uses skb_pull(skb, skb_gro_offset(skb)).

grc->offset is relative to the original skb->data, so the 2-byte
read-modify-write done by skb_gro_remcsum_cleanup() can land in
the payload of a packet which now belongs to the GRO packet.
pskb_may_pull() can also be called on this frag_list member.

With fraglist GRO, this can also be a use-after-free:
pskb_may_pull(skb, skb_gro_offset(skb)) in skb_gro_receive_list()
or udp_gro_receive_segment() releases the first page fragment if it
only contains headers. NAPI_GRO_CB(skb)->frag0 and frag0_len are not
updated, so skb_gro_remcsum_cleanup() takes the fast path and writes
through frag0 into a page the skb no longer holds a reference on.
The non merged case, where frag0 is used after this pull, is handled
in the following patch.

For skbs merged by skb_gro_receive(), the cleanup can be skipped:
the skb is freed, or its headers are rebuilt from the head skb in
skb_segment().

Fraglist members need the cleanup: skb_segment_list() only copies the
link-layer and tunnel headers from the head skb, so members keep their
own inner headers, with the adjusted checksum. A forwarded VXLAN or GUE
packet would then leave with a modified inner checksum, and since
__copy_skb_header() copies the cleared remcsum_offload bit of the head
skb, gue_remcsum() or vxlan_remcsum() would adjust it a second time.
skb_gro_receive_list() pulled the headers in the linear part before
moving skb->data past them, so restore the checksum there.
The remote checksum start and offset come from the packet: only
restore the checksum if it is part of these headers, which is the
case for a valid packet, where it belongs to the inner transport
header. Otherwise the write could land past the linear part.

This affects VXLAN (VXLAN_F_REMCSUM_NOPARTIAL) and GUE
(FOU_F_REMCSUM_NOPARTIAL).

This was found while fixing a report from Taesoo Kim and Cen Zhang,
who used LLM assisted code analysis and provided reproducers.

Fixes: 26c4f7da3e41 ("net: Fix remcsum in GRO path to not change packet")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 include/net/gro.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/net/gro.h b/include/net/gro.h
index 2300b6da05b2..13a8ef217a1d 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -354,6 +354,23 @@ static inline void skb_gro_remcsum_cleanup(struct sk_buff *skb,
 	if (!grc->delta)
 		return;
 
+	if (NAPI_GRO_CB(skb)->same_flow) {
+		/* skb_segment_list() keeps the inner headers of fraglist
+		 * members: restore their checksum. skb_gro_receive_list()
+		 * pulled these headers, then moved skb->data past them.
+		 * A valid remote checksum offset points to the inner
+		 * transport header, which is part of these headers.
+		 * Headers of other merged skbs are not used anymore.
+		 */
+		if (NAPI_GRO_CB(skb)->is_flist &&
+		    grc->offset + sizeof(u16) <= skb_gro_offset(skb))
+			remcsum_unadjust((__sum16 *)(skb->data -
+						     skb_gro_offset(skb) +
+						     grc->offset),
+					 grc->delta);
+		return;
+	}
+
 	ptr = skb_gro_header(skb, plen, grc->offset);
 	if (!ptr)
 		return;
-- 
2.53.0

[PATCH net v3 2/2] net: gro: invalidate frag0 after a pull

[Eric Dumazet]

Since commit bd56a29c7a4e ("net: gro: change skb_gro_network_header()"),
skb_gro_header_slow() no longer invalidates NAPI_GRO_CB(skb)->frag0
after pskb_may_pull(). skb_gro_may_pull() keeps comparing against the
frag0_len value set in skb_gro_reset_offset(), assuming that after a
slow pull, all later accesses are beyond frag0_len.

This is not true:

- udp4_gro_receive() and udp6_gro_receive() read the network header
  with skb_gro_network_header() after udp_gro_udphdr() pulled the
  UDP header.

- vxlan_gro_receive() calls skb_gro_remcsum_cleanup() at a lower
  offset after the inner handlers pulled their headers.

- skb_gro_remcsum_process() can pull beyond the inner Ethernet header
  before eth_gro_receive() reads it.

- napi_frags_skb() calls __skb_pull(skb, ETH_HLEN) after a slow pull,
  so frag0 still points to the Ethernet header.

If the pull consumed the first page fragment, __skb_pull_tail() released
it. If the head had to grow, pskb_expand_head() freed the old one.
These reads, and the 2-byte write in skb_gro_remcsum_cleanup(), then
access memory the skb no longer holds a reference on.

esp4_gro_receive(), esp6_gro_receive(), skb_gro_receive_list() and
udp_gro_receive_segment() also pull directly and never invalidated
frag0.

Add skb_gro_frag0_invalidate(), which only clears frag0_len, and call
it before each of these pulls. This is a single store in the slow path,
and frag0 stays non NULL, so skb_gro_network_header() does not need
a NULL test.

frag0 must be invalidated even if the pull fails: __pskb_pull_tail()
can return NULL after pskb_expand_head() freed the old head, for
instance if a frag_list member has to be cloned or pulled.

As frag0 is now invalidated before a pull that can fail,
gro_try_pull_from_frag0() can see an skb whose linear part does not
cover skb_gro_offset() while frag0 still points to the start of the
packet, for instance after skb_gro_remcsum_cleanup() pulled a shorter
length. Copying from frag0 would then corrupt the headers: only use
frag0 while frag0_len is not zero, and use pskb_may_pull() otherwise.

This was found by Taesoo Kim and Cen Zhang, using LLM assisted code
analysis, and confirmed with reproducers.

Fixes: bd56a29c7a4e ("net: gro: change skb_gro_network_header()")
Fixes: 9fd1ff5d2ac7 ("udp: Support UDP fraglist GRO/GSO.")
Cc: stable@vger.kernel.org
Reported-by: Taesoo Kim <tsgatesv@gmail.com>
Reported-by: Cen Zhang <cenzhang@linux.microsoft.com>
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
---
 include/net/gro.h       | 10 ++++++++++
 net/core/gro.c          |  9 ++++++++-
 net/ipv4/esp4_offload.c |  1 +
 net/ipv4/udp_offload.c  |  1 +
 net/ipv6/esp6_offload.c |  1 +
 5 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/net/gro.h b/include/net/gro.h
index 13a8ef217a1d..2e6e4d6da725 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -160,9 +160,19 @@ static inline bool skb_gro_may_pull(const struct sk_buff *skb,
 	return likely(hlen <= NAPI_GRO_CB(skb)->frag0_len);
 }
 
+/* A pull can release the page fragment frag0 points to, or reallocate
+ * skb->head, even if it fails. Call this before the pull, to make sure later
+ * skb_gro_header() and skb_gro_network_header() calls no longer use frag0.
+ */
+static inline void skb_gro_frag0_invalidate(struct sk_buff *skb)
+{
+	NAPI_GRO_CB(skb)->frag0_len = 0;
+}
+
 static inline void *skb_gro_header_slow(struct sk_buff *skb, unsigned int hlen,
 					unsigned int offset)
 {
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_may_pull(skb, hlen))
 		return NULL;
 
diff --git a/net/core/gro.c b/net/core/gro.c
index 1b033855a20d..49e7f64f2775 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -238,6 +238,7 @@ int skb_gro_receive_list(struct sk_buff *p, struct sk_buff *skb)
 		     NAPI_GRO_CB(skb)->flush))
 		return -E2BIG;
 
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_may_pull(skb, skb_gro_offset(skb))) {
 		NAPI_GRO_CB(skb)->flush = 1;
 		return -ENOMEM;
@@ -454,8 +455,14 @@ static void gro_try_pull_from_frag0(struct sk_buff *skb)
 {
 	int grow = skb_gro_offset(skb) - skb_headlen(skb);
 
-	if (grow > 0)
+	if (grow <= 0)
+		return;
+
+	/* frag0 can only be used if no pull happened. */
+	if (likely(NAPI_GRO_CB(skb)->frag0_len))
 		gro_pull_from_frag0(skb, grow);
+	else
+		pskb_may_pull(skb, skb_gro_offset(skb));
 }
 
 static void gro_flush_oldest(struct gro_node *gro, struct list_head *head)
diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index abd77162f5e7..2309468cfc3d 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -37,6 +37,7 @@ static struct sk_buff *esp4_gro_receive(struct list_head *head,
 	__be32 seq;
 	__be32 spi;
 
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_pull(skb, offset))
 		return NULL;
 
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index cf07c3c6611a..cd9d7426b227 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -743,6 +743,7 @@ static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
 			pp = p;
 		} else {
 			if (NAPI_GRO_CB(skb)->is_flist) {
+				skb_gro_frag0_invalidate(skb);
 				if (!pskb_may_pull(skb, skb_gro_offset(skb))) {
 					NAPI_GRO_CB(skb)->flush = 1;
 					return NULL;
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index 22895521a57d..ca947cb94fb7 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -64,6 +64,7 @@ static struct sk_buff *esp6_gro_receive(struct list_head *head,
 	if (NAPI_GRO_CB(skb)->proto == IPPROTO_UDP)
 		encap_type = UDP_ENCAP_ESPINUDP;
 
+	skb_gro_frag0_invalidate(skb);
 	if (!pskb_pull(skb, offset))
 		return NULL;
 
-- 
2.53.0