Re: [PATCH ovpn net v5 1/6] ovpn: fix NULL dereference when killing missing key

On Tue, 07 Jul 2026 12:18:08 +0200, Sabrina Dubroca <[email protected]> wrote:
> Hi Ralf,
>
> Sorry, I'm slowly catching up with the recent activity here.
>

Hi Sabrina,

No worries, thanks for your time!

> 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 <[email protected]>
> > ---
> > 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);
> [...]
>

Agreed, this is cleaner. I'll adopt this shape for the next version.

-- 
Ralf Lici
Mandelbit Srl