Message-Id: <20220217162848.303601-1-Jason@zx2c4.com>
Date:   Thu, 17 Feb 2022 17:28:48 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
        linux-arch@...r.kernel.org
Cc:     "Jason A. Donenfeld" <Jason@...c4.com>,
        Dinh Nguyen <dinguyen@...nel.org>,
        Nick Hu <nickhu@...estech.com>,
        Max Filippov <jcmvbkbc@...il.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        "David S . Miller" <davem@...emloft.net>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        Michal Simek <monstr@...str.eu>,
        Borislav Petkov <bp@...en8.de>, Guo Ren <guoren@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Joshua Kinard <kumba@...too.org>,
        David Laight <David.Laight@...lab.com>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Eric Biggers <ebiggers@...gle.com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Lennart Poettering <mzxreary@...inter.de>,
        Konstantin Ryabitsev <konstantin@...uxfoundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Theodore Ts'o <tytso@....edu>
Subject: [PATCH v1] random: block in /dev/urandom

This topic has come up countless times, and usually doesn't go anywhere.
This time I thought I'd bring it up with a slightly narrower focus,
updated for some developments over the last three years: we finally can
make /dev/urandom always secure, in light of the fact that our RNG is
now always seeded.

Ever since Linus' 50ee7529ec45 ("random: try to actively add entropy
rather than passively wait for it"), the RNG does a haveged-style jitter
dance around the scheduler, in order to produce entropy (and credit it)
for the case when we're stuck in wait_for_random_bytes(). How ever you
feel about the Linus Jitter Dance is beside the point: it's been there
for three years and usually gets the RNG initialized in a second or so.

As a matter of fact, this is what happens currently when people use
getrandom(). It's already there and working, and most people have been
using it for years without realizing.

So, given that the kernel has grown this mechanism for seeding itself
from nothing, and that this procedure happens pretty fast, maybe there's
no point any longer in having /dev/urandom give insecure bytes. In the
past we didn't want the boot process to deadlock, which was
understandable. But now, in the worst case, a second goes by, and the
problem is resolved. It seems like maybe we're finally at a point when
we can get rid of the infamous "urandom read hole".

The one slight drawback is that the Linus Jitter Dance relies on random_
get_entropy() being implemented. The first lines of try_to_generate_
entropy() are:

	stack.now = random_get_entropy();
	if (stack.now == random_get_entropy())
		return;

On most platforms, random_get_entropy() is simply aliased to get_cycles().
The number of machines without a cycle counter or some other
implementation of random_get_entropy() in 2022, which can also run a
mainline kernel, and at the same time have a both broken and out of date
userspace that relies on /dev/urandom never blocking at boot is thought
to be exceedingly low. And to be clear: those museum pieces without
cycle counters will continue to run Linux just fine, and even
/dev/urandom will be operable just like before; the RNG just needs to be
seeded first through the usual means, which should already be the case
now.

On systems that really do want unseeded randomness, we already offer
getrandom(GRND_INSECURE), which is in use by, e.g., systemd for seeding
their hash tables at boot. Nothing in this commit would affect
GRND_INSECURE, and it remains the means of getting those types of random
numbers.

This patch goes a long way toward eliminating a long overdue userspace
crypto footgun. After several decades of endless user confusion, we will
finally be able to say, "use any single one of our random interfaces and
you'll be fine. They're all the same. It doesn't matter." And that, I
think, is really something. Finally all of those blog posts and
disagreeing forums and contradictory articles will all become correct
about whatever they happened to recommend, and along with it, a whole
class of vulnerabilities eliminated.

With very minimal downside, we're finally in a position where we can
make this change.

Cc: Dinh Nguyen <dinguyen@...nel.org>
Cc: Nick Hu <nickhu@...estech.com>
Cc: Max Filippov <jcmvbkbc@...il.com>
Cc: Palmer Dabbelt <palmer@...belt.com>
Cc: David S. Miller <davem@...emloft.net>
Cc: Yoshinori Sato <ysato@...rs.sourceforge.jp>
Cc: Michal Simek <monstr@...str.eu>
Cc: Borislav Petkov <bp@...en8.de>
Cc: Guo Ren <guoren@...nel.org>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Joshua Kinard <kumba@...too.org>
Cc: David Laight <David.Laight@...lab.com>
Cc: Dominik Brodowski <linux@...inikbrodowski.net>
Cc: Eric Biggers <ebiggers@...gle.com>
Cc: Ard Biesheuvel <ardb@...nel.org>
Cc: Arnd Bergmann <arnd@...db.de>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Andy Lutomirski <luto@...nel.org>
Cc: Kees Cook <keescook@...omium.org>
Cc: Lennart Poettering <mzxreary@...inter.de>
Cc: Konstantin Ryabitsev <konstantin@...uxfoundation.org>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Theodore Ts'o <tytso@....edu>
Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
---
Having learned that MIPS32 isn't affected by this (initially my largest
worry), and then heartened today upon reading LWN's summary of our
previous discussion ("it would seem there are no huge barriers to
removing the final distinction between /dev/random and /dev/urandom"), I
figured I'd go ahead and submit a v1 of this. It seems at least worth
trying and seeing if somebody arrives with legitimate complaints. To
that end I've also widened the CC list quite a bit.

Changes v0->v1:
- We no longer touch GRND_INSECURE at all, in anyway. Lennart (and to an
  extent, Andy) pointed out that getting insecure numbers immediately at
  boot is still something that has legitimate use cases, so this patch
  no longer touches that code.

 drivers/char/mem.c     |  2 +-
 drivers/char/random.c  | 51 ++++++------------------------------------
 include/linux/random.h |  2 +-
 3 files changed, 9 insertions(+), 46 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index cc296f0823bd..9f586025dbe6 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -707,7 +707,7 @@ static const struct memdev {
 	 [5] = { "zero", 0666, &zero_fops, FMODE_NOWAIT },
 	 [7] = { "full", 0666, &full_fops, 0 },
 	 [8] = { "random", 0666, &random_fops, 0 },
-	 [9] = { "urandom", 0666, &urandom_fops, 0 },
+	 [9] = { "urandom", 0666, &random_fops, 0 },
 #ifdef CONFIG_PRINTK
 	[11] = { "kmsg", 0644, &kmsg_fops, 0 },
 #endif
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8d5abeefcc4f..fda5182d655d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -89,8 +89,6 @@ static LIST_HEAD(random_ready_list);
 /* Control how we warn userspace. */
 static struct ratelimit_state unseeded_warning =
 	RATELIMIT_STATE_INIT("warn_unseeded_randomness", HZ, 3);
-static struct ratelimit_state urandom_warning =
-	RATELIMIT_STATE_INIT("warn_urandom_randomness", HZ, 3);
 static int ratelimit_disable __read_mostly;
 module_param_named(ratelimit_disable, ratelimit_disable, int, 0644);
 MODULE_PARM_DESC(ratelimit_disable, "Disable random ratelimit suppression");
@@ -336,11 +334,6 @@ static void crng_reseed(void)
 				  unseeded_warning.missed);
 			unseeded_warning.missed = 0;
 		}
