netdev - Re: [PATCH net-next v6 15/25] ovpn: implement multi-peer support

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: <7acf098f-777b-4785-9009-4388b47b0bd6@openvpn.net>
Date: Fri, 6 Sep 2024 15:26:08 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org, pabeni@...hat.com,
 ryazanov.s.a@...il.com, edumazet@...gle.com, andrew@...n.ch
Subject: Re: [PATCH net-next v6 15/25] ovpn: implement multi-peer support



On 04/09/2024 12:10, Sabrina Dubroca wrote:
> 2024-09-03, 16:40:51 +0200, Sabrina Dubroca wrote:
>> 2024-08-27, 14:07:55 +0200, Antonio Quartulli wrote:
>>> +static int ovpn_peer_add_mp(struct ovpn_struct *ovpn, struct ovpn_peer *peer)
>>> +{
>>> +	struct sockaddr_storage sa = { 0 };
>>> +	struct hlist_nulls_head *nhead;
>>> +	struct sockaddr_in6 *sa6;
>>> +	struct sockaddr_in *sa4;
>>> +	struct hlist_head *head;
>>> +	struct ovpn_bind *bind;
>>> +	struct ovpn_peer *tmp;
>>> +	size_t salen;
>>> +
>>> +	spin_lock_bh(&ovpn->peers->lock_by_id);
>>> +	/* do not add duplicates */
>>> +	tmp = ovpn_peer_get_by_id(ovpn, peer->id);
>>> +	if (tmp) {
>>> +		ovpn_peer_put(tmp);
>>> +		spin_unlock_bh(&ovpn->peers->lock_by_id);
>>> +		return -EEXIST;
>>> +	}
>>> +
>>> +	hlist_add_head_rcu(&peer->hash_entry_id,
>>> +			   ovpn_get_hash_head(ovpn->peers->by_id, &peer->id,
>>> +					      sizeof(peer->id)));
>>> +	spin_unlock_bh(&ovpn->peers->lock_by_id);
>>> +
>>> +	bind = rcu_dereference_protected(peer->bind, true);
> 
> What protects us here? We just released lock_by_id and we're not
> holding peer->lock.

hmm. I think originally it was not possible to hold this peer in any 
other context since the peer was stil being added.
But now we have added it to the by_id table already, so we cannot assume 
that anymore.

Maybe I should simply move this assignment before the 
hlist_add_head_rcu() to regain that assumption..

> 
>>> +	/* peers connected via TCP have bind == NULL */
>>> +	if (bind) {
>>> +		switch (bind->remote.in4.sin_family) {
>>> +		case AF_INET:
>>> +			sa4 = (struct sockaddr_in *)&sa;
>>> +
>>> +			sa4->sin_family = AF_INET;
>>> +			sa4->sin_addr.s_addr = bind->remote.in4.sin_addr.s_addr;
>>> +			sa4->sin_port = bind->remote.in4.sin_port;
>>> +			salen = sizeof(*sa4);
>>> +			break;
>>> +		case AF_INET6:
>>> +			sa6 = (struct sockaddr_in6 *)&sa;
>>> +
>>> +			sa6->sin6_family = AF_INET6;
>>> +			sa6->sin6_addr = bind->remote.in6.sin6_addr;
>>> +			sa6->sin6_port = bind->remote.in6.sin6_port;
>>> +			salen = sizeof(*sa6);
>>> +			break;
>>> +		default:
>>
>> And remove from the by_id hashtable? Or is that handled somewhere that
>> I missed (I don't think ovpn_peer_unhash gets called in that case)?
> 
> ovpn_nl_set_peer_doit does:
> 
> 		ret = ovpn_peer_add(ovpn, peer);
> 		if (ret < 0) {
> [...]
> 		/* release right away because peer is not really used in any
> 		 * context
> 		 */
> 		ovpn_peer_release(peer);
> 		kfree(peer);
> 
> 
> But if we fail at this stage, the peer was published in the by_id
> hashtable and could be used.
> 
> Although AFAICT, ovpn can never create a bind with family !=
> AF_INET{,6}, so this is not a real issue -- in that case I guess a
> DEBUG_NET_WARN_ON_ONCE with a comment that this should never happen
> would be acceptable (but I'd still remove the peer from by_id and go
> through the proper release path instead of direct kfree in
> ovpn_nl_set_peer_doit). Otherwise, you'd have to reorder things in
> this function so that all failures are handled before the peer is
> added to any hashtable.

To be honest I don't mind adding a pre-check and error out immediately.
I don't like adding a peer to the table that is actually failing basic 
sanity checks.

Thanks!

> 
>>> +			return -EPROTONOSUPPORT;
>>> +		}
>>> +
> 

-- 
Antonio Quartulli
OpenVPN Inc.