[PATCH net 0/2] ovpn: fix peer float use-after-free and key slot NULL deref

[PATCH net 0/2] ovpn: fix peer float use-after-free and key slot NULL deref

[Junrui Luo via B4 Relay]

Two independent fixes for the ovpn driver, both in lifetime handling:
one for a crypto key slot that can legitimately be empty, one for a peer
that can be re-linked into a hash table after it has been removed.

Patch 1 fixes a NULL dereference in ovpn_crypto_kill_key(). It locates
the slot holding a given key ID by reading both cs->slots[] entries and
comparing ->key_id, without checking either pointer first. An empty slot
is a normal state: the ordinary rekey sequence - install into the
secondary slot, swap, delete the retired one - leaves primary_idx at 1
with slots[0] empty, and key_id sits at offset 0, so the first
comparison faults on a read of address 0.

Patch 2 fixes a use-after-free in the peer float path.
ovpn_peer_endpoints_update() releases peer->lock to send the float
notification, then re-acquires ovpn->lock and re-links the peer into
by_transp_addr unconditionally - even if ovpn_peer_remove() unlinked it
while the lock was not held. The teardown path never unlinks hash nodes
again, so the peer ends up freed while still on the chain, and every
later datagram walks it in ovpn_peer_get_by_transp_addr(). Reproduced
under KASAN on 7.0-rc3; the splat is included in the patch.

The two patches are independent and touch different files; they can be
applied in either order.

Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
Junrui Luo (2):
      ovpn: don't deref NULL key slot in ovpn_crypto_kill_key()
      ovpn: don't re-hash a removed peer on float

 drivers/net/ovpn/crypto.c | 6 ++++--
 drivers/net/ovpn/peer.c   | 7 ++++++-
 2 files changed, 10 insertions(+), 3 deletions(-)
---
base-commit: 075b74841bd0065a3bda3440873c747938e69b68
change-id: 20260805-ovpn-fixes-52c198458a2a

Best regards,
-- 
Junrui Luo <moonafterrain@outlook.com>

[PATCH net 1/2] ovpn: don't deref NULL key slot in ovpn_crypto_kill_key()

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

ovpn_crypto_kill_key() is reached from ovpn_encrypt_post() when the
packet ID space of the key in use has been exhausted and
ovpn_pktid_xmit_next() returns -ERANGE. It locates the slot holding the
given key ID by reading cs->slots[0] and cs->slots[1] and comparing
->key_id, but it dereferences both pointers without first checking them
for NULL.

An empty key slot is a perfectly normal state. Both
ovpn_crypto_key_slot_delete() and ovpn_crypto_state_release() install
NULL, and the ordinary rekeying sequence - install a new key in the
secondary slot, swap, then delete the retired one - leaves primary_idx
at 1 with slots[0] empty. In that state the very first comparison
dereferences NULL. key_id sits at offset 0 of struct
ovpn_crypto_key_slot, so this faults on a read of address 0.

Every other slot accessor in this file already guards the pointer before
touching it, e.g. ovpn_crypto_key_id_to_slot():

	ks = rcu_dereference(cs->slots[idx]);
	if (ks && ks->key_id == key_id)

Use the same NULL-safe form here.

Fixes: 89d3c0e4612a ("ovpn: kill key and notify userspace in case of IV exhaustion")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/net/ovpn/crypto.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c
index 90580e32052f..2c56fb180ed8 100644
--- a/drivers/net/ovpn/crypto.c
+++ b/drivers/net/ovpn/crypto.c
@@ -60,10 +60,12 @@ bool ovpn_crypto_kill_key(struct ovpn_crypto_state *cs, u8 key_id)
 	struct ovpn_crypto_key_slot *ks = NULL;
 
 	spin_lock_bh(&cs->lock);
