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: <ZtgyA744W7QkXXnX@hog>
Date: Wed, 4 Sep 2024 12:10:11 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Antonio Quartulli <antonio@...nvpn.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

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.

> > +	/* 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.

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

-- 
Sabrina