[PATCH net] net: gro: compare skb->protocol in gro_list_prepare()

[PATCH net] net: gro: compare skb->protocol in gro_list_prepare()

[Eric Dumazet]

gro_list_prepare() checks whether a held packet may belong to the same
flow as the incoming one by comparing the device, VLAN tag, metadata
and the link-layer header, using dev->hard_header_len as its length.

On devices without a link-layer header (hard_header_len == 0), such as
L3 GRE tunnels, no compared byte carries the outer protocol:
skb->protocol comes from the tunnel header. Packets of different
protocols can then stay eligible, and the GRO handler selected from the
incoming skb->protocol parses the held packet with the wrong layout.

For example, take an IPv4/UDP packet held by UDP GRO and a
VLAN/IPv4/TCP packet received on the same ipgre device.
vlan_gro_receive() reads the first four bytes of the held IPv4 header
as a VLAN header. With crafted values, the held packet survives up to
tcp_gro_receive(), which uses its UDP header as a TCP header. With a
short linear part, the FIN/PSH update of the held packet's TCP flags
can then write into skb_shared_info.

Nested GRO handlers (VLAN, GRE, IPv4, IPv6) already compare their own
next-protocol field. Only the outermost protocol was not checked.

Compare skb->protocol too. On Ethernet devices this is redundant with
compare_ether_header(), but it is cheap.

Fixes: 60769a5dcd87 ("ipv4: gre: add GRO capability")
Reported-by: Kyle Zeng <kylebot@openai.com>
Signed-off-by: Eric Dumaze <edumazet@kernel.org>
---
This issue was discovered by an AI code review agent. It has not been
triggered, and the fix was only compile-tested.

 net/core/gro.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/gro.c b/net/core/gro.c
index 1b033855a20db6b8b996179f0b81e38393d3864a..08e4731ddb7e6e371790d70a97db2cedad420706 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -374,6 +374,7 @@ static void gro_list_prepare(const struct list_head *head,
 		}
 
 		diffs = (unsigned long)p->dev ^ (unsigned long)skb->dev;
+		diffs |= p->protocol != skb->protocol;
 		diffs |= p->vlan_all ^ skb->vlan_all;
 		diffs |= skb_metadata_differs(p, skb);
 		if (maclen == ETH_HLEN)
-- 
2.53.0

Re: [PATCH net] net: gro: compare skb->protocol in gro_list_prepare()

[Willem de Bruijn]

Eric Dumazet wrote:
> gro_list_prepare() checks whether a held packet may belong to the same
> flow as the incoming one by comparing the device, VLAN tag, metadata
> and the link-layer header, using dev->hard_header_len as its length.
> 
> On devices without a link-layer header (hard_header_len == 0), such as
> L3 GRE tunnels, no compared byte carries the outer protocol:
> skb->protocol comes from the tunnel header. Packets of different
> protocols can then stay eligible, and the GRO handler selected from the
> incoming skb->protocol parses the held packet with the wrong layout.
> 
> For example, take an IPv4/UDP packet held by UDP GRO and a
> VLAN/IPv4/TCP packet received on the same ipgre device.
> vlan_gro_receive() reads the first four bytes of the held IPv4 header
> as a VLAN header. With crafted values, the held packet survives up to
> tcp_gro_receive(), which uses its UDP header as a TCP header. With a
> short linear part, the FIN/PSH update of the held packet's TCP flags
> can then write into skb_shared_info.
> 
> Nested GRO handlers (VLAN, GRE, IPv4, IPv6) already compare their own
> next-protocol field. Only the outermost protocol was not checked.
> 
> Compare skb->protocol too. On Ethernet devices this is redundant with
> compare_ether_header(), but it is cheap.
> 
> Fixes: 60769a5dcd87 ("ipv4: gre: add GRO capability")
> Reported-by: Kyle Zeng <kylebot@openai.com>
> Signed-off-by: Eric Dumaze <edumazet@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net] net: gro: compare skb->protocol in gro_list_prepare()

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete.
- [Low] The Signed-off-by trailer does not match the commit author.

--

net: gro: compare skb->protocol in gro_list_prepare()

This commit adds a skb->protocol comparison to gro_list_prepare(). Held
packets with a different outer protocol are then no longer treated as
part of the same flow. It targets devices with hard_header_len == 0, such
as L3 GRE tunnels, where none of the compared link-layer bytes carry the
protocol.

> Fixes: 60769a5dcd87 ("ipv4: gre: add GRO capability")
> Reported-by: Kyle Zeng <kylebot@openai.com>
> Signed-off-by: Eric Dumaze <edumazet@kernel.org>

[Severity: Low]
This isn't a bug, but the name in the Signed-off-by is missing its final
"t", so it doesn't match the Author line (Eric Dumazet). checkpatch.pl
reports this as a From:/Signed-off-by: mismatch.

> diff --git a/net/core/gro.c b/net/core/gro.c
> index 1b033855a20db..08e4731ddb7e6 100644
> --- a/net/core/gro.c
> +++ b/net/core/gro.c
> @@ -374,6 +374,7 @@ static void gro_list_prepare(const struct list_head *head,
>  		}
>  
>  		diffs = (unsigned long)p->dev ^ (unsigned long)skb->dev;
> +		diffs |= p->protocol != skb->protocol;

