[PATCH 0/3] random: wire up in-kernel virtual machine fork notifications

[PATCH 0/3] random: wire up in-kernel virtual machine fork notifications

[Jason A. Donenfeld]

As discussed, here is the notifier for learning when a virtual machine
forks, as well as a first use case for it, which is unsurprisingly
WireGuard, since I happen to know that case rather well.

The first patch is a small cleanup discovered when working on the second
patch, which is adding the actual notifier. The third case then
trivially adds it to WireGuard.

Jason A. Donenfeld (3):
  random: replace custom notifier chain with standard one
  random: provide notifier for VM fork
  wireguard: device: clear keys on VM fork

 drivers/char/random.c          | 82 ++++++++++++++--------------------
 drivers/net/wireguard/device.c | 27 +++++------
 include/linux/random.h         | 16 +++----
 lib/random32.c                 | 12 ++---
 lib/vsprintf.c                 | 10 +++--
 5 files changed, 69 insertions(+), 78 deletions(-)

-- 
2.35.1

[PATCH 1/3] random: replace custom notifier chain with standard one

[Jason A. Donenfeld]

We previously rolled our own randomness readiness notifier, which only
has two users in the whole kernel. Replace this with a more standard
atomic notifier block that serves the same purpose with less code. Also
unexport the symbols, because no modules use it, only unconditional
builtins. The only drawback is that it's possible for a notification
handler returning the "stop" code to prevent further processing, but
given that there are only two users, and that we're unexporting this
anyway, that doesn't seem like a significant drawback for the
simplification we receive here.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 67 ++++++++++++------------------------------
 include/linux/random.h | 11 ++-----
 lib/random32.c         | 12 ++++----
 lib/vsprintf.c         | 10 ++++---
 4 files changed, 35 insertions(+), 65 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index fe477a12c1ad..6bd1bbab7392 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -83,8 +83,8 @@ static int crng_init = 0;
 /* Various types of waiters for crng_init->2 transition. */
 static DECLARE_WAIT_QUEUE_HEAD(crng_init_wait);
 static struct fasync_struct *fasync;
-static DEFINE_SPINLOCK(random_ready_list_lock);
-static LIST_HEAD(random_ready_list);
+static DEFINE_SPINLOCK(random_ready_chain_lock);
+static RAW_NOTIFIER_HEAD(random_ready_chain);
 
 /* Control how we warn userspace. */
 static struct ratelimit_state unseeded_warning =
@@ -145,72 +145,43 @@ EXPORT_SYMBOL(wait_for_random_bytes);
  *
  * returns: 0 if callback is successfully added
  *	    -EALREADY if pool is already initialised (callback not called)
- *	    -ENOENT if module for callback is not alive
  */
-int add_random_ready_callback(struct random_ready_callback *rdy)
+int register_random_ready_notifier(struct notifier_block *nb)
 {
-	struct module *owner;
 	unsigned long flags;
-	int err = -EALREADY;
+	int ret = -EALREADY;
 
 	if (crng_ready())
-		return err;
-
-	owner = rdy->owner;
-	if (!try_module_get(owner))
-		return -ENOENT;
-
-	spin_lock_irqsave(&random_ready_list_lock, flags);
-	if (crng_ready())
-		goto out;
-
-	owner = NULL;
-
-	list_add(&rdy->list, &random_ready_list);
-	err = 0;
-
-out:
-	spin_unlock_irqrestore(&random_ready_list_lock, flags);
-
-	module_put(owner);
+		return ret;
 
-	return err;
+	spin_lock_irqsave(&random_ready_chain_lock, flags);
+	if (!crng_ready())
+		ret = raw_notifier_chain_register(&random_ready_chain, nb);
+	spin_unlock_irqrestore(&random_ready_chain_lock, flags);
+	return ret;
 }
-EXPORT_SYMBOL(add_random_ready_callback);
 
 /*
  * Delete a previously registered readiness callback function.
  */
-void del_random_ready_callback(struct random_ready_callback *rdy)
+int unregister_random_ready_notifier(struct notifier_block *nb)
 {
 	unsigned long flags;
-	struct module *owner = NULL;
-
-	spin_lock_irqsave(&random_ready_list_lock, flags);
-	if (!list_empty(&rdy->list)) {
-		list_del_init(&rdy->list);
-		owner = rdy->owner;
-	}
-	spin_unlock_irqrestore(&random_ready_list_lock, flags);
+	int ret;
 
-	module_put(owner);
+	spin_lock_irqsave(&random_ready_chain_lock, flags);
+	ret = raw_notifier_chain_unregister(&random_ready_chain, nb);
+	spin_unlock_irqrestore(&random_ready_chain_lock, flags);
+	return ret;
 }
