[Openvpn-devel,net,v2,2/2] ovpn: defer key slot crypto freeing to workqueue
Commit Message
ovpn_crypto_key_slot_release currently schedules an RCU callback to
destroy the key slot. That callback calls into AEAD teardown and frees
the crypto transforms.
RCU callbacks run in atomic context, but crypto transform teardown is
not guaranteed to be atomic-safe. Some AEAD driver exit paths may wait
for hardware or take sleepable locks, so calling them from the RCU
callback can trigger sleeping-in-atomic bugs.
Queue key slot destruction on an ovpn-owned workqueue when the slot
refcount reaches zero. A system workqueue would not be drained by ovpn
module exit, so use a module-owned workqueue that is destroyed before
the final rcu_barrier.
This relies on the previous crypto-completion fix: callbacks now queue
key-slot release before dropping the peer reference that can unblock
module teardown.
ovpn_crypto_config_get was the only lockless path that queried the
transform without holding the slot. Take a temporary slot reference
there before reading the algorithm, preserving the invariant that AEAD
transforms are only reachable by key-slot kref holders.
Bypassing call_rcu for crypto teardown is safe because the AEAD
transforms are only used by key-slot kref holders. Once the kref
reaches zero, no transform user remains. The slot memory still needs
RCU protection because lockless readers may have observed the slot
before they failed to acquire the kref, so the worker releases the
transforms from process context and then frees the slot with kfree_rcu.
Fixes: 8534731dbf2d ("ovpn: implement packet processing")
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
---
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 | 25 ++++++++++++++++++-------
drivers/net/ovpn/crypto.h | 7 +++++++
drivers/net/ovpn/crypto_aead.c | 20 ++++++++++++++------
drivers/net/ovpn/crypto_aead.h | 1 -
drivers/net/ovpn/main.c | 15 +++++++++++++--
5 files changed, 52 insertions(+), 16 deletions(-)
@@ -10,6 +10,7 @@
#include <linux/types.h>
#include <linux/net.h>
#include <linux/netdevice.h>
+#include <linux/workqueue.h>
#include <uapi/linux/ovpn.h>
#include "ovpnpriv.h"
@@ -18,12 +19,21 @@
#include "crypto_aead.h"
#include "crypto.h"
-static void ovpn_ks_destroy_rcu(struct rcu_head *head)
+static struct workqueue_struct *ovpn_wq;
+
+int ovpn_crypto_workqueue_init(void)
{
- struct ovpn_crypto_key_slot *ks;
+ ovpn_wq = alloc_workqueue("ovpn", 0, 0);
+ if (!ovpn_wq)
+ return -ENOMEM;
+
+ return 0;
+}
- ks = container_of(head, struct ovpn_crypto_key_slot, rcu);
- ovpn_aead_crypto_key_slot_destroy(ks);
+void ovpn_crypto_workqueue_destroy(void)
+{
+ destroy_workqueue(ovpn_wq);
+ ovpn_wq = NULL;
}
void ovpn_crypto_key_slot_release(struct kref *kref)
@@ -31,7 +41,7 @@ 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 +211,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;
};
@@ -121,6 +124,10 @@ ovpn_crypto_key_slot_primary(const struct ovpn_crypto_state *cs)
void ovpn_crypto_key_slot_release(struct kref *kref);
+int ovpn_crypto_workqueue_init(void);
+
+void ovpn_crypto_workqueue_destroy(void);
+
static inline void ovpn_crypto_key_slot_put(struct ovpn_crypto_key_slot *ks)
{
kref_put(&ks->refcount, ovpn_crypto_key_slot_release);
@@ -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);
@@ -18,6 +18,7 @@
#include <uapi/linux/if_arp.h>
#include "ovpnpriv.h"
+#include "crypto.h"
#include "main.h"
#include "netlink.h"
#include "io.h"
@@ -233,13 +234,20 @@ static struct rtnl_link_ops ovpn_link_ops = {
static int __init ovpn_init(void)
{
- int err = rtnl_link_register(&ovpn_link_ops);
+ int err;
+ err = ovpn_crypto_workqueue_init();
if (err) {
- pr_err("ovpn: can't register rtnl link ops: %d\n", err);
+ pr_err("ovpn: can't allocate workqueue: %d\n", err);
return err;
}
+ err = rtnl_link_register(&ovpn_link_ops);
+ if (err) {
+ pr_err("ovpn: can't register rtnl link ops: %d\n", err);
+ goto destroy_wq;
+ }
+
err = ovpn_nl_register();
if (err) {
pr_err("ovpn: can't register netlink family: %d\n", err);
@@ -252,6 +260,8 @@ static int __init ovpn_init(void)
unreg_rtnl:
rtnl_link_unregister(&ovpn_link_ops);
+destroy_wq:
+ ovpn_crypto_workqueue_destroy();
return err;
}
@@ -259,6 +269,7 @@ static __exit void ovpn_cleanup(void)
{
ovpn_nl_unregister();
rtnl_link_unregister(&ovpn_link_ops);
+ ovpn_crypto_workqueue_destroy();
rcu_barrier();
}