Re: [PATCH RFC v1 1/2] random: add mechanism for VM forks to reinitialize crng

[Eric Biggers]

On Wed, Feb 23, 2022 at 02:12:30PM +0100, Jason A. Donenfeld wrote:
> When a VM forks, we must immediately mix in additional information to
> the stream of random output so that two forks or a rollback don't
> produce the same stream of random numbers, which could have catastrophic
> cryptographic consequences. This commit adds a simple API, add_vmfork_
> randomness(), for that.
> 
> Cc: Theodore Ts'o <tytso@mit.edu>
> Cc: Jann Horn <jannh@google.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/char/random.c  | 58 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/random.h |  1 +
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 536237a0f073..29d6ce484d15 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -344,6 +344,46 @@ static void crng_reseed(void)
>       }
>  }
>  
> +/*
> + * This mixes unique_vm_id directly into the base_crng key as soon as
> + * possible, similarly to crng_pre_init_inject(), even if the crng is
> + * already running, in order to immediately branch streams from prior
> + * VM instances.
> + */
> +static void crng_vm_fork_inject(const void *unique_vm_id, size_t len)
> +{
> +     unsigned long flags, next_gen;
> +     struct blake2s_state hash;
> +
> +     /*
> +      * Unlike crng_reseed(), we take the lock as early as possible,
> +      * since we don't want the RNG to be used until it's updated.
> +      */
> +     spin_lock_irqsave(&base_crng.lock, flags);
> +
> +     /*
> +      * Also update the generation, while locked, as early as
> +      * possible. This will mean unlocked reads of the generation
> +      * will cause a reseeding of per-cpu crngs, and those will
> +      * spin on the base_crng lock waiting for the rest of this
> +      * operation to complete, which achieves the goal of blocking
> +      * the production of new output until this is done.
> +      */
> +     next_gen = base_crng.generation + 1;
> +     if (next_gen == ULONG_MAX)
> +             ++next_gen;
> +     WRITE_ONCE(base_crng.generation, next_gen);
> +     WRITE_ONCE(base_crng.birth, jiffies);
> +
> +     /* This is the same formulation used by crng_pre_init_inject(). */
> +     blake2s_init(&hash, sizeof(base_crng.key));
> +     blake2s_update(&hash, base_crng.key, sizeof(base_crng.key));
> +     blake2s_update(&hash, unique_vm_id, len);
> +     blake2s_final(&hash, base_crng.key);
> +
> +     spin_unlock_irqrestore(&base_crng.lock, flags);
> +}
[...]
> +/*
> + * Handle a new unique VM ID, which is unique, not secret, so we
> + * don't credit it, but we do mix it into the entropy pool and
> + * inject it into the crng.
> + */
> +void add_vmfork_randomness(const void *unique_vm_id, size_t size)
> +{
> +     add_device_randomness(unique_vm_id, size);
> +     crng_vm_fork_inject(unique_vm_id, size);
> +}
> +EXPORT_SYMBOL_GPL(add_vmfork_randomness);

I think we should be removing cases where the base_crng key is changed directly
besides extraction from the input_pool, not adding new ones.  Why not implement
this as add_device_randomness() followed by crng_reseed(force=true), where the
'force' argument forces a reseed to occur even if the entropy_count is too low?

- Eric