From:   Jann Horn <jannh@google.com>
To:     "Theodore Ts'o" <tytso@mit.edu>,
        "Jason A . Donenfeld" <Jason@zx2c4.com>
Cc:     linux-kernel@vger.kernel.org, Jann Horn <jannh@google.com>
Subject: [PATCH] random: Don't reset crng_init_cnt on urandom_read()
Date:   Mon,  3 Jan 2022 16:59:31 +0100
Message-Id: <20220103155931.411722-1-jannh@google.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

At the moment, urandom_read() (used for /dev/urandom) resets crng_init_cnt
to zero when it is called at crng_init<2. This is inconsistent: We do it
for /dev/urandom reads, but not for the equivalent
getrandom(GRND_INSECURE).

(And worse, as Jason pointed out, we're only doing this as long as
maxwarn>0.)

crng_init_cnt is only read in crng_fast_load(); it is relevant at
crng_init=3D=3D0 for determining when to switch to crng_init=3D=3D1 (and wh=
ere in
the RNG state array to write).

As far as I understand:

 - crng_init=3D=3D0 means "we have nothing, we might just be returning the =
same
   exact numbers on every boot on every machine, we don't even have
   non-cryptographic randomness; we should shove every bit of entropy we
   can get into the RNG immediately"
 - crng_init=3D=3D1 means "well we have something, it might not be
   cryptographic, but at least we're not gonna return the same data every
   time or whatever, it's probably good enough for TCP and ASLR and stuff;
   we now have time to build up actual cryptographic entropy in the input
   pool"
 - crng_init=3D=3D2 means "this is supposed to be cryptographically secure =
now,
   but we'll keep adding more entropy just to be sure".

The current code means that if someone is pulling data from /dev/urandom
fast enough at crng_init=3D=3D0, we'll keep resetting crng_init_cnt, and we=
'll
never make forward progress to crng_init=3D=3D1. It seems to be intended to
prevent an attacker from bruteforcing the contents of small individual RNG
inputs on the way from crng_init=3D=3D0 to crng_init=3D=3D1, but that's mis=
guided;
crng_init=3D=3D1 isn't supposed to provide proper cryptographic security
anyway, RNG users who care about getting secure RNG output have to wait
until crng_init=3D=3D2.

This code was inconsistent, and it probably made things worse - just get
rid of it.

Signed-off-by: Jann Horn <jannh@google.com>
---
 drivers/char/random.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 605969ed0f96..3c3f5c62378c 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1816,7 +1816,6 @@ urandom_read_nowarn(struct file *file, char __user *b=
uf, size_t nbytes,
 static ssize_t
 urandom_read(struct file *file, char __user *buf, size_t nbytes, loff_t *p=
pos)
 {
-	unsigned long flags;
 	static int maxwarn =3D 10;
=20
 	if (!crng_ready() && maxwarn > 0) {
@@ -1824,9 +1823,6 @@ urandom_read(struct file *file, char __user *buf, siz=
e_t nbytes, loff_t *ppos)
 		if (__ratelimit(&urandom_warning))
 			pr_notice("%s: uninitialized urandom read (%zd bytes read)\n",
 				  current->comm, nbytes);
-		spin_lock_irqsave(&primary_crng.lock, flags);
-		crng_init_cnt =3D 0;
-		spin_unlock_irqrestore(&primary_crng.lock, flags);
 	}
=20
 	return urandom_read_nowarn(file, buf, nbytes, ppos);

base-commit: c9e6606c7fe92b50a02ce51dda82586ebdf99b48
--=20
2.34.1.448.ga2b2bfdf31-goog