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

[netdev-bot+sashiko@kernel.org]

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