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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5432cc4f-99f2-43c2-b228-94fb38c2f2d0@redhat.com>
Date: Tue, 25 Mar 2025 10:52:54 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>, netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
 Nathan Chancellor <nathan@...nel.org>
Subject: Re: [PATCH net-next v2 1/5] udp_tunnel: properly deal with xfrm gro
 encap.

Hi,

On 3/21/25 5:33 PM, Willem de Bruijn wrote:
> 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.
>>
>> Add the GRO accounting for XFRM tunnel when GRO is enabled, and
>> adjust the known gro types accordingly.
>>
>> Note that we can't use setup_udp_tunnel_sock() here, as the xfrm
>> tunnel setup can be "incremental" - e.g. the encapsulation is created
>> first and GRO is enabled later.
>>
>> Also we can not allow GRO sk lookup optimization for XFRM tunnels, as
>> the socket could match the selection criteria at enable time, and
>> later on the user-space could disconnect/bind it breaking such
>> criteria.
>>
>> 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>
>> ---
>> v1 -> v2:
>>  - do proper account for xfrm, retain the warning
>> ---
>>  net/ipv4/udp.c         | 5 +++++
>>  net/ipv4/udp_offload.c | 4 +++-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index db606f7e41638..79efbf465fb04 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -2903,10 +2903,15 @@ static void set_xfrm_gro_udp_encap_rcv(__u16 encap_type, unsigned short family,
>>  {
>>  #ifdef CONFIG_XFRM
>>  	if (udp_test_bit(GRO_ENABLED, sk) && encap_type == UDP_ENCAP_ESPINUDP) {
>> +		bool old_enabled = !!udp_sk(sk)->gro_receive;
>> +
>>  		if (family == AF_INET)
>>  			WRITE_ONCE(udp_sk(sk)->gro_receive, xfrm4_gro_udp_encap_rcv);
>>  		else if (IS_ENABLED(CONFIG_IPV6) && family == AF_INET6)
>>  			WRITE_ONCE(udp_sk(sk)->gro_receive, ipv6_stub->xfrm6_gro_udp_encap_rcv);
>> +
>> +		if (!old_enabled && udp_sk(sk)->gro_receive)
>> +			udp_tunnel_update_gro_rcv(sk, true);
> 
> The second part of the condition is always true right?

Jakub noted my initial reply did not land on the ML, sorry.

Yes, AFAICS the second part of the condition should be always true, or
at least I fail to see how to reach there otherwise.

Still syzkaller is too good to prove me wrong, I guess it's good to err
on the pedantic/safe side here - checking that condition explicitly.
It's also IMHO more readable.

Cheers,

Paolo


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