-EXPORT_SYMBOL(del_random_ready_callback);
 
 static void process_random_ready_list(void)
 {
 	unsigned long flags;
-	struct random_ready_callback *rdy, *tmp;
 
-	spin_lock_irqsave(&random_ready_list_lock, flags);
-	list_for_each_entry_safe(rdy, tmp, &random_ready_list, list) {
-		struct module *owner = rdy->owner;
-
-		list_del_init(&rdy->list);
-		rdy->func(rdy);
-		module_put(owner);
-	}
-	spin_unlock_irqrestore(&random_ready_list_lock, flags);
+	spin_lock_irqsave(&random_ready_chain_lock, flags);
+	raw_notifier_call_chain(&random_ready_chain, 0, NULL);
+	spin_unlock_irqrestore(&random_ready_chain_lock, flags);
 }
 
 #define warn_unseeded_randomness(previous) \
diff --git a/include/linux/random.h b/include/linux/random.h
index f209f1a78899..e84b6fa27435 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -7,15 +7,10 @@
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/once.h>
+#include <linux/notifier.h>
 
 #include <uapi/linux/random.h>
 
-struct random_ready_callback {
-	struct list_head list;
-	void (*func)(struct random_ready_callback *rdy);
-	struct module *owner;
-};
-
 extern void add_device_randomness(const void *, size_t);
 extern void add_bootloader_randomness(const void *, size_t);
 
@@ -42,8 +37,8 @@ extern void get_random_bytes(void *buf, size_t nbytes);
 extern int wait_for_random_bytes(void);
 extern int __init rand_initialize(void);
 extern bool rng_is_initialized(void);
-extern int add_random_ready_callback(struct random_ready_callback *rdy);
-extern void del_random_ready_callback(struct random_ready_callback *rdy);
+extern int register_random_ready_notifier(struct notifier_block *nb);
+extern int unregister_random_ready_notifier(struct notifier_block *nb);
 extern size_t __must_check get_random_bytes_arch(void *buf, size_t nbytes);
 
 #ifndef MODULE
diff --git a/lib/random32.c b/lib/random32.c
index 3c19820796d0..976632003ec6 100644
--- a/lib/random32.c
+++ b/lib/random32.c
@@ -551,9 +551,11 @@ static void prandom_reseed(struct timer_list *unused)
  * To avoid worrying about whether it's safe to delay that interrupt
  * long enough to seed all CPUs, just schedule an immediate timer event.
  */
-static void prandom_timer_start(struct random_ready_callback *unused)
+static int prandom_timer_start(struct notifier_block *nb,
+			       unsigned long action, void *data)
 {
 	mod_timer(&seed_timer, jiffies);
+	return 0;
 }
 
 #ifdef CONFIG_RANDOM32_SELFTEST
