netdev - Re: [PATCH net-next 10/10] ovpn: ensure sk is still valid during cleanup

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: <20250512183742.28fad543@kernel.org>
Date: Mon, 12 May 2025 18:37:42 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>, Paolo Abeni
 <pabeni@...hat.com>, Sabrina Dubroca <sd@...asysnail.net>, Al Viro
 <viro@...iv.linux.org.uk>, Qingfang Deng <dqfext@...il.com>, Gert Doering
 <gert@...enie.muc.de>
Subject: Re: [PATCH net-next 10/10] ovpn: ensure sk is still valid during
 cleanup

On Fri,  9 May 2025 16:26:20 +0200 Antonio Quartulli wrote:
> In case of UDP peer timeout, an openvpn client (userspace)
> performs the following actions:
> 1. receives the peer deletion notification (reason=timeout)
> 2. closes the socket
> 
> Upon 1. we have the following:
> - ovpn_peer_keepalive_work()
>  - ovpn_socket_release()
>   - synchronize_rcu()
> At this point, 2. gets a chance to complete and ovpn_sock->sock->sk
> becomes NULL. ovpn_socket_release() will then attempt dereferencing it,
> resulting in the following crash log:

What runs where is a bit unclear to me. Specifically I'm not sure what
runs the code under the "if (released)" branch of ovpn_socket_release()
if the user closes the socket. Because you now return without a WARN().

> @@ -75,13 +76,14 @@ void ovpn_socket_release(struct ovpn_peer *peer)
>  	if (!sock)
>  		return;
>  
> -	/* sanity check: we should not end up here if the socket
> -	 * was already closed
> +	/* sock->sk may be released concurrently, therefore we
> +	 * first attempt grabbing a reference.
> +	 * if sock->sk is NULL it means it is already being
> +	 * destroyed and we don't need any further cleanup
>  	 */
> -	if (!sock->sock->sk) {
> -		DEBUG_NET_WARN_ON_ONCE(1);
> +	sk = sock->sock->sk;
> +	if (!sk || !refcount_inc_not_zero(&sk->sk_refcnt))

How is sk protected from getting reused here?
refcount_inc_not_zero() still needs the underlying object to be allocated.
I don't see any locking here, and code says this function may sleep so 
it can't be called under RCU, either.