netdev - Re: [PATCH net-next v11 17/23] ovpn: add support for peer floating

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: <ZzM0U81dmvdEWqdF@hog>
Date: Tue, 12 Nov 2024 11:56:19 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.net>
Cc: Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Donald Hunter <donald.hunter@...il.com>,
	Shuah Khan <shuah@...nel.org>, ryazanov.s.a@...il.com,
	Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH net-next v11 17/23] ovpn: add support for peer floating

2024-10-29, 11:47:30 +0100, Antonio Quartulli wrote:
> diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> index 63c140138bf98e5d1df79a2565b666d86513323d..0e8a6f2c76bc7b2ccc287ad1187cf50f033bf261 100644
> --- a/drivers/net/ovpn/io.c
> +++ b/drivers/net/ovpn/io.c
> @@ -135,6 +135,15 @@ void ovpn_decrypt_post(void *data, int ret)
>  	/* keep track of last received authenticated packet for keepalive */
>  	peer->last_recv = ktime_get_real_seconds();
>  
> +	if (peer->sock->sock->sk->sk_protocol == IPPROTO_UDP) {

What prevents peer->sock from being replaced and released
concurrently?

Or possibly reading the error value that ovpn_socket_new can return
before peer->sock is reset to NULL, just noticed this in
ovpn_nl_peer_modify:

	if (attrs[OVPN_A_PEER_SOCKET]) {
		// ...
		peer->sock = ovpn_socket_new(sock, peer);
		if (IS_ERR(peer->sock)) {
			// ...
			peer->sock = NULL;


(ovpn_encrypt_post has a similar check on
peer->sock->sock->sk->sk_protocol that I don't think is safe either)


> +		/* check if this peer changed it's IP address and update
> +		 * state
> +		 */
> +		ovpn_peer_float(peer, skb);
> +		/* update source endpoint for this peer */
> +		ovpn_peer_update_local_endpoint(peer, skb);

Why not do both in the same function? They're not called anywhere else
(at least in this version of the series). They both modify peer->bind
depending on skb_protocol_to_family(skb), and operate under
peer->lock.


> +void ovpn_peer_float(struct ovpn_peer *peer, struct sk_buff *skb)
> +{
> +	struct hlist_nulls_head *nhead;
> +	struct sockaddr_storage ss;
> +	const u8 *local_ip = NULL;
> +	struct sockaddr_in6 *sa6;
> +	struct sockaddr_in *sa;
> +	struct ovpn_bind *bind;
> +	sa_family_t family;
> +	size_t salen;
> +
> +	rcu_read_lock();
> +	bind = rcu_dereference(peer->bind);
> +	if (unlikely(!bind)) {
> +		rcu_read_unlock();
> +		return;
> +	}
> +
> +	spin_lock_bh(&peer->lock);

You could take the lock from the start, instead of using rcu_read_lock
to get peer->bind. It would guarantee that the bind we got isn't
already being replaced just as we wait to update it. And same in
ovpn_peer_update_local_endpoint, it would make sure we're updating the
local IP for the active bind.

(sorry I didn't think about that last time we discussed this)

> +	if (likely(ovpn_bind_skb_src_match(bind, skb)))
> +		goto unlock;
> +
> +	family = skb_protocol_to_family(skb);
> +

-- 
Sabrina