Re: [PATCHv5] random: Make /dev/random wait for input_pool initializedy

[Theodore Y. Ts'o]

On Thu, Feb 21, 2019 at 07:24:14PM +0000, Bernd Edlinger wrote:
> 
> My observation, with a previous attempt (v3) of my patch is that a system
> where only interrupt randomness is available it takes typically 2 minutes
> to accomplish the initial seeding of the CRNG, if from there one has to
> wait until there is 128 bit entropy in the blocking pool it will take
> much too long.  The patch was actually buggy and did only ensure 80 bits
> of entropy in the blocking pool but even that did take 6.5 minutes, which
> felt to me like an absolutely unacceptable waiting time.
> 
> Therefore I would like to keep that to 2-3 minutes that the CRNG takes
> for it's initialization.  From there I thought, even if the entroy
> in the input_pool drops to zero, the information content however is still
> there, and after adding the next 64 bits if raw entropy, it would be fine
> to extract 8 bytes form the input_pool and feed that to the
> blocking pool, that would make 6 bytes of output, which should be
> completely unpredictable, the approach taken in my v5 patch.
> If that is not good enough, maybe extract 128 bits from the urandom
> and inject that into the blocking pool without incrementing
> the blocking_pool's entropy count.

The whole premise of reading from /dev/random is that it should only
allow reads up to available estimated entropy.  I'm assuming here that
sane users of /dev/random will be reading in chunks of at least 128
bits, reading smaller amounts for, say, a 32-bit SPECK key, is not
someone who is paranoid enough to want to use /dev/random is likely to
want to do.  So what I was doing was to simply prevent any reads from
/dev/random until it had accumulated 128 bits worth of entropy.  If
the user is reading 128 bits in order to generate a 128-bit session
key, this won't actually slow down /dev/random any more that it does
today.

It will slow down someone who just wants to read a byte from
/dev/random immediately after boot --- but as far as I'm concerned,
that's crazy, so I don't really care about optimizing it.  Your
suggestion of simply not allowing any reads until the CRNG is
initialized, and then injecting 128-bits into the blocking pool would
also work, but it wouldn't speed up the use case of "the user is
trying to read 128 bits from /dev/random".  It only speeds up "read 1
byte from /dev/random".

Personally, I would generally be simply tell users, "use getrandom(2)
and be happy", and I consider /dev/random to be a legacy interface.
It's just that there are some crazy customers who seem to believe that
/dev/random is required for FIPS compliance.

So optimizing for users who want to read vast amount of data from
/dev/random is a non-goal as far as I am concerned.  In particular,
seeding the CRNG and keeping it properly reseeded is higher priority
as far as I'm concerned.  If that slows down /dev/random a bit,
/dev/random is *always* going to be slow.

> > -			struct entropy_store *other = &blocking_pool;
> > -
> > -			if (other->entropy_count <=
> > -			    3 * other->poolinfo->poolfracbits / 4) {
> > -				schedule_work(&other->push_work);
> > -				r->entropy_total = 0;
> > -			}
> > -		}
> > +		if (!work_pending(&other->push_work) &&
> > +		    (ENTROPY_BITS(r) > 6 * r->poolinfo->poolbytes) &&
> > +		    (ENTROPY_BITS(other) <= 6 * other->poolinfo->poolbytes))
> > +			schedule_work(&other->push_work);
> 
> push_to_pool will transfer chunks of random_read_wakeup_bits.
> 
> I think push_to_pool should also match this change.

I was trying to keep the size of this patch as small as practical,
since the primary goal was to improve the security of the bits
returned when reading the a 128 bit of randomness immediately after
boot.

> I like it that this path is controllable via random_write_wakeup_bits,
> that would be lost with this change.

Very few people actually use these knobs, and in fact I regret making
them available, since changing these to insane values can impact the
security properties of /dev/random.  I don't actually see a good
reason why a user *would* want to adjust the behavior of this code
path, and it makes it much simpler to reason about how this code path
works if we don't make it controllable by the user.

> > @@ -1553,6 +1548,11 @@ static ssize_t extract_entropy_user(struct entropy_store *r, void __user *buf,
> >  	int large_request = (nbytes > 256);
> >  
> >  	trace_extract_entropy_user(r->name, nbytes, ENTROPY_BITS(r), _RET_IP_);
> > +	if (!r->initialized && r->pull) {
> > +		xfer_secondary_pool(r, ENTROPY_BITS(r->pull)/8);
> > +		if (!r->initialized)
> > +			return 0;
> 
> Did you want to do that in push_to_pool?

No, this was deliberate.  The point here is that if the blocking pool
is not initialized (which is defined as having accumulated 128 bits of
entropy once), we refuse to return any entropy at all.

> The second part of the _random_read does not match this change:
> 
>                 wait_event_interruptible(random_read_wait,
>                         ENTROPY_BITS(&input_pool) >=
>                         random_read_wakeup_bits);
>                 if (signal_pending(current))
>                         return -ERESTARTSYS;
> 
> 
> and will go into a busy loop, when
> ENTROPY_BITS(&input_pool) >= random_read_wakeup_bits, right?

No, because what's being tested is the entropy estimate in the *input*
pool.  If there is entropy available, then xfer_secondary_pool() will
transfer entropy from the input pool to the blocking pool.  This will
decrease the entropy estimate for the input pool, so we won't busy
loop.  If the entropy estimate for the blocking pool increases to
above 128 bits, then the initialized flag will get set, and at that
point we will start returning random data to the user.

> The select is basically done here I think this should not indicate read readiness
> before the pool is initialized that is needs to be changed, right?

Yes, we should adjust random_pool so it won't report that the fd is
readable unless the blocking pool is initialized.

	 					- Ted