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 <[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);
[...]
--
Sabrina
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