@@ -617,13 +619,13 @@ core_initcall(prandom32_state_selftest);
  */
 static int __init prandom_init_late(void)
 {
-	static struct random_ready_callback random_ready = {
-		.func = prandom_timer_start
+	static struct notifier_block random_ready = {
+		.notifier_call = prandom_timer_start
 	};
-	int ret = add_random_ready_callback(&random_ready);
+	int ret = register_random_ready_notifier(&random_ready);
 
 	if (ret == -EALREADY) {
-		prandom_timer_start(&random_ready);
+		prandom_timer_start(&random_ready, 0, NULL);
 		ret = 0;
 	}
 	return ret;
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b8129dd374c..36574a806a81 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -757,14 +757,16 @@ static void enable_ptr_key_workfn(struct work_struct *work)
 
 static DECLARE_WORK(enable_ptr_key_work, enable_ptr_key_workfn);
 
-static void fill_random_ptr_key(struct random_ready_callback *unused)
+static int fill_random_ptr_key(struct notifier_block *nb,
+			       unsigned long action, void *data)
 {
 	/* This may be in an interrupt handler. */
 	queue_work(system_unbound_wq, &enable_ptr_key_work);
+	return 0;
 }
 
-static struct random_ready_callback random_ready = {
-	.func = fill_random_ptr_key
+static struct notifier_block random_ready = {
+	.notifier_call = fill_random_ptr_key
 };
 
 static int __init initialize_ptr_random(void)
@@ -778,7 +780,7 @@ static int __init initialize_ptr_random(void)
 		return 0;
 	}
 
-	ret = add_random_ready_callback(&random_ready);
+	ret = register_random_ready_notifier(&random_ready);
 	if (!ret) {
 		return 0;
 	} else if (ret == -EALREADY) {
-- 
2.35.1

[PATCH 2/3] random: provide notifier for VM fork

[Jason A. Donenfeld]

Drivers such as WireGuard need to learn when VMs fork in order to clear
sessions. This commit provides a simple notifier_block for that, with a
register and unregister function. When no VM fork detection is compiled
in, this turns into a no-op, similar to how the power notifier works.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c  | 15 +++++++++++++++
 include/linux/random.h |  5 +++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 6bd1bbab7392..483fd2dc2057 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1141,6 +1141,8 @@ void add_bootloader_randomness(const void *buf, size_t size)
 EXPORT_SYMBOL_GPL(add_bootloader_randomness);
 
 #if IS_ENABLED(CONFIG_VMGENID)
+static BLOCKING_NOTIFIER_HEAD(vmfork_notifier);
+
 /*
  * Handle a new unique VM ID, which is unique, not secret, so we
  * don't credit it, but we do immediately force a reseed after so
@@ -1152,11 +1154,24 @@ void add_vmfork_randomness(const void *unique_vm_id, size_t size)
 	if (crng_ready()) {
 		crng_reseed(true);
 		pr_notice("crng reseeded due to virtual machine fork\n");
+		blocking_notifier_call_chain(&vmfork_notifier, 0, NULL);
 	}
 }
 #if IS_MODULE(CONFIG_VMGENID)
 EXPORT_SYMBOL_GPL(add_vmfork_randomness);
 #endif
+
+int register_random_vmfork_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&vmfork_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(register_random_vmfork_notifier);
+
+int unregister_random_vmfork_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&vmfork_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(unregister_random_vmfork_notifier);
 #endif
 
 struct fast_pool {
diff --git a/include/linux/random.h b/include/linux/random.h
index e84b6fa27435..7fccbc7e5a75 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -31,6 +31,11 @@ extern void add_hwgenerator_randomness(const void *buffer, size_t count,
 				       size_t entropy);
 #if IS_ENABLED(CONFIG_VMGENID)
 extern void add_vmfork_randomness(const void *unique_vm_id, size_t size);
+extern int register_random_vmfork_notifier(struct notifier_block *nb);
+extern int unregister_random_vmfork_notifier(struct notifier_block *nb);
+#else
+static inline int register_random_vmfork_notifier(struct notifier_block *nb) { return 0; }
+static inline int unregister_random_vmfork_notifier(struct notifier_block *nb) { return 0; }
 #endif
 
 extern void get_random_bytes(void *buf, size_t nbytes);
-- 
2.35.1

[PATCH 3/3] wireguard: device: clear keys on VM fork

[Jason A. Donenfeld]

When a virtual machine forks, it's important that WireGuard clear
existing sessions so that different plaintext is not transmitted using
the same key+nonce, which can result in catastrophic cryptographic
failure. To accomplish this, we simply hook into the newly added vmfork
notifier, which can use the same notification function we're already
using for PM notifications.

As a bonus, it turns out that, like the vmfork registration function,
the PM registration function is stubbed out when CONFIG_PM_SLEEP is not
set, so we can actually just remove the maze of ifdefs, which makes it
really quite clean to support both notifiers at once.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Hi Jakub,

I wasn't planning on sending other WireGuard changes to net-next this
cycle, and this one here depends on previous things in my random.git
tree. Is it okay with you if I take this through my tree rather than
net-next? Alternatively, I could send it through net after rc1 if you'd
prefer that. Or we could just wait for 5.19, but that seems a long way's
off.

Thanks,
Jason

 drivers/net/wireguard/device.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index a46067c38bf5..22cc27c221f8 100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -59,7 +59,10 @@ static int wg_open(struct net_device *dev)
 	return ret;
 }
 
-#ifdef CONFIG_PM_SLEEP
+static int wg_pm_notification(struct notifier_block *nb, unsigned long action, void *data);
+static struct notifier_block pm_notifier = { .notifier_call = wg_pm_notification };
+static struct notifier_block vm_notifier = { .notifier_call = wg_pm_notification };
+
 static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
 			      void *data)
 {
@@ -70,10 +73,10 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
 	 * its normal operation rather than as a somewhat rare event, then we
 	 * don't actually want to clear keys.
 	 */
-	if (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_ANDROID))
+	if (nb == &pm_notifier && (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_ANDROID)))
 		return 0;
 
-	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
+	if (nb == &pm_notifier && action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
 		return 0;
 
 	rtnl_lock();
@@ -91,9 +94,6 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
 	return 0;
 }
 
-static struct notifier_block pm_notifier = { .notifier_call = wg_pm_notification };
-#endif
-
 static int wg_stop(struct net_device *dev)
 {
 	struct wg_device *wg = netdev_priv(dev);
@@ -424,16 +424,18 @@ int __init wg_device_init(void)
 {
 	int ret;
 
-#ifdef CONFIG_PM_SLEEP
 	ret = register_pm_notifier(&pm_notifier);
 	if (ret)
 		return ret;
-#endif
 
-	ret = register_pernet_device(&pernet_ops);
+	ret = register_random_vmfork_notifier(&vm_notifier);
 	if (ret)
 		goto error_pm;
 
+	ret = register_pernet_device(&pernet_ops);
+	if (ret)
+		goto error_vm;
+
 	ret = rtnl_link_register(&link_ops);
 	if (ret)
 		goto error_pernet;
@@ -442,10 +444,10 @@ int __init wg_device_init(void)
 
 error_pernet:
 	unregister_pernet_device(&pernet_ops);
+error_vm:
+	unregister_random_vmfork_notifier(&vm_notifier);
 error_pm:
-#ifdef CONFIG_PM_SLEEP
 	unregister_pm_notifier(&pm_notifier);
-#endif
 	return ret;
 }
 
@@ -453,8 +455,7 @@ void wg_device_uninit(void)
 {
 	rtnl_link_unregister(&link_ops);
 	unregister_pernet_device(&pernet_ops);
-#ifdef CONFIG_PM_SLEEP
+	unregister_random_vmfork_notifier(&vm_notifier);
 	unregister_pm_notifier(&pm_notifier);
-#endif
 	rcu_barrier();
 }
-- 
2.35.1

Re: [PATCH 3/3] wireguard: device: clear keys on VM fork

[Jakub Kicinski]

On Wed,  2 Mar 2022 00:10:38 +0100 Jason A. Donenfeld wrote:
> I wasn't planning on sending other WireGuard changes to net-next this
> cycle, and this one here depends on previous things in my random.git
> tree. Is it okay with you if I take this through my tree rather than
> net-next?

Yup,

Acked-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH 1/3] random: replace custom notifier chain with standard one

[Dominik Brodowski]

Am Wed, Mar 02, 2022 at 12:10:36AM +0100 schrieb Jason A. Donenfeld:
>  /*
>   * Delete a previously registered readiness callback function.
>   */
> -void del_random_ready_callback(struct random_ready_callback *rdy)
> +int unregister_random_ready_notifier(struct notifier_block *nb)
>  {
>  	unsigned long flags;
> -	struct module *owner = NULL;
> -
> -	spin_lock_irqsave(&random_ready_list_lock, flags);
> -	if (!list_empty(&rdy->list)) {
> -		list_del_init(&rdy->list);
> -		owner = rdy->owner;
> -	}
> -	spin_unlock_irqrestore(&random_ready_list_lock, flags);
> +	int ret;
>  
> -	module_put(owner);
> +	spin_lock_irqsave(&random_ready_chain_lock, flags);
> +	ret = raw_notifier_chain_unregister(&random_ready_chain, nb);
> +	spin_unlock_irqrestore(&random_ready_chain_lock, flags);
> +	return ret;
>  }
> -EXPORT_SYMBOL(del_random_ready_callback);

That doesn't seem to be used anywhere, so I'd suggest removing this function
altogether.

Otherwise:
	Reviewed-by: Dominik Brodowski <linux@dominikbrodowski.net>

Re: [PATCH 3/3] wireguard: device: clear keys on VM fork

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 12:10:38AM +0100, Jason A. Donenfeld wrote:
> When a virtual machine forks, it's important that WireGuard clear
> existing sessions so that different plaintext is not transmitted using
> the same key+nonce, which can result in catastrophic cryptographic
> failure. To accomplish this, we simply hook into the newly added vmfork
> notifier, which can use the same notification function we're already
> using for PM notifications.
> 
> As a bonus, it turns out that, like the vmfork registration function,
> the PM registration function is stubbed out when CONFIG_PM_SLEEP is not
> set, so we can actually just remove the maze of ifdefs, which makes it
> really quite clean to support both notifiers at once.
> 
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Catastrophic cryptographic failure sounds bad :(
So in another thread we discussed that there's a race with this
approach, and we don't know how big it is. Question is how expensive
it would be to fix it properly checking for fork after every use of
key+nonce and before transmitting it. I did a quick microbenchmark
and it did not seem too bad - care posting some numbers?

> ---
> Hi Jakub,
> 
> I wasn't planning on sending other WireGuard changes to net-next this
> cycle, and this one here depends on previous things in my random.git
> tree. Is it okay with you if I take this through my tree rather than
> net-next? Alternatively, I could send it through net after rc1 if you'd
> prefer that. Or we could just wait for 5.19, but that seems a long way's
> off.
> 
> Thanks,
> Jason
> 
>  drivers/net/wireguard/device.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
> index a46067c38bf5..22cc27c221f8 100644
> --- a/drivers/net/wireguard/device.c
> +++ b/drivers/net/wireguard/device.c
> @@ -59,7 +59,10 @@ static int wg_open(struct net_device *dev)
>  	return ret;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> +static int wg_pm_notification(struct notifier_block *nb, unsigned long action, void *data);
> +static struct notifier_block pm_notifier = { .notifier_call = wg_pm_notification };
> +static struct notifier_block vm_notifier = { .notifier_call = wg_pm_notification };
> +
>  static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
>  			      void *data)
>  {
> @@ -70,10 +73,10 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
>  	 * its normal operation rather than as a somewhat rare event, then we
>  	 * don't actually want to clear keys.
>  	 */
> -	if (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_ANDROID))
> +	if (nb == &pm_notifier && (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_ANDROID)))
>  		return 0;
>  
> -	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
> +	if (nb == &pm_notifier && action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
>  		return 0;
>  
>  	rtnl_lock();
> @@ -91,9 +94,6 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
>  	return 0;
>  }
>  
> -static struct notifier_block pm_notifier = { .notifier_call = wg_pm_notification };
> -#endif
> -
>  static int wg_stop(struct net_device *dev)
>  {
>  	struct wg_device *wg = netdev_priv(dev);
> @@ -424,16 +424,18 @@ int __init wg_device_init(void)
>  {
>  	int ret;
>  
> -#ifdef CONFIG_PM_SLEEP
>  	ret = register_pm_notifier(&pm_notifier);
>  	if (ret)
>  		return ret;
> -#endif
>  
> -	ret = register_pernet_device(&pernet_ops);
> +	ret = register_random_vmfork_notifier(&vm_notifier);
>  	if (ret)
>  		goto error_pm;
>  
> +	ret = register_pernet_device(&pernet_ops);
> +	if (ret)
> +		goto error_vm;
> +
>  	ret = rtnl_link_register(&link_ops);
>  	if (ret)
>  		goto error_pernet;
> @@ -442,10 +444,10 @@ int __init wg_device_init(void)
>  
>  error_pernet:
>  	unregister_pernet_device(&pernet_ops);
> +error_vm:
> +	unregister_random_vmfork_notifier(&vm_notifier);
>  error_pm:
> -#ifdef CONFIG_PM_SLEEP
>  	unregister_pm_notifier(&pm_notifier);
> -#endif
>  	return ret;
>  }
>  
> @@ -453,8 +455,7 @@ void wg_device_uninit(void)
>  {
>  	rtnl_link_unregister(&link_ops);
>  	unregister_pernet_device(&pernet_ops);
> -#ifdef CONFIG_PM_SLEEP
> +	unregister_random_vmfork_notifier(&vm_notifier);
>  	unregister_pm_notifier(&pm_notifier);
> -#endif
>  	rcu_barrier();
>  }
> -- 
> 2.35.1
> 
> 

Re: [PATCH 2/3] random: provide notifier for VM fork

[Greg KH]

On Wed, Mar 02, 2022 at 12:10:37AM +0100, Jason A. Donenfeld wrote:
> Drivers such as WireGuard need to learn when VMs fork in order to clear
> sessions. This commit provides a simple notifier_block for that, with a
> register and unregister function. When no VM fork detection is compiled
> in, this turns into a no-op, similar to how the power notifier works.
> 
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/char/random.c  | 15 +++++++++++++++
>  include/linux/random.h |  5 +++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 6bd1bbab7392..483fd2dc2057 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1141,6 +1141,8 @@ void add_bootloader_randomness(const void *buf, size_t size)
>  EXPORT_SYMBOL_GPL(add_bootloader_randomness);
>  
>  #if IS_ENABLED(CONFIG_VMGENID)
> +static BLOCKING_NOTIFIER_HEAD(vmfork_notifier);
> +
>  /*
>   * Handle a new unique VM ID, which is unique, not secret, so we
>   * don't credit it, but we do immediately force a reseed after so
> @@ -1152,11 +1154,24 @@ void add_vmfork_randomness(const void *unique_vm_id, size_t size)
>  	if (crng_ready()) {
>  		crng_reseed(true);
>  		pr_notice("crng reseeded due to virtual machine fork\n");
> +		blocking_notifier_call_chain(&vmfork_notifier, 0, NULL);
>  	}
>  }
>  #if IS_MODULE(CONFIG_VMGENID)
>  EXPORT_SYMBOL_GPL(add_vmfork_randomness);
>  #endif
> +
> +int register_random_vmfork_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&vmfork_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(register_random_vmfork_notifier);
> +
> +int unregister_random_vmfork_notifier(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&vmfork_notifier, nb);
> +}
> +EXPORT_SYMBOL_GPL(unregister_random_vmfork_notifier);
>  #endif
>  
>  struct fast_pool {
> diff --git a/include/linux/random.h b/include/linux/random.h
> index e84b6fa27435..7fccbc7e5a75 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -31,6 +31,11 @@ extern void add_hwgenerator_randomness(const void *buffer, size_t count,
>  				       size_t entropy);
>  #if IS_ENABLED(CONFIG_VMGENID)
>  extern void add_vmfork_randomness(const void *unique_vm_id, size_t size);
> +extern int register_random_vmfork_notifier(struct notifier_block *nb);
> +extern int unregister_random_vmfork_notifier(struct notifier_block *nb);
> +#else
> +static inline int register_random_vmfork_notifier(struct notifier_block *nb) { return 0; }
> +static inline int unregister_random_vmfork_notifier(struct notifier_block *nb) { return 0; }
>  #endif
>  
>  extern void get_random_bytes(void *buf, size_t nbytes);
> -- 
> 2.35.1
> 

It seems crazy that the "we just were spawned as a new vm" notifier is
based in the random driver, but sure, put it here for now!  :)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/3] random: provide notifier for VM fork

[Jason A. Donenfeld]

Hi Greg,

On Wed, Mar 2, 2022 at 9:53 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> It seems crazy that the "we just were spawned as a new vm" notifier is
> based in the random driver, but sure, put it here for now!  :)

I was thinking you might say this. I see it both ways, but I think I'm
more inclined to doing it this way, at least for now. Here's how it
breaks down:

VM forking is usually an okay thing to do because computers are
deterministic. Usually. Where is there non-determinism in a place that
it matters? The RNG is supposed to be "the" place of non-determinism.
If anything is going to happen in response to a VM fork, it's going to
necessarily be _after_ the RNG becomes sufficiently non-deterministic
again, and so it's the RNG who announces, "hey I'm safe to use again,
and please read from me again if you're doing non-misuse resistant
crypto." It's the proper place to announce that.

On the other hand, I think you could argue that really this should
come from vmgenid itself, with the caveat that the notifier is called
after add_vmfork_randomness is called. For now that would exist in
vmgenid.o itself, and then if we ever have multiple drivers notifying,
some shared infrastructure could be made. Except vmgenid.o might be
vmgenid.ko, and then the whole problem gets kind of annoying and maybe
we actually want that shared infrastructure _now_ instead. And now we
find ourselves complicating everything with additional Kbuild symbols
and header files and stubs. It just seems like the road of more pain.

Anyway, even if we go with the first solution -- keeping it in
random.o -- now, I wouldn't be opposed to revisiting that decision
later if the landscape becomes more complex. Luckily this is just the
kernel side of things and not userspace, so we can easily change
things down the road.

Jason

Re: [PATCH 1/3] random: replace custom notifier chain with standard one

[Jason A. Donenfeld]

Hi Dominik,

On Wed, Mar 2, 2022 at 6:35 AM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Am Wed, Mar 02, 2022 at 12:10:36AM +0100 schrieb Jason A. Donenfeld:
> >  /*
> >   * Delete a previously registered readiness callback function.
> >   */
> > -void del_random_ready_callback(struct random_ready_callback *rdy)
> > +int unregister_random_ready_notifier(struct notifier_block *nb)
> >  {
> >       unsigned long flags;
> > -     struct module *owner = NULL;
> > -
> > -     spin_lock_irqsave(&random_ready_list_lock, flags);
> > -     if (!list_empty(&rdy->list)) {
> > -             list_del_init(&rdy->list);
> > -             owner = rdy->owner;
> > -     }
> > -     spin_unlock_irqrestore(&random_ready_list_lock, flags);
> > +     int ret;
> >
> > -     module_put(owner);
> > +     spin_lock_irqsave(&random_ready_chain_lock, flags);
> > +     ret = raw_notifier_chain_unregister(&random_ready_chain, nb);
> > +     spin_unlock_irqrestore(&random_ready_chain_lock, flags);
> > +     return ret;
> >  }
> > -EXPORT_SYMBOL(del_random_ready_callback);
>
> That doesn't seem to be used anywhere, so I'd suggest removing this function
> altogether.

I thought about this, but it feels weird to have a registration
function without an unregistration function... No other notifier is
unbalanced like that.

Jason

Re: [PATCH 3/3] wireguard: device: clear keys on VM fork

[Jason A. Donenfeld]

Hi Michael,

On Wed, Mar 2, 2022 at 9:36 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> Catastrophic cryptographic failure sounds bad :(
> So in another thread we discussed that there's a race with this
> approach, and we don't know how big it is. Question is how expensive
> it would be to fix it properly checking for fork after every use of
> key+nonce and before transmitting it. I did a quick microbenchmark
> and it did not seem too bad - care posting some numbers?

I followed up in that thread, which is a larger one, so it might be
easiest to keep discussion there. My response to you here is the same
as it was over there. :)

https://lore.kernel.org/lkml/CAHmME9pf-bjnZuweoLqoFEmPy1OK7ogEgGEAva1T8uVTufhCuw@mail.gmail.com/

Jason

Re: [PATCH 3/3] wireguard: device: clear keys on VM fork

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 12:44:45PM +0100, Jason A. Donenfeld wrote:
> Hi Michael,
> 
> On Wed, Mar 2, 2022 at 9:36 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > Catastrophic cryptographic failure sounds bad :(
> > So in another thread we discussed that there's a race with this
> > approach, and we don't know how big it is. Question is how expensive
> > it would be to fix it properly checking for fork after every use of
> > key+nonce and before transmitting it. I did a quick microbenchmark
> > and it did not seem too bad - care posting some numbers?
> 
> I followed up in that thread, which is a larger one, so it might be
> easiest to keep discussion there. My response to you here is the same
> as it was over there. :)
> 
> https://lore.kernel.org/lkml/CAHmME9pf-bjnZuweoLqoFEmPy1OK7ogEgGEAva1T8uVTufhCuw@mail.gmail.com/
> 
> Jason

Okay. The reason to respond here was since this is the user of the
interface. Maybe unite the patchsets?

Thanks,

-- 
MST

Re: [PATCH 1/3] random: replace custom notifier chain with standard one

[Dominik Brodowski]

Hi Jason,

Am Wed, Mar 02, 2022 at 12:42:56PM +0100 schrieb Jason A. Donenfeld:
> On Wed, Mar 2, 2022 at 6:35 AM Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> >
> > Am Wed, Mar 02, 2022 at 12:10:36AM +0100 schrieb Jason A. Donenfeld:
> > >  /*
> > >   * Delete a previously registered readiness callback function.
> > >   */
> > > -void del_random_ready_callback(struct random_ready_callback *rdy)
> > > +int unregister_random_ready_notifier(struct notifier_block *nb)
> > >  {
> > >       unsigned long flags;
> > > -     struct module *owner = NULL;
> > > -
> > > -     spin_lock_irqsave(&random_ready_list_lock, flags);
> > > -     if (!list_empty(&rdy->list)) {
> > > -             list_del_init(&rdy->list);
> > > -             owner = rdy->owner;
> > > -     }
> > > -     spin_unlock_irqrestore(&random_ready_list_lock, flags);
> > > +     int ret;
> > >
> > > -     module_put(owner);
> > > +     spin_lock_irqsave(&random_ready_chain_lock, flags);
> > > +     ret = raw_notifier_chain_unregister(&random_ready_chain, nb);
> > > +     spin_unlock_irqrestore(&random_ready_chain_lock, flags);
> > > +     return ret;
> > >  }
> > > -EXPORT_SYMBOL(del_random_ready_callback);
> >
> > That doesn't seem to be used anywhere, so I'd suggest removing this function
> > altogether.
> 
> I thought about this, but it feels weird to have a registration
> function without an unregistration function... No other notifier is
> unbalanced like that.

... but having unused code compiled in (unless LTO is enabled, of course)
seems worse. Maybe comment it out, #ifdef COMPILE_TEST or something like
that?

Thanks,
	Dominik

[PATCH v2] wireguard: device: clear keys on VM fork

[Jason A. Donenfeld]

When a virtual machine forks, it's important that WireGuard clear
existing sessions so that different plaintexts are not transmitted using
the same key+nonce, which can result in catastrophic cryptographic
failure. To accomplish this, we simply hook into the newly added vmfork
notifier.

As a bonus, it turns out that, like the vmfork registration function,
the PM registration function is stubbed out when CONFIG_PM_SLEEP is not
set, so we can actually just remove the maze of ifdefs, which makes it
really quite clean to support both notifiers at once.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Theodore Ts'o <tytso@mit.edu>
Acked-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Fundamentally the same as v1, but just refactored into its own function
to be less hacky. Meant to post this 10 days ago as it's been sitting in
my tree being tested like this for a while now; this is what I've got
queued up for 5.18, so posting to the list for completeness.

 drivers/net/wireguard/device.c | 38 ++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c
index a46067c38bf5..0fad1331303c 100644
--- a/drivers/net/wireguard/device.c
+++ b/drivers/net/wireguard/device.c
@@ -59,9 +59,7 @@ static int wg_open(struct net_device *dev)
 	return ret;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
-			      void *data)
+static int wg_pm_notification(struct notifier_block *nb, unsigned long action, void *data)
 {
 	struct wg_device *wg;
 	struct wg_peer *peer;
@@ -92,7 +90,24 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action,
 }
 
 static struct notifier_block pm_notifier = { .notifier_call = wg_pm_notification };
