[PATCH] random: fix locking for crng_init in crng_reseed()

[PATCH] random: fix locking for crng_init in crng_reseed()

[Dominik Brodowski]

crng_init is protected by primary_crng->lock. Therefore, we need
to hold this lock when increasing crng_init to 2. As we shouldn't
hold this lock for too long, only hold it for those parts which
require protection.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/char/random.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index cc4d9d414df2..aee56032ebb4 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -497,6 +497,7 @@ static void crng_slow_load(const void *cp, unsigned int len)
 
 static void crng_reseed(void)
 {
+	bool complete_init = false;
 	unsigned long flags;
 	int entropy_count;
 	unsigned long next_gen;
@@ -526,12 +527,14 @@ static void crng_reseed(void)
 		++next_gen;
 	WRITE_ONCE(base_crng.generation, next_gen);
 	base_crng.birth = jiffies;
-	spin_unlock_irqrestore(&base_crng.lock, flags);
-	memzero_explicit(key, sizeof(key));
-
 	if (crng_init < 2) {
 		invalidate_batched_entropy();
 		crng_init = 2;
+		complete_init = true;
+	}
+	spin_unlock_irqrestore(&base_crng.lock, flags);
+	memzero_explicit(key, sizeof(key));
+	if (complete_init) {
 		process_random_ready_list();
 		wake_up_interruptible(&crng_init_wait);
 		kill_fasync(&fasync, SIGIO, POLL_IN);

Re: [PATCH] random: fix locking for crng_init in crng_reseed()

[Jason A. Donenfeld]

Hi Dominik,

Thanks, applied. I changed complete_init to finalize_init, to match
our naming scheme from earlier, and I moved
invalidate_batched_entropy() to outside the lock and after
crng_init=2, since now it uses atomics, and it should probably be
ordered after crng_init = 2, so the new batch gets the new entropy.

Actually, though, come to think of it: shouldn't we always call
invalidate_batched_entropy() after reseeding? More generally, we can
instead probably tie the entropy generation counter to the base_crng
counter, and have this all done automatically. That might be something
interesting to do in the future.

Jason

Re: [PATCH] random: fix locking for crng_init in crng_reseed()

[Dominik Brodowski]

Hi Jason,

Am Wed, Feb 09, 2022 at 10:39:17PM +0100 schrieb Jason A. Donenfeld:
> Thanks, applied. I changed complete_init to finalize_init, to match
> our naming scheme from earlier, and I moved
> invalidate_batched_entropy() to outside the lock and after
> crng_init=2, since now it uses atomics, and it should probably be
> ordered after crng_init = 2, so the new batch gets the new entropy.

Doesn't that mean that there is a small window where crng_init == 2, but
get_random_u64/get_random_u32 still returns old data, with potentially
insufficient entropy (as obtained at a time when crng_init was still < 2)?
That's why I moved invalidate_batched_entropy() under the lock.

But with your subsequent patch, it doesn't matter any more.

Thanks,
	Dominik

Re: [PATCH] random: fix locking for crng_init in crng_reseed()

[Jason A. Donenfeld]

Hi Dominik,

Thanks, I see your point. I'll do it the way you suggested (which as
you pointed out goes away anyway right after).

Jason

Re: [PATCH] random: fix locking for crng_init in crng_reseed()

[Eric Biggers]

On Wed, Feb 09, 2022 at 07:57:06PM +0100, Dominik Brodowski wrote:
> crng_init is protected by primary_crng->lock. Therefore, we need
> to hold this lock when increasing crng_init to 2. As we shouldn't
> hold this lock for too long, only hold it for those parts which
> require protection.
> 
> Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> ---
>  drivers/char/random.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Biggers <ebiggers@google.com>

Though to bikeshed on the variable name, I think that 'became_ready' would be
more self-explanatory than 'complete_init' (this patch) and 'finalize_init'
(the version committed).

- Eric