-	if (rcu_access_pointer(cs->slots[0])->key_id == key_id) {
+	if (rcu_access_pointer(cs->slots[0]) &&
+	    rcu_access_pointer(cs->slots[0])->key_id == key_id) {
 		ks = rcu_replace_pointer(cs->slots[0], NULL,
 					 lockdep_is_held(&cs->lock));
-	} else if (rcu_access_pointer(cs->slots[1])->key_id == key_id) {
+	} else if (rcu_access_pointer(cs->slots[1]) &&
+		   rcu_access_pointer(cs->slots[1])->key_id == key_id) {
 		ks = rcu_replace_pointer(cs->slots[1], NULL,
 					 lockdep_is_held(&cs->lock));
 	}

-- 
2.51.2

Re: [PATCH net 1/2] ovpn: don't deref NULL key slot in ovpn_crypto_kill_key()

[Antonio Quartulli]

Hi Junrui,

On 05/08/2026 15:29, Junrui Luo via B4 Relay wrote:
> From: Junrui Luo <moonafterrain@outlook.com>
> 
> ovpn_crypto_kill_key() is reached from ovpn_encrypt_post() when the
> packet ID space of the key in use has been exhausted and
> ovpn_pktid_xmit_next() returns -ERANGE. It locates the slot holding the
> given key ID by reading cs->slots[0] and cs->slots[1] and comparing
> ->key_id, but it dereferences both pointers without first checking them
> for NULL.
> 
> An empty key slot is a perfectly normal state. Both
> ovpn_crypto_key_slot_delete() and ovpn_crypto_state_release() install
> NULL, and the ordinary rekeying sequence - install a new key in the
> secondary slot, swap, then delete the retired one - leaves primary_idx
> at 1 with slots[0] empty. In that state the very first comparison
> dereferences NULL. key_id sits at offset 0 of struct
> ovpn_crypto_key_slot, so this faults on a read of address 0.
> 
> Every other slot accessor in this file already guards the pointer before
> touching it, e.g. ovpn_crypto_key_id_to_slot():
> 
> 	ks = rcu_dereference(cs->slots[idx]);
> 	if (ks && ks->key_id == key_id)
> 
> Use the same NULL-safe form here.
> 
> Fixes: 89d3c0e4612a ("ovpn: kill key and notify userspace in case of IV exhaustion")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Assisted-by: Claude:claude-opus-5
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>

A patch fixing this issue was already sent over the openvpn-devel 
mailing list:

https://patchwork.openvpn.net/project/ovpn/patch/da59b0f39ad7eb59174bd1ffdc2b5ab5e2499e08.1785318038.git.ralf@mandelbit.com/

and it will be sent to net soonish.

Feel Free to test/review that patch if you want.

Thanks!



-- 
Antonio Quartulli
OpenVPN Inc.

[PATCH net 2/2] ovpn: don't re-hash a removed peer on float

[Junrui Luo via B4 Relay]

From: Junrui Luo <moonafterrain@outlook.com>

ovpn_peer_endpoints_update() releases peer->lock before sending the
float notification, then re-acquires ovpn->lock and peer->lock to move
the peer to its new bucket in the by_transp_addr table. The only
re-check in that second critical section is for a NULL bind, which
cannot detect removal: bind is cleared by ovpn_peer_release() only
after the refcount drops to zero, and the RX path holds a reference
across the whole float.

Both paths take ovpn->lock, but that only serialises them - it does
not order them:

  CPU0 (RX softirq)              CPU1
  ovpn_peer_endpoints_update()
    spin_unlock_bh(&peer->lock)
    ovpn_nl_peer_float_notify()
                                 ovpn_nl_peer_del_doit()
                                   ovpn_peer_remove()  <- unlinks peer
                                   unlock_ovpn()       <- drops last ref
    spin_lock_bh(&ovpn->lock)
    hlist_nulls_add_head_rcu()   <- removed peer re-linked

ovpn_peer_release_rcu() then frees the peer without unlinking it again,
leaving a dangling node in by_transp_addr that every later datagram
walks in ovpn_peer_get_by_transp_addr():

  BUG: KASAN: slab-use-after-free in ovpn_peer_endpoints_update+0xa5a/0x1010
  Write of size 8 at addr ffff888008576858 by task trigger/78

  Call Trace:
   ovpn_peer_endpoints_update+0xa5a/0x1010
   ovpn_decrypt_post+0x212/0x1040
   ovpn_recv+0x2b6/0x540
   ovpn_udp_encap_recv+0x21c/0x420
   udp_queue_rcv_one_skb+0x1060/0x11a0
   process_backlog+0x451/0x600

  Freed by task 0:
   kfree+0x11a/0x390
   rcu_core+0x7aa/0x1570

  Last potentially related work creation:
   call_rcu+0x82/0x720
   ovpn_peer_release_kref+0x5c/0xd0
   ovpn_nl_peer_del_doit+0x355/0x550

Fix it by extending the existing early return to also bail out when the
peer is no longer hashed by ID. hash_entry_id is unhashed with
hlist_del_init_rcu() by ovpn_peer_remove() under ovpn->lock, which the
float path holds across both the check and the rehash, so a peer that
passes the test cannot be removed before it is re-linked.

Fixes: f0281c1d3732 ("ovpn: add support for updating local or remote UDP endpoint")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Junrui Luo <moonafterrain@outlook.com>
---
 drivers/net/ovpn/peer.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index a21d02ac715e..03886c46ec85 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -302,7 +302,12 @@ void ovpn_peer_endpoints_update(struct ovpn_peer *peer, struct sk_buff *skb)
 		spin_lock_bh(&peer->lock);
 		bind = rcu_dereference_protected(peer->bind,
 						 lockdep_is_held(&peer->lock));
-		if (unlikely(!bind)) {
+		/* peer->lock was released above, therefore the peer may have
+		 * been removed in the meantime: ovpn_peer_remove() unhashes
+		 * hash_entry_id under ovpn->lock. Re-linking a removed peer
+		 * would leave it reachable after it has been freed.
+		 */
+		if (unlikely(!bind || hlist_unhashed(&peer->hash_entry_id))) {
 			spin_unlock_bh(&peer->lock);
 			spin_unlock_bh(&peer->ovpn->lock);
 			return;

-- 
2.51.2

Re: [PATCH net 2/2] ovpn: don't re-hash a removed peer on float

[Antonio Quartulli]

Hi Junrui,

On 05/08/2026 15:29, Junrui Luo via B4 Relay wrote:
> From: Junrui Luo <moonafterrain@outlook.com>
> 
> ovpn_peer_endpoints_update() releases peer->lock before sending the
> float notification, then re-acquires ovpn->lock and peer->lock to move
> the peer to its new bucket in the by_transp_addr table. The only
> re-check in that second critical section is for a NULL bind, which
> cannot detect removal: bind is cleared by ovpn_peer_release() only
> after the refcount drops to zero, and the RX path holds a reference
> across the whole float.
> 
> Both paths take ovpn->lock, but that only serialises them - it does
> not order them:
> 
>    CPU0 (RX softirq)              CPU1
>    ovpn_peer_endpoints_update()
>      spin_unlock_bh(&peer->lock)
>      ovpn_nl_peer_float_notify()
>                                   ovpn_nl_peer_del_doit()
>                                     ovpn_peer_remove()  <- unlinks peer
>                                     unlock_ovpn()       <- drops last ref
>      spin_lock_bh(&ovpn->lock)
>      hlist_nulls_add_head_rcu()   <- removed peer re-linked
> 
> ovpn_peer_release_rcu() then frees the peer without unlinking it again,
> leaving a dangling node in by_transp_addr that every later datagram
> walks in ovpn_peer_get_by_transp_addr():
> 
>    BUG: KASAN: slab-use-after-free in ovpn_peer_endpoints_update+0xa5a/0x1010
>    Write of size 8 at addr ffff888008576858 by task trigger/78
> 
>    Call Trace:
>     ovpn_peer_endpoints_update+0xa5a/0x1010
>     ovpn_decrypt_post+0x212/0x1040
>     ovpn_recv+0x2b6/0x540
>     ovpn_udp_encap_recv+0x21c/0x420
>     udp_queue_rcv_one_skb+0x1060/0x11a0
>     process_backlog+0x451/0x600
> 
>    Freed by task 0:
>     kfree+0x11a/0x390
>     rcu_core+0x7aa/0x1570
> 
>    Last potentially related work creation:
>     call_rcu+0x82/0x720
>     ovpn_peer_release_kref+0x5c/0xd0
>     ovpn_nl_peer_del_doit+0x355/0x550
> 
> Fix it by extending the existing early return to also bail out when the
> peer is no longer hashed by ID. hash_entry_id is unhashed with
> hlist_del_init_rcu() by ovpn_peer_remove() under ovpn->lock, which the
> float path holds across both the check and the rehash, so a peer that
> passes the test cannot be removed before it is re-linked.
> 
> Fixes: f0281c1d3732 ("ovpn: add support for updating local or remote UDP endpoint")
> Reported-by: Yuhao Jiang <danisjiang@gmail.com>
> Assisted-by: Claude:claude-opus-5
> Cc: stable@vger.kernel.org
> Signed-off-by: Junrui Luo <moonafterrain@outlook.com>

A fix for this issue was recently merged to net:

commit 33ec10567fe14456063daf549fdf1a4f53448e4c
Author: Antonio Quartulli <antonio@openvpn.net>
Date:   Tue Jul 28 13:48:47 2026 +0200

     ovpn: skip rehash for peers already removed from by_id

Probably you came up with this fix on an older codebase.

I suggest two things:
1) always check the openvpn-devel mailing list and our patchwork 
instance for already submitted patches that are waiting review (or that 
are queued for sending to net/net-next)
2) make sure to always pull the latest tree patches are based on (net or 
net-next) before sending them over.

In this period of hectic AI works things are moving exceptionally fast 
(well, not always :))

Thanks a lot anyway for looking after our code!

Best Regards,

-- 
Antonio Quartulli
OpenVPN Inc.

Re: [PATCH net 2/2] ovpn: don't re-hash a removed peer on float

[Junrui Luo]

On Thu, Aug 06, 2026 at 11:51:07AM +0200, Antonio Quartulli wrote:
> A fix for this issue was recently merged to net:
> 
> commit 33ec10567fe14456063daf549fdf1a4f53448e4c
> Author: Antonio Quartulli <antonio@openvpn.net>
> Date:   Tue Jul 28 13:48:47 2026 +0200
> 
>     ovpn: skip rehash for peers already removed from by_id
> 
> Probably you came up with this fix on an older codebase.

Hi Antonio,

Thanks for the review and pointers, and sorry for the duplicates, same for
1/2. I based the series on mainline 7.2-rc6, so I missed both.

> I suggest two things:
> 1) always check the openvpn-devel mailing list and our patchwork instance
> for already submitted patches that are waiting review (or that are queued
> for sending to net/net-next)
> 2) make sure to always pull the latest tree patches are based on (net or
> net-next) before sending them over.

Suggestions taken, I'll check them before sending. :)

Thanks,
Junrui Luo