diff mbox series

[Openvpn-devel,net,v5,1/6] ovpn: fix NULL dereference when killing missing key

Message ID b2f5120a3efa20c62a83397d96e949eac6a983df.1783336121.git.ralf@mandelbit.com
State New
Headers
Series ovpn: fix key and workqueue lifetime issues |

Commit Message

Ralf Lici July 6, 2026, 11:33 a.m. UTC 
  ovpn_crypto_kill_key assumes both crypto slots are populated and
dereferences each slot before checking it. That is not guaranteed: a
peer can have only one installed key, and the kill path may be asked to
remove a key that is not present.

Read each slot once while holding the crypto state lock, check for NULL
before looking at key_id, and only replace the slot that actually
matches.

Fixes: 89d3c0e4612a ("ovpn: kill key and notify userspace in case of IV exhaustion")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
Changes since v4 of this series https://lore.kernel.org/openvpn-devel/981d2ea51cca45138210aa52c6e5a0e55c0da7a0.1783099626.git.ralf@mandelbit.com/
- Add this previously posted standalone fix to the series so the whole
  set can be picked in order.
- No changes since v2 of the original single patch
  https://lore.kernel.org/openvpn-devel/19318904cf077d067cd4ec628a22bab03ed7dd29.1782993857.git.ralf@mandelbit.com/

Changes since v1 of the original single patch https://lore.kernel.org/openvpn-devel/9fc33e6f9fae10b9e372a3e06934d697edf5b024.1782829171.git.ralf@mandelbit.com/
- Remove unnecessary braces around single-statement if/else branches.

 drivers/net/ovpn/crypto.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Sabrina Dubroca July 7, 2026, 10:18 a.m. UTC | #1 
Hi Ralf,

Sorry, I'm slowly catching up with the recent activity here.

2026-07-06, 13:33:59 +0200, Ralf Lici wrote:
> ovpn_crypto_kill_key assumes both crypto slots are populated and
> dereferences each slot before checking it. That is not guaranteed: a
> peer can have only one installed key, and the kill path may be asked to
> remove a key that is not present.
> 
> Read each slot once while holding the crypto state lock, check for NULL
> before looking at key_id, and only replace the slot that actually
> matches.
> 
> Fixes: 89d3c0e4612a ("ovpn: kill key and notify userspace in case of IV exhaustion")
> Signed-off-by: Ralf Lici <ralf@mandelbit.com>
> ---
> Changes since v4 of this series https://lore.kernel.org/openvpn-devel/981d2ea51cca45138210aa52c6e5a0e55c0da7a0.1783099626.git.ralf@mandelbit.com/
> - Add this previously posted standalone fix to the series so the whole
>   set can be picked in order.
> - No changes since v2 of the original single patch
>   https://lore.kernel.org/openvpn-devel/19318904cf077d067cd4ec628a22bab03ed7dd29.1782993857.git.ralf@mandelbit.com/
> 
> Changes since v1 of the original single patch https://lore.kernel.org/openvpn-devel/9fc33e6f9fae10b9e372a3e06934d697edf5b024.1782829171.git.ralf@mandelbit.com/
> - Remove unnecessary braces around single-statement if/else branches.
> 
>  drivers/net/ovpn/crypto.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c
> index 90580e32052f..8cb7078a1d93 100644
> --- a/drivers/net/ovpn/crypto.c
> +++ b/drivers/net/ovpn/crypto.c
> @@ -58,15 +58,19 @@ void ovpn_crypto_state_release(struct ovpn_crypto_state *cs)
>  bool ovpn_crypto_kill_key(struct ovpn_crypto_state *cs, u8 key_id)
>  {
>  	struct ovpn_crypto_key_slot *ks = NULL;
> +	struct ovpn_crypto_key_slot *tmp;
>  
>  	spin_lock_bh(&cs->lock);
> -	if (rcu_access_pointer(cs->slots[0])->key_id == key_id) {
> +	tmp = rcu_access_pointer(cs->slots[0]);
> +	if (tmp && tmp->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
> +		tmp = rcu_access_pointer(cs->slots[1]);
> +
> +	if (!ks && tmp && tmp->key_id == key_id)
>  		ks = rcu_replace_pointer(cs->slots[1], NULL,
>  					 lockdep_is_held(&cs->lock));
> -	}

I find the "!ks && tmp" logic really confusing. Maybe something like
this (untested) would be more readable?


	int slot;

	spin_lock_bh(&cs->lock);
	slot = 0;
	tmp = rcu_access_pointer(cs->slots[slot]);
	if (!tmp || tmp->key_id != key_id) {
		slot = 1;
		tmp = rcu_access_pointer(cs->slots[slot]);
	}

	if (tmp && tmp->key_id == key_id)
		ks = rcu_replace_pointer(cs->slots[slot], NULL,
					 lockdep_is_held(&cs->lock));
	spin_unlock_bh(&cs->lock);
[...]
diff mbox series

Patch

diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c
index 90580e32052f..8cb7078a1d93 100644
--- a/drivers/net/ovpn/crypto.c
+++ b/drivers/net/ovpn/crypto.c
@@ -58,15 +58,19 @@  void ovpn_crypto_state_release(struct ovpn_crypto_state *cs)
 bool ovpn_crypto_kill_key(struct ovpn_crypto_state *cs, u8 key_id)
 {
 	struct ovpn_crypto_key_slot *ks = NULL;
+	struct ovpn_crypto_key_slot *tmp;
 
 	spin_lock_bh(&cs->lock);
-	if (rcu_access_pointer(cs->slots[0])->key_id == key_id) {
+	tmp = rcu_access_pointer(cs->slots[0]);
+	if (tmp && tmp->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
+		tmp = rcu_access_pointer(cs->slots[1]);
+
+	if (!ks && tmp && tmp->key_id == key_id)
 		ks = rcu_replace_pointer(cs->slots[1], NULL,
 					 lockdep_is_held(&cs->lock));
-	}
 	spin_unlock_bh(&cs->lock);
 
 	if (ks)