diff mbox series

[v2,4/9] random: ensure early RDSEED goes through mixer on init

Message ID 20220209011919.493762-5-Jason@zx2c4.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series random: cleanups around per-cpu crng & rdrand | expand

Commit Message

Jason A. Donenfeld Feb. 9, 2022, 1:19 a.m. UTC 
Continuing the reasoning of "random: use RDSEED instead of RDRAND in
entropy extraction" from this series, at init time we also don't want to
be xoring RDSEED directly into the crng. Instead it's safer to put it
into our entropy collector and then re-extract it, so that it goes
through a hash function with preimage resistance.

Cc: Theodore Ts'o <tytso@mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Reviewed-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/char/random.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

Comments

Dominik Brodowski Feb. 9, 2022, 8:23 a.m. UTC | #1 
Am Wed, Feb 09, 2022 at 02:19:14AM +0100 schrieb Jason A. Donenfeld:
> Continuing the reasoning of "random: use RDSEED instead of RDRAND in
> entropy extraction" from this series, at init time we also don't want to
> be xoring RDSEED directly into the crng. Instead it's safer to put it
> into our entropy collector and then re-extract it, so that it goes
> through a hash function with preimage resistance.

Any reason why you re-order

> +	mix_pool_bytes(utsname(), sizeof(*(utsname())));
>  	mix_pool_bytes(&now, sizeof(now));

? It shouldn't matter, but it's an additional change I see no rationale for.

Also, AFAICS, we now only call rdseed 8 times (to mix into the input pool
directly and to update the primary pool indirectly) instead of 8 times (for
the input pool) and 12 times (for initializing the primary pool). That's
still 64 bytes, and we use that to seed 48 bytes, we're still on the safe
side. So feel free to add my

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

Thanks,
	Dominik
Jason A. Donenfeld Feb. 9, 2022, 10:37 a.m. UTC | #2 
On Wed, Feb 9, 2022 at 9:31 AM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> Am Wed, Feb 09, 2022 at 02:19:14AM +0100 schrieb Jason A. Donenfeld:
> > Continuing the reasoning of "random: use RDSEED instead of RDRAND in
> > entropy extraction" from this series, at init time we also don't want to
> > be xoring RDSEED directly into the crng. Instead it's safer to put it
> > into our entropy collector and then re-extract it, so that it goes
> > through a hash function with preimage resistance.
>
> Any reason why you re-order
>
> > +     mix_pool_bytes(utsname(), sizeof(*(utsname())));
> >       mix_pool_bytes(&now, sizeof(now));

My "vim fingers" did that as a matter of habit. But it's actually
maybe worse this way, in a very subtle way that really doesn't really
matter. The RDSEED bytes should be hashed in first, not last, so that
we don't need to rely on the hash function's collision protection. In
general crypto hygiene, HASH(secret||thing) is sometimes preferable to
HASH(thing||secret). I'll fix that up and mention it in the commit
message. Thanks for noticing it.

>
> ? It shouldn't matter, but it's an additional change I see no rationale for.
>
> Also, AFAICS, we now only call rdseed 8 times (to mix into the input pool
> directly and to update the primary pool indirectly) instead of 8 times (for
> the input pool) and 12 times (for initializing the primary pool). That's
> still 64 bytes, and we use that to seed 48 bytes, we're still on the safe
> side. So feel free to add my

And later in this patchset, this is reduced to a 32 byte extraction
(which is the size of our pool, which is what we were aiming for).
Compressing an entire 64 byte blake block of rdseed down to 32 bytes
puts us in a very good position.

Jason
diff mbox series

Patch

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 81786bef0a8e..75dc370d83b5 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1218,24 +1218,18 @@  int __init rand_initialize(void)
 	bool arch_init = true;
 	unsigned long rv;
 
+	mix_pool_bytes(utsname(), sizeof(*(utsname())));
 	mix_pool_bytes(&now, sizeof(now));
 	for (i = BLAKE2S_BLOCK_SIZE; i > 0; i -= sizeof(rv)) {
-		if (!arch_get_random_seed_long(&rv) &&
-		    !arch_get_random_long(&rv))
-			rv = random_get_entropy();
-		mix_pool_bytes(&rv, sizeof(rv));
-	}
-	mix_pool_bytes(utsname(), sizeof(*(utsname())));
-
-	extract_entropy(&primary_crng.state[4], sizeof(u32) * 12);
-	for (i = 4; i < 16; i++) {
 		if (!arch_get_random_seed_long_early(&rv) &&
 		    !arch_get_random_long_early(&rv)) {
 			rv = random_get_entropy();
 			arch_init = false;
 		}
-		primary_crng.state[i] ^= rv;
+		mix_pool_bytes(&rv, sizeof(rv));
 	}
+
+	extract_entropy(&primary_crng.state[4], sizeof(u32) * 12);
 	if (arch_init && trust_cpu && crng_init < 2) {
 		invalidate_batched_entropy();
 		crng_init = 2;