-		if (urandom_warning.missed) {
-			pr_notice("%d urandom warning(s) missed due to ratelimiting\n",
-				  urandom_warning.missed);
-			urandom_warning.missed = 0;
-		}
 	}
 }
 
@@ -993,10 +986,8 @@ int __init rand_initialize(void)
 		pr_notice("crng init done (trusting CPU's manufacturer)\n");
 	}
 
-	if (ratelimit_disable) {
-		urandom_warning.interval = 0;
+	if (ratelimit_disable)
 		unseeded_warning.interval = 0;
-	}
 	return 0;
 }
 
@@ -1382,20 +1373,16 @@ static void try_to_generate_entropy(void)
  * getrandom(2) is the primary modern interface into the RNG and should
  * be used in preference to anything else.
  *
- * Reading from /dev/random has the same functionality as calling
- * getrandom(2) with flags=0. In earlier versions, however, it had
- * vastly different semantics and should therefore be avoided, to
- * prevent backwards compatibility issues.
- *
- * Reading from /dev/urandom has the same functionality as calling
- * getrandom(2) with flags=GRND_INSECURE. Because it does not block
- * waiting for the RNG to be ready, it should not be used.
+ * Reading from /dev/random and /dev/urandom both have the same effect
+ * as calling getrandom(2) with flags=0. (In earlier versions, however,
+ * they each had different semantics.)
  *
  * Writing to either /dev/random or /dev/urandom adds entropy to
  * the input pool but does not credit it.
  *
- * Polling on /dev/random indicates when the RNG is initialized, on
- * the read side, and when it wants new entropy, on the write side.
+ * Polling on /dev/random or /dev/urandom indicates when the RNG
+ * is initialized, on the read side, and when it wants new entropy,
+ * on the write side.
  *
  * Both /dev/random and /dev/urandom have the same set of ioctls for
  * adding entropy, getting the entropy count, zeroing the count, and
@@ -1480,21 +1467,6 @@ static ssize_t random_write(struct file *file, const char __user *buffer,
 	return (ssize_t)count;
 }
 
-static ssize_t urandom_read(struct file *file, char __user *buf, size_t nbytes,
-			    loff_t *ppos)
-{
-	static int maxwarn = 10;
-
-	if (!crng_ready() && maxwarn > 0) {
-		maxwarn--;
-		if (__ratelimit(&urandom_warning))
-			pr_notice("%s: uninitialized urandom read (%zd bytes read)\n",
-				  current->comm, nbytes);
-	}
-
-	return get_random_bytes_user(buf, nbytes);
-}
-
 static ssize_t random_read(struct file *file, char __user *buf, size_t nbytes,
 			   loff_t *ppos)
 {
@@ -1581,15 +1553,6 @@ const struct file_operations random_fops = {
 	.llseek = noop_llseek,
 };
 
-const struct file_operations urandom_fops = {
-	.read = urandom_read,
-	.write = random_write,
-	.unlocked_ioctl = random_ioctl,
-	.compat_ioctl = compat_ptr_ioctl,
-	.fasync = random_fasync,
-	.llseek = noop_llseek,
-};
-
 
 /********************************************************************
  *
diff --git a/include/linux/random.h b/include/linux/random.h
index d7354de9351e..38ff777aad19 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -44,7 +44,7 @@ extern void del_random_ready_callback(struct random_ready_callback *rdy);
 extern size_t __must_check get_random_bytes_arch(void *buf, size_t nbytes);
 
 #ifndef MODULE
-extern const struct file_operations random_fops, urandom_fops;
+extern const struct file_operations random_fops;
 #endif
 
 u32 get_random_u32(void);
-- 
2.35.0

Powered by blists - more mailing lists