Re: [PATCH net v2] udp_tunnel: fix GRO callback mismatch during setup

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · 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 still lets receive and completion use different tunnel
  parsers on the same packet, so the out-of-bounds write the commit…
- [Medium] udp_tunnel_gro_count only describes how the head skb of a GRO
  entry was parsed, not the packets merged into it, so packets in the…
- [Medium] The Fixes: tag blames only 5d7f5b2f6b93 ("udp_tunnel: use
  static call for GRO hooks when possible", v6.16), but part of what the…
- [Low] The new synchronize_net() in udp_tunnel_update_gro_rcv()
  (net/ipv4/udp_offload.c:139) runs with the global…

--

udp_tunnel: fix GRO callback mismatch during setup

setup_udp_tunnel_sock() now writes gro_complete first and publishes
gro_receive last with release semantics, and udp_gro_receive() reads
gro_receive once. A new per-skb udp_tunnel_gro_count in napi_gro_cb is
incremented for each tunnel gro_receive callback and consumed in
udp_gro_complete(). The static call is used only when its target matches
the socket's callback, and a grace period is waited before resetting it.

> Fixes: 5d7f5b2f6b93 ("udp_tunnel: use static call for GRO hooks when possible")

[Severity: Medium]
Is 5d7f5b2f6b93 the only commit that should be named here?

That commit added the static call machinery and udp_tunnel_gro_rcv(). It
did not touch the gro_receive/gro_complete stores in
setup_udp_tunnel_sock() or this check in udp_gro_complete():

    if (sk && udp_sk(sk)->gro_complete) {

The older mismatch goes like this. Receive treats the packet as plain UDP
and holds it through udp_gro_receive_segment(). One example is sk == NULL
because udp_encap_needed_key is not enabled yet, on a device with
NETIF_F_GRO_UDP_FWD. setup_udp_tunnel_sock() then sets gro_complete. The
unconditional lookup in udp_gro_complete() finds the socket and runs the
tunnel gro_complete on a packet that was never tunnel-parsed.

The new udp_tunnel_gro_count is what closes that case: with a count of 0
the packet goes to udp_gro_complete_segment(). The commit message only
describes the socket callbacks and the global static call disagreeing,
though.

Stable tooling relies on the Fixes: tag, so kernels without 5d7f5b2f6b93
(6.12, 6.6, 6.1, ...) would not get this fix. Could another Fixes: tag be
added for the commit that introduced socket-based UDP tunnel GRO
completion, or at least a note in the commit message?

> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index b77ae2f00c626..92c5e34bcf734 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -136,6 +136,7 @@ void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
>  		static_branch_enable(&udp_tunnel_static_call);
>  	} else if (old_gro_type_nr == 1) {
>  		static_branch_disable(&udp_tunnel_static_call);
> +		synchronize_net();
>  		static_call_update(udp_tunnel_gro_rcv, dummy_gro_rcv);
>  	}

[Severity: Low]
This synchronize_net() runs with the global udp_tunnel_gro_type_lock mutex
held. Is that a concern, given that unprivileged users can reach this
path?

udp_lib_setsockopt(UDP_ENCAP, UDP_ENCAP_ESPINUDP) on a socket with
UDP_GRO set:
    set_xfrm_gro_udp_encap_rcv()->udp_tunnel_update_gro_rcv(sk, true)

close():
    udp_destroy_sock()->udp_tunnel_cleanup_gro()
        ->udp_tunnel_update_gro_rcv(sk, false)

These callers don't hold rtnl, so synchronize_net() turns into a regular
synchronize_rcu(). An unprivileged process can keep triggering the 1->0
or 1->2 transitions, for example by alternating xfrm4/xfrm6 ESP-in-UDP
sockets.

vxlan, geneve and fou setup and teardown take the same mutex, usually
under rtnl. Would they then wait behind those grace periods and add to
rtnl latency?

[ ... ]

> @@ -847,7 +863,7 @@ struct sk_buff *udp_gro_receive(struct list_head *head, struct sk_buff *skb,
>  
>  	skb_gro_pull(skb, sizeof(struct udphdr)); /* pull encapsulating udp header */
>  	skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
> -	pp = udp_tunnel_gro_rcv(sk, head, skb);
> +	pp = udp_tunnel_gro_rcv(gro_receive, sk, head, skb);
>  
>  out:
>  	skb_gro_flush_final(skb, pp, flush);

[Severity: Medium]
Does udp_tunnel_gro_count cover the packets merged into a held GRO entry,
or only the head skb?

Say a packet P is held before setup_udp_tunnel_sock() publishes
gro_receive. P goes through udp_gro_receive_segment() in one of two ways:

  - NETIF_F_GRO_FRAGLIST, with is_flist = 1 because the kernel tunnel
    socket lacks GRO_ENABLED
  - sk == NULL plus NETIF_F_GRO_UDP_FWD while udp_encap_needed_key is
    still off

Either way, P has udp_tunnel_gro_count == 0 and encap_mark == 0.

A packet S on the same outer flow arrives after setup and takes the
tunnel path. The loop over head above only compares the ports and
whether the checksum is zero:

    if ((*(u32 *)&uh->source != *(u32 *)&uh2->source) ||
        (!uh->check ^ !uh2->check)) {

Nothing checks whether p itself was tunnel-parsed. Then
udp_tunnel_gro_rcv()->vxlan_gro_receive()->eth/inet/tcp gro_receive walk
P at p->data + off_vx and the inner offsets. They can merge S's inner
payload into P through skb_gro_receive() or skb_gro_receive_list().

At flush, udp_gro_complete() (or the fraglist shortcut in
udp4/6_gro_complete()) looks only at P's own count of 0. It completes the
aggregate as plain SKB_GSO_UDP_L4 or fraglist. P's gso_size is the outer
UDP payload size, but the merged data has its tunnel and inner headers
stripped.

Can this corrupt segmentation and payload for packets that arrive during
the setup window?

For napi_gro_frags() style drivers, P's linear area ends at the outer UDP
payload (see gro_try_pull_from_frag0()). Would S's tunnel receive then
also read P's inner headers beyond skb_headlen(P)?

[ ... ]

> @@ -933,7 +950,12 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff,
>  
>  	sk = INDIRECT_CALL_INET(lookup, udp6_lib_lookup_skb,
>  				udp4_lib_lookup_skb, skb, uh->source, uh->dest);
> -	if (sk && udp_sk(sk)->gro_complete) {
> +	if (NAPI_GRO_CB(skb)->udp_tunnel_gro_count) {
> +		NAPI_GRO_CB(skb)->udp_tunnel_gro_count--;
> +		if (sk)
> +			gro_complete = READ_ONCE(udp_sk(sk)->gro_complete);
> +	}
> +	if (gro_complete) {
>  		skb_shinfo(skb)->gso_type = uh->check ? SKB_GSO_UDP_TUNNEL_CSUM
>  					: SKB_GSO_UDP_TUNNEL;
>  

[Severity: High]
Does the counter actually tie completion to the gro_receive that ran at
receive time?

udp_tunnel_gro_rcv() only increments udp_tunnel_gro_count. It does not
record which callback or socket ran. Here udp_gro_complete() still does a
fresh udp4_lib_lookup_skb()/udp6_lib_lookup_skb() at flush time. Whenever
the counter is non-zero, it calls the gro_complete of whatever socket
that lookup returns.

Held GRO packets are not inside an RCU section. They can stay held across
NAPI polls when gro_flush_timeout and napi_defer_hard_irqs are set, since
napi_complete_done() only does gro_flush_normal(&n->gro, !!timeout). So
something like this looks possible:

  vxlan_gro_receive() aggregates the packet, udp_tunnel_gro_count = 1,
  and the packet is held on the NAPI GRO list

  the vxlan link is deleted:
    vxlan_sock_release()->udp_tunnel_sock_release()
    (nothing flushes the lower device's GRO lists)

  a fou/GUE port is added on the same UDP port:
    setup_udp_tunnel_sock() runs while the packet is still held

  flush:
    udp_gro_complete()->udp4_lib_lookup_skb() returns the GUE socket
    gro_complete is gue_gro_complete()

gue_gro_complete() in net/ipv4/fou_core.c then does:

    guehlen = sizeof(*guehdr) + (guehdr->hlen << 2);
    ...
    err = ops->callbacks.gro_complete(skb, nhoff + guehlen);

hlen comes from vxlan header bytes. vxlan_gro_prepare_receive() only
compared those bytes for flags and VNI, so guehlen can be up to 128
bytes. The inner inet/ipv6 gro_complete handlers then write
tot_len/payload_len/check and dispatch L4 completes at offsets that
receive never validated.

Can this still cause the slab-out-of-bounds write shown in the KASAN
report in the commit message? It needs CAP_NET_ADMIN in a netns, which
an unprivileged user can get through a user namespace with a tun
IFF_NAPI device.

The opposite case also seems possible. If the new socket has no
gro_complete, or the completion lookup returns NULL while the receive
lookup found a socket, the counter is still consumed. Then
udp_gro_complete_segment() marks a tunnel-aggregated skb as plain
SKB_GSO_UDP_L4.

One way for the two lookups to differ is the port-only fast path in
udp4_gro_lookup_skb():

    sk = udp_tunnel_sk(net, false);
    if (sk && dport == htons(sk->sk_num))
        return sk;

__udp4_lib_lookup() at completion has no such path. A VRF slave with
udp_l3mdev_accept=0 is one example.

This sequence could also happen before this patch. However, the stated
goal of the patch is to stop receive and completion from parsing one
packet differently. Would it work better to record at receive time which
callback or socket ran, or to refuse tunnel completion when they differ,
instead of counting callbacks?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111214622.2482547-2-Jeremy.Jean%40oss.cyber.gouv.fr