[PATCH] random: fix add_hwgenerator_randomness entropy accounting

[PATCH] random: fix add_hwgenerator_randomness entropy accounting

[Jan Varho]

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.

Signed-off-by: Jan Varho <jan.varho@gmail.com>
---
 drivers/char/random.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 1d8242969751..fb20178f1044 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1141,12 +1141,10 @@ 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)
+		if (crng_pre_init_inject(buffer, count, true) > 0) {
+			mix_pool_bytes(buffer, count);
 			return;
+		}
 	}
 
 	/*
-- 
2.25.1

Re: [PATCH] random: fix add_hwgenerator_randomness entropy accounting

[Jason A. Donenfeld]

Hi Jan,

On Mon, Apr 4, 2022 at 5:07 PM Jan Varho <jan.varho@gmail.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;
 	}
 
 	/*

Re: [PATCH] random: fix add_hwgenerator_randomness entropy accounting

[Jan Varho]

Hi,

On Mon, Apr 4, 2022 at 6:20 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > 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.

Yes, but since it's a hwrng, hopefully it will supply more soon.

And like you say, the new condition makes it less likely to happen.

> 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.

I thought about doing it that way, but the return value allows
checking that any input was actually used instead of fast init
completing in the mean time.

If you want I can do that.

Jan

Re: [PATCH] random: fix add_hwgenerator_randomness entropy accounting

[Jason A. Donenfeld]

Hi Jan,

On Mon, Apr 4, 2022 at 5:29 PM Jan Varho <jan.varho@gmail.com> wrote:
> I thought about doing it that way, but the return value allows
> checking that any input was actually used instead of fast init
> completing in the mean time.

I'm not too worried about races here. If lots of things are fighting
to inject entropy here, we're not in a starved situation. Also, the
other callers aren't looking at the return value. Let's keep it
simple; there's little harm in doing so in this instance.

Jason

[PATCH v2] random: fix add_hwgenerator_randomness entropy accounting

[Jan Varho]

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 when using the input for fast init.

Signed-off-by: Jan Varho <jan.varho@gmail.com>
---
 drivers/char/random.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 1d8242969751..4d77de688016 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;
@@ -452,18 +449,15 @@ static size_t crng_pre_init_inject(const void *input, size_t len, bool account)
 	spin_lock_irqsave(&base_crng.lock, flags);
 	if (crng_init != 0) {
 		spin_unlock_irqrestore(&base_crng.lock, flags);
-		return 0;
+		return;
 	}
 
-	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;
 	}
 
 	/*
-- 
2.25.1

[PATCH v3] random: fix add_hwgenerator_randomness entropy accounting

[Jan Varho]

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 when using the input for fast init.

Signed-off-by: Jan Varho <jan.varho@gmail.com>
---
 drivers/char/random.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 1d8242969751..ee3ad2ba0942 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;
@@ -452,18 +449,15 @@ static size_t crng_pre_init_inject(const void *input, size_t len, bool account)
 	spin_lock_irqsave(&base_crng.lock, flags);
 	if (crng_init != 0) {
 		spin_unlock_irqrestore(&base_crng.lock, flags);
-		return 0;
+		return;
 	}
 
-	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;
 	}
 
 	/*
-- 
2.25.1

Re: [PATCH v3] random: fix add_hwgenerator_randomness entropy accounting

[Jason A. Donenfeld]

Hi Jan,

On Mon, Apr 4, 2022 at 6:43 PM Jan Varho <jan.varho@gmail.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 when using the input for fast init.

Looks good, thanks. Applied as
https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git/commit/?id=527a9867

Jason