2024-09-03, 16:40:51 +0200, Sabrina Dubroca wrote:

2024-08-27, 14:07:55 +0200, Antonio Quartulli wrote:

quoted

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

quoted

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

quoted

+			return -EPROTONOSUPPORT;
+		}
+

-- 
Sabrina