linux-kernel - Re: [PATCH] random: fix add_hwgenerator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Apr 2022 17:20:36 +0200
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Jan Varho <jan.varho@...il.com>
Cc:     Theodore Ts'o <tytso@....edu>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] random: fix add_hwgenerator_randomness entropy accounting

Hi Jan,

On Mon, Apr 4, 2022 at 5:07 PM Jan Varho <jan.varho@...il.com> wrote:
> add_hwgenerator_randomness tries to only use the required amound of input
> for fast init, but credits all the entropy if even a byte was left over.
>
> Fix by not crediting entropy if any input was consumed for fast init.

Yea, I'd seen this and wasn't really sure what the correct fix was. My
recent addition of `&& entropy < POOL_MIN_BITS` is a step in the right
direction of making your fix the desirable path, since it makes it less
likely that we'd wind up throwing away "good" entropy. So maybe it's
time we do that.

The alternative I had considered was something like `entropy -= ret *
entropy / buf`, with some additional care around rounding in the right
direction. But even then, that makes a big assumption about the
distribution of the entropy within the buffer bitstring. What if it's
all at the beginning and none at the end? The fact that entropy might
not be equal to count means all bets are off the table and we might well
be facing pretty meh input.

Anyway, if your approach is indeed the way forward, the fuller version
of this patch is probably something like the below, where we just get
rid of the now-useless return value, and then since we're now doing
partial mixing, we can change the way the account parameter bounds it.
This is untested, but if you want to test it and submit it at v2, I
think it might be an okay incarnation of the lazy approach.

Jason

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 1d8242969751..de8040db426e 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -437,11 +437,8 @@ static void crng_make_state(u32 chacha_state[CHACHA_STATE_WORDS],
  * This shouldn't be set by functions like add_device_randomness(),
  * where we can't trust the buffer passed to it is guaranteed to be
  * unpredictable (so it might not have any entropy at all).
- *
- * Returns the number of bytes processed from input, which is bounded
- * by CRNG_INIT_CNT_THRESH if account is true.
  */
-static size_t crng_pre_init_inject(const void *input, size_t len, bool account)
+static void crng_pre_init_inject(const void *input, size_t len, bool account)
 {
 	static int crng_init_cnt = 0;
 	struct blake2s_state hash;
@@ -455,15 +452,12 @@ static size_t crng_pre_init_inject(const void *input, size_t len, bool account)
 		return 0;
 	}

-	if (account)
-		len = min_t(size_t, len, CRNG_INIT_CNT_THRESH - crng_init_cnt);
-
 	blake2s_update(&hash, base_crng.key, sizeof(base_crng.key));
 	blake2s_update(&hash, input, len);
 	blake2s_final(&hash, base_crng.key);

 	if (account) {
-		crng_init_cnt += len;
+		crng_init_cnt += min_t(size_t, len, CRNG_INIT_CNT_THRESH - crng_init_cnt);
 		if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
 			++base_crng.generation;
 			crng_init = 1;
@@ -474,8 +468,6 @@ static size_t crng_pre_init_inject(const void *input, size_t len, bool account)

 	if (crng_init == 1)
 		pr_notice("fast init done\n");
-
-	return len;
 }

 static void _get_random_bytes(void *buf, size_t nbytes)
@@ -1141,12 +1133,9 @@ void add_hwgenerator_randomness(const void *buffer, size_t count,
 				size_t entropy)
 {
 	if (unlikely(crng_init == 0 && entropy < POOL_MIN_BITS)) {
-		size_t ret = crng_pre_init_inject(buffer, count, true);
-		mix_pool_bytes(buffer, ret);
-		count -= ret;
-		buffer += ret;
-		if (!count || crng_init == 0)
-			return;
+		crng_pre_init_inject(buffer, count, true);
+		mix_pool_bytes(buffer, count);
+		return;
 	}

 	/*