[PATCH] ovpn: prevent UAF re-add to by_transp_addr on float-vs-delete race

[PATCH] ovpn: prevent UAF re-add to by_transp_addr on float-vs-delete race

[Ibrahim Hashimov]

ovpn_peer_endpoints_update() reacts to a data-channel "float" (a
peer's packets arriving from a new source transport address) by
first committing the new endpoint under peer->lock, then dropping
peer->lock, and only afterwards re-acquiring peer->ovpn->lock and
peer->lock to rehash the peer into peers->by_transp_addr:

	spin_unlock_bh(&peer->lock);
	ovpn_nl_peer_float_notify(peer, &ss);
	if (peer->ovpn->mode == OVPN_MODE_MP) {
		spin_lock_bh(&peer->ovpn->lock);
		spin_lock_bh(&peer->lock);
		bind = rcu_dereference_protected(peer->bind, ...);
		if (unlikely(!bind)) {
			... return;
		}
		...
		hlist_nulls_del_init_rcu(&peer->hash_entry_transp_addr);
		nhead = ovpn_get_hash_head(peer->ovpn->peers->by_transp_addr, ...);
		hlist_nulls_add_head_rcu(&peer->hash_entry_transp_addr, nhead);
		...
	}

Between the spin_unlock_bh(&peer->lock) and the re-acquire of
peer->ovpn->lock, this thread holds *no* lock on the peer at all. If
an OVPN_CMD_PEER_DEL arrives in that window, ovpn_peer_remove()
(which only requires peer->ovpn->lock) runs to completion: it
unhashes the peer from every table, including by_transp_addr, and
queues it on the release list. peer->bind is only cleared much
later, when the peer is actually released, so the
"if (unlikely(!bind))" check performed after re-acquiring the locks
does *not* observe that the peer has already been removed.
ovpn_peer_endpoints_update() then proceeds to unconditionally re-add
hash_entry_transp_addr, resurrecting the already-removed peer in the
by_transp_addr hash table. Because ovpn_peer_remove() itself guards
against a double remove with
"if (hlist_unhashed(&peer->hash_entry_id)) return;", nothing ever
unhashes the peer a second time. Once the in-flight RX packet that
triggered the float drops its reference and the refcount reaches
zero, the peer is kfree()'d via RCU while still linked in
by_transp_addr. The next matching lookup in
ovpn_peer_get_by_transp_addr() walks that bucket and calls
ovpn_peer_transp_match(), dereferencing the freed peer's ->bind
*before* ovpn_peer_hold() is attempted -- a slab-use-after-free read
on the RX softirq path, runtime-confirmed under KASAN (715
independent "slab-use-after-free in ovpn_peer_get_by_transp_addr"
reports, kmalloc-1k / struct ovpn_peer, freed by the RCU callback,
read from udp_queue_rcv_one_skb -> ovpn_udp_encap_recv ->
ovpn_peer_get_by_transp_addr).

Fix it the same way ovpn_peer_remove() protects itself against a
racing double-remove: after re-acquiring peer->ovpn->lock, check
hlist_unhashed(&peer->hash_entry_id) before touching
hash_entry_transp_addr. ovpn_peer_remove() only mutates the peer's
hashtable membership while holding peer->ovpn->lock, and this check
is performed while we hold that same lock, so the observation is
race-free: either the remove has already happened and hash_entry_id
is unhashed (in which case we must not resurrect the peer and simply
return), or it has not happened yet and cannot happen until we
release peer->ovpn->lock (by which point the rehash under this lock
has already completed). This mirrors the existing double-remove
idiom in ovpn_peer_remove() (drivers/net/ovpn/peer.c) rather than
introducing a new locking primitive.

This is a minimal, targeted fix for the float-vs-delete race; it
does not attempt to shrink the lock-free window itself (peer->lock
is still dropped around ovpn_nl_peer_float_notify()), only to stop
the rehash path from acting on a peer it can no longer safely assume
is still part of the peer tables.

Runtime-verified on a v6.19 KASAN-instrumented kernel: a reproducer
that races authenticated-peer float traffic against a concurrent
OVPN_CMD_PEER_DEL reliably trips a KASAN slab-use-after-free read in
ovpn_peer_get_by_transp_addr() before this fix, and the same
reproducer no longer triggers it once this fix is applied.

Fixes: f0281c1d3732 ("ovpn: add support for updating local or remote UDP endpoint")
Cc: stable@vger.kernel.org
Signed-off-by: Ibrahim Hashimov <security@auditcode.ai>
Assisted-by: AuditCode-AI:2026.07
---
 drivers/net/ovpn/peer.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a09d61296425..aeb69f0b06fa 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -307,6 +307,28 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
 			return;
 		}
 
+		/* Guard against a peer that was concurrently removed (e.g.
+		 * OVPN_CMD_PEER_DEL -> ovpn_peer_remove()) while we held neither
+		 * peer->lock nor ovpn->lock, i.e. in the window opened by the
+		 * spin_unlock_bh(&peer->lock) above. ovpn_peer_remove() only
+		 * unhashes the peer and queues it for release: peer->bind is
+		 * not cleared until the peer is actually released, so the
+		 * !bind check we just did above does not catch this case.
+		 * Blindly re-adding hash_entry_transp_addr below would
+		 * resurrect an already-removed (and soon to be freed) peer in
+		 * the by_transp_addr table, causing a use-after-free the next
+		 * time that table is walked. Reuse the same
+		 * hlist_unhashed(&peer->hash_entry_id) test ovpn_peer_remove()
+		 * itself uses to detect a duplicate removal: ovpn->lock is
+		 * held here too, so this observation is race-free with any
+		 * in-flight or future removal.
+		 */
+		if (unlikely(hlist_unhashed(&peer->hash_entry_id))) {
+			spin_unlock_bh(&peer->lock);
+			spin_unlock_bh(&peer->ovpn->lock);
+			return;
+		}
+
 		/* This function may be invoked concurrently, therefore another
 		 * float may have happened in parallel: perform rehashing
 		 * using the peer->bind->remote directly as key
-- 
2.50.1 (Apple Git-155)