Re: [PATCH] random: mix in timestamps and reseed on system restore

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Hi Dominik,

On Fri, May 13, 2022 at 08:25:12AM +0200, Dominik Brodowski wrote:
> Am Sun, May 01, 2022 at 02:38:49PM +0200 schrieb Jason A. Donenfeld:
> > Since the RNG loses freshness system suspend/hibernation, when we
> > resume, immediately reseed using whatever data we can, which for this
> > particular case is the various timestamps regarding system suspend time,
> > in addition to more generally the RDSEED/RDRAND/RDTSC values that happen
> > whenever the crng reseeds.
> > 
> > On systems that suspend and resume automatically all the time -- such as
> > Android -- we skip the reseeding on suspend resumption, since that could
> > wind up being far too busy. This is the same trade-off made in
> > WireGuard.
> > 
> > In addition to reseeding upon resumption always mix into the pool these
> > various stamps on every power notification event.
> > 
> > Cc: Theodore Ts'o <tytso@mit.edu>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> >  drivers/char/random.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > index 0935a140795e..48eac27214ea 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -53,6 +53,7 @@
> >  #include <linux/completion.h>
> >  #include <linux/uuid.h>
> >  #include <linux/uaccess.h>
> > +#include <linux/suspend.h>
> >  #include <crypto/chacha.h>
> >  #include <crypto/blake2s.h>
> >  #include <asm/processor.h>
> > @@ -966,6 +967,37 @@ static int __init parse_trust_bootloader(char *arg)
> >  early_param("random.trust_cpu", parse_trust_cpu);
> >  early_param("random.trust_bootloader", parse_trust_bootloader);
> >  
> > +static int random_pm_notification(struct notifier_block *nb, unsigned long action, void *data)
> > +{
> > +	unsigned long flags, entropy = random_get_entropy();
> > +
> > +	/*
> > +	 * Encode a representation of how long the system has been suspended,
> > +	 * in a way that is distinct from prior system suspends.
> > +	 */
> > +	ktime_t stamps[] = {
> > +		ktime_get(),
> > +		ktime_get_boottime(),
> > +		ktime_get_real()
> > +	};
> > +
> > +	spin_lock_irqsave(&input_pool.lock, flags);
> > +	_mix_pool_bytes(&action, sizeof(action));
> > +	_mix_pool_bytes(stamps, sizeof(stamps));
> > +	_mix_pool_bytes(&entropy, sizeof(entropy));
> > +	spin_unlock_irqrestore(&input_pool.lock, flags);
> > +
> > +	if (action == PM_RESTORE_PREPARE ||
> > +	    (action == PM_POST_SUSPEND &&
> > +	     !IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !IS_ENABLED(CONFIG_ANDROID))) {
> > +		crng_reseed(true);
> > +		pr_notice("crng reseeded on system resumption\n");
> > +	}
> > +	return 0;
> > +}
> 
> Should this also wake up any thread waiting in add_hwgenerator_randomness()
> / "use" the input already in store there?

That would be a nice enhancement I think. Feel free to submit a patch.
Maybe more generally, the add_hwgenerator_randomness() thing should wake
up not based on a timer but just everytime crng_reseed() is called by
something? There are a lot of ways to skin that cat, and hopefully we'll
be able to really revamp the way hwrandomness is used in the future,
both here and other concerns you've brought up like 0 crediting.

Jason