[Openvpn-devel,net,v4,3/3] ovpn: defer key slot crypto freeing to workqueue
Commit Message
Key slots are released through a kref and the existing release path frees
the AEAD transforms from an RCU callback. That is not safe for all crypto
implementations: crypto_free_aead can sleep, for example when an async or
hardware implementation has teardown work to complete.
The AEAD transforms are only used while a key-slot reference is held, so
the final kref put is enough to prove that no transform user remains.
Queue the final crypto teardown on the ovpn workqueue, where sleeping is
allowed, and keep the slot memory itself RCU-freed after the transforms
have been released.
ovpn_crypto_config_get was the remaining lockless reader of transform
state through ovpn_aead_crypto_alg. Make it take a key-slot reference
before reading that state, so the transform lifetime is consistently tied
to key-slot references. The previous patch drains ovpn_wq during module
exit, so queued key-slot teardown work cannot outlive module text.
Fixes: 8534731dbf2d ("ovpn: implement packet processing")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
Changes since v3 https://lore.kernel.org/openvpn-devel/ac2842c8e759849c51447126343376dcc793c7ae.1783080055.git.ralf@mandelbit.com/
- Reuse the module-owned ovpn workqueue introduced by patch 2 instead
of allocating a crypto-specific workqueue for key-slot teardown.
Changes since v2 https://lore.kernel.org/openvpn-devel/b5dfebec718f230783ff47aa354c3b3fa1aa2ed7.1783068961.git.ralf@mandelbit.com/
- Reword the message to note the dependency on patch 1.
Changes since v1 https://lore.kernel.org/openvpn-devel/350f6b48c363dde5a8d0bdf7a1b9fd2abb8b1034.1783057762.git.ralf@mandelbit.com/
- Fix a potential AEAD transform UAF in ovpn_crypto_config_get by
holding a slot kref while reading the cipher algorithm.
drivers/net/ovpn/crypto.c | 15 ++++-----------
drivers/net/ovpn/crypto.h | 3 +++
drivers/net/ovpn/crypto_aead.c | 20 ++++++++++++++------
drivers/net/ovpn/crypto_aead.h | 1 -
4 files changed, 21 insertions(+), 18 deletions(-)
@@ -18,20 +18,12 @@
#include "crypto_aead.h"
#include "crypto.h"
-static void ovpn_ks_destroy_rcu(struct rcu_head *head)
-{
- struct ovpn_crypto_key_slot *ks;
-
- ks = container_of(head, struct ovpn_crypto_key_slot, rcu);
- ovpn_aead_crypto_key_slot_destroy(ks);
-}
-
void ovpn_crypto_key_slot_release(struct kref *kref)
{
struct ovpn_crypto_key_slot *ks;
ks = container_of(kref, struct ovpn_crypto_key_slot, refcount);
- call_rcu(&ks->rcu, ovpn_ks_destroy_rcu);
+ queue_work(ovpn_wq, &ks->free_work);
}
/* can only be invoked when all peer references have been dropped (i.e. RCU
@@ -201,14 +193,15 @@ int ovpn_crypto_config_get(struct ovpn_crypto_state *cs,
rcu_read_lock();
ks = rcu_dereference(cs->slots[idx]);
- if (!ks) {
+ if (!ks || !ovpn_crypto_key_slot_hold(ks)) {
rcu_read_unlock();
return -ENOENT;
}
+ rcu_read_unlock();
keyconf->cipher_alg = ovpn_aead_crypto_alg(ks);
keyconf->key_id = ks->key_id;
- rcu_read_unlock();
+ ovpn_crypto_key_slot_put(ks);
return 0;
}
@@ -10,6 +10,8 @@
#ifndef _NET_OVPN_OVPNCRYPTO_H_
#define _NET_OVPN_OVPNCRYPTO_H_
+#include <linux/workqueue_types.h>
+
#include "pktid.h"
#include "proto.h"
@@ -45,6 +47,7 @@ struct ovpn_crypto_key_slot {
struct ovpn_pktid_recv pid_recv ____cacheline_aligned_in_smp;
struct ovpn_pktid_xmit pid_xmit ____cacheline_aligned_in_smp;
+ struct work_struct free_work;
struct kref refcount;
struct rcu_head rcu;
};
@@ -9,6 +9,7 @@
#include <crypto/aead.h>
#include <linux/skbuff.h>
+#include <linux/workqueue.h>
#include <net/ip.h>
#include <net/ipv6.h>
#include <net/udp.h>
@@ -380,14 +381,19 @@ static struct crypto_aead *ovpn_aead_init(const char *title,
return ERR_PTR(ret);
}
-void ovpn_aead_crypto_key_slot_destroy(struct ovpn_crypto_key_slot *ks)
+static void ovpn_aead_crypto_key_slot_free(struct ovpn_crypto_key_slot *ks)
{
- if (!ks)
- return;
-
crypto_free_aead(ks->encrypt);
crypto_free_aead(ks->decrypt);
- kfree(ks);
+}
+
+static void ovpn_aead_crypto_key_slot_free_work(struct work_struct *work)
+{
+ struct ovpn_crypto_key_slot *ks;
+
+ ks = container_of(work, struct ovpn_crypto_key_slot, free_work);
+ ovpn_aead_crypto_key_slot_free(ks);
+ kfree_rcu(ks, rcu);
}
struct ovpn_crypto_key_slot *
@@ -420,6 +426,7 @@ ovpn_aead_crypto_key_slot_new(const struct ovpn_key_config *kc)
ks->encrypt = NULL;
ks->decrypt = NULL;
+ INIT_WORK(&ks->free_work, ovpn_aead_crypto_key_slot_free_work);
kref_init(&ks->refcount);
ks->key_id = kc->key_id;
@@ -453,7 +460,8 @@ ovpn_aead_crypto_key_slot_new(const struct ovpn_key_config *kc)
return ks;
destroy_ks:
- ovpn_aead_crypto_key_slot_destroy(ks);
+ ovpn_aead_crypto_key_slot_free(ks);
+ kfree(ks);
return ERR_PTR(ret);
}
@@ -22,7 +22,6 @@ int ovpn_aead_decrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
struct ovpn_crypto_key_slot *
ovpn_aead_crypto_key_slot_new(const struct ovpn_key_config *kc);
-void ovpn_aead_crypto_key_slot_destroy(struct ovpn_crypto_key_slot *ks);
enum ovpn_cipher_alg ovpn_aead_crypto_alg(struct ovpn_crypto_key_slot *ks);