Re: [PATCH net] ipv6: gro: avoid truncating transport header offsets

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The fix is incomplete because inet_gro_receive() still truncates
  the 16-bit transport header offset.

--

ipv6: gro: avoid truncating transport header offsets

This patch switches ipv6_gro_receive() to skb_set_transport_header_careful().
A long IPv6 extension header chain can no longer silently truncate the
16-bit skb->transport_header offset. If the offset can't be represented,
the skb is not held as a GRO head.

> diff --git a/net/ipv6/ip6_offload.c b/net/ipv6/ip6_offload.c
> index 700f1861d8eb7..de4bab42359ba 100644
> --- a/net/ipv6/ip6_offload.c
> +++ b/net/ipv6/ip6_offload.c
> @@ -241,7 +241,8 @@ INDIRECT_CALLABLE_SCOPE struct sk_buff *ipv6_gro_receive(struct list_head *head,
>  		skb_gro_pull(skb, sizeof(*iph));
>  	}
>  
> -	skb_set_transport_header(skb, skb_gro_offset(skb));
> +	if (!skb_set_transport_header_careful(skb, skb_gro_offset(skb)))
> +		goto out;

[Severity: High]
Is the fix complete? This check only validates the outer IPv6 transport
offset, so any value up to 65534 passes.

ip4ip6_offload is always registered for IPPROTO_IPIP in ipv6_offload_init()
via inet6_add_offload(&ip4ip6_offload, IPPROTO_IPIP). Its handler,
ip4ip6_gro_receive(), passes the skb straight to inet_gro_receive(), which
still does an unchecked assignment:

net/ipv4/af_inet.c:inet_gro_receive() {
    ...
	skb_gro_pull(skb, sizeof(*iph));
	skb_set_transport_header(skb, skb_gro_offset(skb));
    ...
}

ipv6_gro_pull_exthdrs() does not limit the chain length. Take a packet of
the form IPv6 + DSTOPTS chain + IPIP + IPv4 (0x45) + TCP/UDP. Choose
ext_len so that (skb->data - skb->head) + 40 + ext_len falls in
65516..65534. That window is 19 bytes wide, so it always contains a
multiple of 8.

Such a packet passes skb_set_transport_header_careful(). The 20-byte inner
IPv4 header then pushes the offset to 65536 or more. transport_header wraps
to 0..18, which points into the headroom at p->head:

ipv6_gro_receive()
  ipv6_gro_pull_exthdrs()
  skb_set_transport_header_careful()   <- passes, offset <= 65534
  ip4ip6_gro_receive()
    inet_gro_receive()
      skb_set_transport_header()       <- offset + 20 truncated
      tcp4_gro_receive()
        tcp_gro_receive()
          gro_receive_network_flush()

For a held packet p, tcp_gro_lookup() compares ports through tcp_hdr(p).
If they match, gro_receive_network_flush() gets a negative
skb_transport_offset(p). That leads to the same read, about 64KiB before
p's network header, that the commit message describes.

If the packets merge, it gets worse. tcp4_gro_complete() would write
th->check through the truncated tcp_hdr() into the headroom, and
tcp_gro_complete() would set csum_start from the same bad pointer.

GRE over IPv6 looks like it reaches inet_gro_receive() the same way, since
gre_offload is also registered with inet6_add_offload(). So do UDP tunnels
over IPv6 that carry an inner IPv4 header. In those cases the extra
encapsulation bytes make the window wider.

Should inet_gro_receive() also use skb_set_transport_header_careful()? Or
could the check be done in one common place so every L3 GRO handler is
covered?

>  
>  	NAPI_GRO_CB(skb)->proto = proto;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/avTTQNyvwPP6LtGx%40v4bel