-#endif
+
+static int wg_vm_notification(struct notifier_block *nb, unsigned long action, void *data)
+{
+	struct wg_device *wg;
+	struct wg_peer *peer;
+
+	rtnl_lock();
+	list_for_each_entry(wg, &device_list, device_list) {
+		mutex_lock(&wg->device_update_lock);
+		list_for_each_entry(peer, &wg->peer_list, peer_list)
+			wg_noise_expire_current_peer_keypairs(peer);
+		mutex_unlock(&wg->device_update_lock);
+	}
+	rtnl_unlock();
+	return 0;
+}
+
+static struct notifier_block vm_notifier = { .notifier_call = wg_vm_notification };
 
 static int wg_stop(struct net_device *dev)
 {
@@ -424,16 +439,18 @@ int __init wg_device_init(void)
 {
 	int ret;
 
-#ifdef CONFIG_PM_SLEEP
 	ret = register_pm_notifier(&pm_notifier);
 	if (ret)
 		return ret;
-#endif
 
-	ret = register_pernet_device(&pernet_ops);
+	ret = register_random_vmfork_notifier(&vm_notifier);
 	if (ret)
 		goto error_pm;
 
+	ret = register_pernet_device(&pernet_ops);
+	if (ret)
+		goto error_vm;
+
 	ret = rtnl_link_register(&link_ops);
 	if (ret)
 		goto error_pernet;
@@ -442,10 +459,10 @@ int __init wg_device_init(void)
 
 error_pernet:
 	unregister_pernet_device(&pernet_ops);
+error_vm:
+	unregister_random_vmfork_notifier(&vm_notifier);
 error_pm:
-#ifdef CONFIG_PM_SLEEP
 	unregister_pm_notifier(&pm_notifier);
-#endif
 	return ret;
 }
 
@@ -453,8 +470,7 @@ void wg_device_uninit(void)
 {
 	rtnl_link_unregister(&link_ops);
 	unregister_pernet_device(&pernet_ops);
-#ifdef CONFIG_PM_SLEEP
+	unregister_random_vmfork_notifier(&vm_notifier);
 	unregister_pm_notifier(&pm_notifier);
-#endif
 	rcu_barrier();
 }
-- 
2.35.1