[PATCH] random: invalidate crngs and batches on cpuhp teardown

[PATCH] random: invalidate crngs and batches on cpuhp teardown

[Jason A. Donenfeld]

Now that we have a cpuhp teardown notifier, we can invalidate the keys
used by the per-cpu crngs and the batches used by per-cpu batched
entropy, so that if the cpus come back online, and the generation
counter happens to have cycled all the way around to where it was
before, it doesn't mistakenly use the old data. The chances of this
happening are exceedingly rare, but since we now have the notifier
setup, doing this is basically free.

Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Sultan Alsawaf <sultan@kerneltoast.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Theodore Ts'o <tytso@mit.edu>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index df5aef93da34..ce199af9bc56 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1225,6 +1225,15 @@ int random_dead_cpu(unsigned int cpu)
 	 * since the MIX_INFLIGHT flag will be cleared.
 	 */
 	per_cpu_ptr(&irq_randomness, cpu)->count = 0;
+
+	/*
+	 * We also want to invalidate per-cpu crngs and batches,
+	 * so that if the CPU does come back online, it uses
+	 * fresh entropy.
+	 */
+	per_cpu_ptr(&crngs, cpu)->generation = ULONG_MAX;
+	per_cpu_ptr(&batched_entropy_u32, cpu)->position = UINT_MAX;
+	per_cpu_ptr(&batched_entropy_u64, cpu)->position = UINT_MAX;
 	return 0;
 }
 
-- 
2.35.0

Re: [PATCH] random: invalidate crngs and batches on cpuhp teardown

[Jason A. Donenfeld]

Hi Sebastian,

This is the follow up I alluded to in my earlier email. If we get the
cpuhp notifier working correctly, per
<https://lore.kernel.org/lkml/20220214133735.966528-1-Jason@zx2c4.com/>,
then this patch here should be an easy and correct extension.

I was wondering, though, how common this general pattern is, and
whether there's an opportunity at some point down the road for a more
general mechanism. Right now, we have this:

static DEFINE_PER_CPU(struct crng, crngs) = {
    .generation = ULONG_MAX,
    .lock = INIT_LOCAL_LOCK(crngs.lock),
};

What we're running into is that we want the structure to have that
contents immediately after the CPU boots up (which is why this patch
and the previous one sets it during teardown). Maybe other things also
find themselves wanting this too, such that we could have something
called:

static DEFINE_PER_CPU_HPINIT(struct crng, crngs) = {
    .generation = ULONG_MAX,
    .lock = INIT_LOCAL_LOCK(crngs.lock),
};

These would go in a separate linker section, and cpuhp would memcpy
the various sections in at the appropriate time.

Does this seem feasible / desirable? Or is the use case we've found
here actually pretty niche so this wouldn't help with much? Anyhow,
I'm mostly just curious, and intend to move ahead with the approach of
the patch here, but thought I'd mention this.

Jason

Re: [PATCH] random: invalidate crngs and batches on cpuhp teardown

[Sebastian Andrzej Siewior]

On 2022-02-14 14:48:38 [+0100], Jason A. Donenfeld wrote:
> Now that we have a cpuhp teardown notifier, we can invalidate the keys
> used by the per-cpu crngs and the batches used by per-cpu batched
> entropy, so that if the cpus come back online, and the generation
> counter happens to have cycled all the way around to where it was
> before, it doesn't mistakenly use the old data. The chances of this
> happening are exceedingly rare, but since we now have the notifier
> setup, doing this is basically free.

Wasn't aware that random bits get bad over time ;)

> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Sultan Alsawaf <sultan@kerneltoast.com>
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Theodore Ts'o <tytso@mit.edu>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/char/random.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index df5aef93da34..ce199af9bc56 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1225,6 +1225,15 @@ int random_dead_cpu(unsigned int cpu)
>  	 * since the MIX_INFLIGHT flag will be cleared.
>  	 */
>  	per_cpu_ptr(&irq_randomness, cpu)->count = 0;
> +
> +	/*
> +	 * We also want to invalidate per-cpu crngs and batches,
> +	 * so that if the CPU does come back online, it uses
> +	 * fresh entropy.
> +	 */
> +	per_cpu_ptr(&crngs, cpu)->generation = ULONG_MAX;
> +	per_cpu_ptr(&batched_entropy_u32, cpu)->position = UINT_MAX;
> +	per_cpu_ptr(&batched_entropy_u64, cpu)->position = UINT_MAX;

I think if you want to do this, then it would also make sense to put it
into the startup callback. If there is an user doing get_random_u32()
then you would preload the "old" entropy. But on your way "online" you
would preload it with the new entropy.

>  	return 0;
>  }
>  

Sebastian