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.r...@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.r...@mandelbit.com/
> >
> > Changes since v1 of the original single patch
> > https://lore.kernel.org/openvpn-devel/9fc33e6f9fae10b9e372a3e06934d697edf5b024.1782829171.git.r...@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
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
