| 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
| ||
|
Message-ID: <2fdfcff0-aa80-4098-95e1-55d8733b7bc1@openvpn.net>
Date: Tue, 27 May 2025 09:46:21 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Sabrina Dubroca <sd@...asysnail.net>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
Oleksandr Natalenko <oleksandr@...alenko.name>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/4] ovpn: properly deconfigure UDP-tunnel
Hi Jakub,
please refrain from pulling this series.
While stress testing the code in this patch, we realized that:
1) udp_tunnel_encap_disable() is calling the wrong bit-related function
(copy/paste error);
2) cleanup_udp_tunnel_sock() can race with udp_destroy_socket() and lead
to a double decrease of udp_encap_needed_key.
We have already developed and verified the fixes, which I will send with
a following PR.
Should I already target 'net' (even though there is no 6.16-rc1 yet)?
Regards,
On 22/05/2025 16:06, Antonio Quartulli wrote:
> When deconfiguring a UDP-tunnel from a socket, we cannot
> call setup_udp_tunnel_sock(), as it is expected to be invoked
> only during setup.
>
> Implement a new function named cleanup_udp_tunnel_sock(),
> that reverts was what done during setup, and invoke it.
>
> This new function takes care of reverting everything that
> was done by setup_udp_tunnel_sock() (i.e. unsetting various
> members and cleaning up the encap state in the kernel).
>
> Note that cleanup_udp_tunnel_sock() takes 'struct sock'
> as argument in preparation for a follow up patch, where
> 'struct ovpn' won't hold any reference to any 'struct socket'
> any more, but rather to its 'struct sock' member only.
>
> Moreover implement udpv6_encap_disable() in order to
> allow udpv6_encap_needed_key to be decreased outside of
> ipv6.ko (similarly to udp_encap_disable() for IPv4).
>
> Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> Cc: Simon Horman <horms@...nel.org>
> Cc: David Ahern <dsahern@...nel.org>
> Cc: Sabrina Dubroca <sd@...asysnail.net>
> Cc: Oleksandr Natalenko <oleksandr@...alenko.name>
> Reported-by: Paolo Abeni <pabeni@...hat.com>
> Closes: https://lore.kernel.org/netdev/1a47ce02-fd42-4761-8697-f3f315011cc6@redhat.com
> Signed-off-by: Antonio Quartulli <antonio@...nvpn.net>
> ---
> drivers/net/ovpn/udp.c | 5 +----
> include/net/ipv6_stubs.h | 1 +
> include/net/udp.h | 1 +
> include/net/udp_tunnel.h | 13 +++++++++++++
> net/ipv4/udp_tunnel_core.c | 22 ++++++++++++++++++++++
> net/ipv6/af_inet6.c | 1 +
> net/ipv6/udp.c | 6 ++++++
> 7 files changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
> index aef8c0406ec9..c8a81c4d6489 100644
> --- a/drivers/net/ovpn/udp.c
> +++ b/drivers/net/ovpn/udp.c
> @@ -442,8 +442,5 @@ int ovpn_udp_socket_attach(struct ovpn_socket *ovpn_sock,
> */
> void ovpn_udp_socket_detach(struct ovpn_socket *ovpn_sock)
> {
> - struct udp_tunnel_sock_cfg cfg = { };
> -
> - setup_udp_tunnel_sock(sock_net(ovpn_sock->sock->sk), ovpn_sock->sock,
> - &cfg);
> + cleanup_udp_tunnel_sock(ovpn_sock->sock->sk);
> }
> diff --git a/include/net/ipv6_stubs.h b/include/net/ipv6_stubs.h
> index 8a3465c8c2c5..2a57df87d11b 100644
> --- a/include/net/ipv6_stubs.h
> +++ b/include/net/ipv6_stubs.h
> @@ -55,6 +55,7 @@ struct ipv6_stub {
> struct nl_info *info);
>
> void (*udpv6_encap_enable)(void);
> + void (*udpv6_encap_disable)(void);
> void (*ndisc_send_na)(struct net_device *dev, const struct in6_addr *daddr,
> const struct in6_addr *solicited_addr,
> bool router, bool solicited, bool override, bool inc_opt);
> diff --git a/include/net/udp.h b/include/net/udp.h
> index a772510b2aa5..e3d7b8622f59 100644
> --- a/include/net/udp.h
> +++ b/include/net/udp.h
> @@ -580,6 +580,7 @@ void udp_encap_disable(void);
> #if IS_ENABLED(CONFIG_IPV6)
> DECLARE_STATIC_KEY_FALSE(udpv6_encap_needed_key);
> void udpv6_encap_enable(void);
> +void udpv6_encap_disable(void);
> #endif
>
> static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
> index 2df3b8344eb5..843e7c11e5ee 100644
> --- a/include/net/udp_tunnel.h
> +++ b/include/net/udp_tunnel.h
> @@ -92,6 +92,7 @@ struct udp_tunnel_sock_cfg {
> /* Setup the given (UDP) sock to receive UDP encapsulated packets */
> void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
> struct udp_tunnel_sock_cfg *sock_cfg);
> +void cleanup_udp_tunnel_sock(struct sock *sk);
>
> /* -- List of parsable UDP tunnel types --
> *
> @@ -218,6 +219,18 @@ static inline void udp_tunnel_encap_enable(struct sock *sk)
> udp_encap_enable();
> }
>
> +static inline void udp_tunnel_encap_disable(struct sock *sk)
> +{
> + if (udp_test_and_set_bit(ENCAP_ENABLED, sk))
> + return;
> +
> +#if IS_ENABLED(CONFIG_IPV6)
> + if (READ_ONCE(sk->sk_family) == PF_INET6)
> + ipv6_stub->udpv6_encap_disable();
> +#endif
> + udp_encap_disable();
> +}
> +
> #define UDP_TUNNEL_NIC_MAX_TABLES 4
>
> enum udp_tunnel_nic_info_flags {
> diff --git a/net/ipv4/udp_tunnel_core.c b/net/ipv4/udp_tunnel_core.c
> index 2326548997d3..624b6afcf812 100644
> --- a/net/ipv4/udp_tunnel_core.c
> +++ b/net/ipv4/udp_tunnel_core.c
> @@ -98,6 +98,28 @@ void setup_udp_tunnel_sock(struct net *net, struct socket *sock,
> }
> EXPORT_SYMBOL_GPL(setup_udp_tunnel_sock);
>
> +void cleanup_udp_tunnel_sock(struct sock *sk)
> +{
> + /* Re-enable multicast loopback */
> + inet_set_bit(MC_LOOP, sk);
> +
> + /* Disable CHECKSUM_UNNECESSARY to CHECKSUM_COMPLETE conversion */
> + inet_dec_convert_csum(sk);
> +
> + udp_sk(sk)->encap_type = 0;
> + udp_sk(sk)->encap_rcv = NULL;
> + udp_sk(sk)->encap_err_rcv = NULL;
> + udp_sk(sk)->encap_err_lookup = NULL;
> + udp_sk(sk)->encap_destroy = NULL;
> + udp_sk(sk)->gro_receive = NULL;
> + udp_sk(sk)->gro_complete = NULL;
> +
> + rcu_assign_sk_user_data(sk, NULL);
> + udp_tunnel_encap_disable(sk);
> + udp_tunnel_cleanup_gro(sk);
> +}
> +EXPORT_SYMBOL_GPL(cleanup_udp_tunnel_sock);
> +
> void udp_tunnel_push_rx_port(struct net_device *dev, struct socket *sock,
> unsigned short type)
> {
> diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
> index acaff1296783..f3745705a811 100644
> --- a/net/ipv6/af_inet6.c
> +++ b/net/ipv6/af_inet6.c
> @@ -1049,6 +1049,7 @@ static const struct ipv6_stub ipv6_stub_impl = {
> .fib6_rt_update = fib6_rt_update,
> .ip6_del_rt = ip6_del_rt,
> .udpv6_encap_enable = udpv6_encap_enable,
> + .udpv6_encap_disable = udpv6_encap_disable,
> .ndisc_send_na = ndisc_send_na,
> #if IS_ENABLED(CONFIG_XFRM)
> .xfrm6_local_rxpmtu = xfrm6_local_rxpmtu,
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 7317f8e053f1..2f40aa605c82 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -602,6 +602,12 @@ void udpv6_encap_enable(void)
> }
> EXPORT_SYMBOL(udpv6_encap_enable);
>
> +void udpv6_encap_disable(void)
> +{
> + static_branch_dec(&udpv6_encap_needed_key);
> +}
> +EXPORT_SYMBOL(udpv6_encap_disable);
> +
> /* Handler for tunnels with arbitrary destination ports: no socket lookup, go
> * through error handlers in encapsulations looking for a match.
> */
--
Antonio Quartulli
OpenVPN Inc.
Powered by blists - more mailing lists