netdev - Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <67dad64082fc5_594829474@willemb.c.googlers.com.notmuch>
Date: Wed, 19 Mar 2025 10:35:44 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Paolo Abeni <pabeni@...hat.com>, 
 netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>, 
 David Ahern <dsahern@...nel.org>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Simon Horman <horms@...nel.org>, 
 Willem de Bruijn <willemb@...gle.com>, 
 steffen.klassert@...unet.com
Subject: Re: [PATCH net-next] udp_tunnel: properly deal with xfrm gro encap.

Paolo Abeni wrote:
> The blamed commit below does not take in account that xfrm
> can enable GRO over UDP encapsulation without going through
> setup_udp_tunnel_sock().
> 
> At deletion time such socket will still go through
> udp_tunnel_cleanup_gro(), and the failed GRO type lookup will
> trigger the reported warning.
> 
> We can safely remove such warning, simply performing no action
> on failed GRO type lookup at deletion time.
> 
> Reported-by: syzbot+8c469a2260132cd095c1@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=8c469a2260132cd095c1
> Fixes: 311b36574ceac ("udp_tunnel: use static call for GRO hooks when possible")
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>

Because XFRM does not call udp_tunnel_update_gro_rcv when enabling its
UDP GRO offload, from set_xfrm_gro_udp_encap_rcv. But it does call it
when disabling the offload, as called for all udp sockest from
udp(v6)_destroy_sock. (Just to verify my understanding.)

Not calling udp_tunnel_update_gro_rcv on add will have the unintended
side effect of enabling the static call if one other tunnel is also
active, breaking UDP GRO for XFRM socket, right? Not a significant
consequence. But eventually XFRM would want to be counted, I suppose.

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

> ---
>  net/ipv4/udp_offload.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 088aa8cb8ac0c..2e0b52ae665bc 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -110,14 +110,7 @@ void udp_tunnel_update_gro_rcv(struct sock *sk, bool add)
>  		cur = &udp_tunnel_gro_types[udp_tunnel_gro_type_nr++];
>  		refcount_set(&cur->count, 1);
>  		cur->gro_receive = up->gro_receive;
> -	} else {
> -		/*
> -		 * The stack cleanups only successfully added tunnel, the
> -		 * lookup on removal should never fail.
> -		 */
> -		if (WARN_ON_ONCE(!cur))
> -			goto out;
> -
> +	} else if (cur) {
>  		if (!refcount_dec_and_test(&cur->count))
>  			goto out;
>  
> -- 
> 2.48.1
>