[Severity: High]
Is this enough to close the bug class? The commit message says:

  Nested GRO handlers (VLAN, GRE, IPv4, IPv6) already compare their own
  next-protocol field. Only the outermost protocol was not checked.

In gue_gro_receive() in net/ipv4/fou_core.c, a GUE version 1 packet
(direct IPv4/IPv6 encapsulation, with no GUE header) jumps past the loop
that compares held packets:

net/ipv4/fou_core.c:gue_gro_receive() {
    ...
	case 1:
		switch (((struct iphdr *)guehdr)->version) {
		case 4:
			proto = IPPROTO_IPIP;
			break;
		case 6:
			proto = IPPROTO_IPV6;
			break;
		default:
			goto out;
		}
		goto next_proto;
    ...
	list_for_each_entry(p, head, list) {
		...
		if (guehdr->word != guehdr2->word) {
			NAPI_GRO_CB(p)->same_flow = 0;
			continue;
		}
    ...
}

When a v1 packet arrives on the same outer 5-tuple, a held GUE v0 packet
or a held v1 packet of the other IP family keeps same_flow set.

The incoming packet then goes through ipip_gro_receive()->
inet_gro_receive() (or ipv6_gro_receive()). That code reads the held
packet at the same offset:

net/ipv4/af_inet.c:inet_gro_receive() {
    ...
		iph2 = (struct iphdr *)(p->data + off);
		/* The above works because, with the exception of the top
		 * (inner most) layer, we only aggregate pkts with the same
		 * hdr length so all the hdrs we'll need to verify will start
		 * at the same offset.
		 */
    ...
}

For a held v0 packet, the bytes compared as protocol, saddr and daddr are
really the GUE header plus inner IP header bytes. In the cross-family
case they are IPv6 saddr bytes. The sender controls all of them.

After that, tcp4_gro_receive()->tcp_gro_lookup()->tcp_gro_receive() can
treat a held UDP header as a TCP header. On a merge it does:

	tcp_flag_word(th2) |= flags & (TCP_FLAG_FIN | TCP_FLAG_PSH);

This writes 12 bytes past the start of the 8 byte UDP header. The commit
message describes the same layout confusion and write for ipgre.

Both packets have outer skb->protocol == ETH_P_IP on the same device, so
the new check in gro_list_prepare() doesn't catch this case.

The reverse direction (incoming v0, held v1) is caught by the
guehdr->word comparison, because the version bits differ.

A remote sender can reach this once a GUE port is configured with "ip
fou add ... gue".

Does gue_gro_receive() also need a held packet comparison on the v1 path?
For example, it could clear same_flow for held packets whose first GUE/IP
byte differs.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106195136.2496777-1-edumazet%40kernel.org

Re: [PATCH net] net: gro: compare skb->protocol in gro_list_prepare()

[Eric Dumazet]

Le sam. 7 nov. 2026 à 21:28, <netdev-bot+sashiko@kernel.org> a écrit :
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · 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).
>
> - [High] The fix is incomplete.
> - [Low] The Signed-off-by trailer does not match the commit author.
>
> --
>
> net: gro: compare skb->protocol in gro_list_prepare()
>
> This commit adds a skb->protocol comparison to gro_list_prepare(). Held
> packets with a different outer protocol are then no longer treated as
> part of the same flow. It targets devices with hard_header_len == 0, such
> as L3 GRE tunnels, where none of the compared link-layer bytes carry the
> protocol.
>
> > Fixes: 60769a5dcd87 ("ipv4: gre: add GRO capability")
> > Reported-by: Kyle Zeng <kylebot@openai.com>
> > Signed-off-by: Eric Dumaze <edumazet@kernel.org>
>
> [Severity: Low]
> This isn't a bug, but the name in the Signed-off-by is missing its final
> "t", so it doesn't match the Author line (Eric Dumazet). checkpatch.pl
> reports this as a From:/Signed-off-by: mismatch.
>

Right, will fix in v2.

> > diff --git a/net/core/gro.c b/net/core/gro.c
> > index 1b033855a20db..08e4731ddb7e6 100644
> > --- a/net/core/gro.c
> > +++ b/net/core/gro.c
> > @@ -374,6 +374,7 @@ static void gro_list_prepare(const struct list_head *head,
> >               }
> >
> >               diffs = (unsigned long)p->dev ^ (unsigned long)skb->dev;
> > +             diffs |= p->protocol != skb->protocol;
>
> [Severity: High]
> Is this enough to close the bug class?

The GUE direct IP case is a separate bug: both packets have the same
outer protocol, so no gro_list_prepare() check can catch it. It is
fixed separately by "fou: gue: check IP version of held packets for
direct IP encapsulation", which clears same_flow for held packets
whose first nibble does not match the incoming IP version. This covers
both held GUE v0 packets and held v1 packets of the other family.

I will clarify this in the changelog.

pw-bot: cr