Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness

[Jason A. Donenfeld]

Hi Ted,

On Wed, Jun 21, 2017 at 10:38 PM, Theodore Ts'o  wrote:
> I agree completely with all of this.  The following patch replaces the
> current topmost patch on the random.git tree:
> For developers who want to work on improving this situation,
> CONFIG_WARN_UNSEEDED_RANDOM has been renamed to
> CONFIG_WARN_ALL_UNSEEDED_RANDOM.  By default the kernel will always
> print the first use of unseeded randomness.  This way, hopefully the
> security obsessed will be happy that there is _some_ indication when
> the kernel boots there may be a potential issue with that architecture
> or subarchitecture.  To see all uses of unseeded randomness,
> developers can enable CONFIG_WARN_ALL_UNSEEDED_RANDOM.

Seems fine to me.

Acked-by: Jason A. Donenfeld 

Jason

Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness

[Theodore Ts'o]

On Wed, Jun 21, 2017 at 04:06:49PM +1000, Michael Ellerman wrote:
> All the distro kernels I'm aware of have DEBUG_KERNEL=y.
> 
> Where all includes at least RHEL, SLES, Fedora, Ubuntu & Debian.
> 
> So it's still essentially default y.
> 
> Emitting *one* warning by default would be reasonable. That gives users
> who are interested something to chase, they can then turn on the option
> to get the full story.
> 
> Filling the dmesg buffer with repeated warnings is really not helpful.

I agree completely with all of this.  The following patch replaces the
current topmost patch on the random.git tree:


>From 25b683ee9bd5536807f813efbd19809333461f89 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o 
Date: Thu, 8 Jun 2017 04:16:59 -0400
Subject: [PATCH] random: suppress spammy warnings about unseeded randomness

Unfortunately, on some models of some architectures getting a fully
seeded CRNG is extremely difficult, and so this can result in dmesg
getting spammed for a surprisingly long time.  This is really bad from
a security perspective, and so architecture maintainers needed to do
what they can to get the CRNG seeded sooner after the system is
booted.  However, users can't do anything actionble to address this,
and spamming the kernel messages log will only just annoy people.

For developers who want to work on improving this situation,
CONFIG_WARN_UNSEEDED_RANDOM has been renamed to
CONFIG_WARN_ALL_UNSEEDED_RANDOM.  By default the kernel will always
print the first use of unseeded randomness.  This way, hopefully the
security obsessed will be happy that there is _some_ indication when
the kernel boots there may be a potential issue with that architecture
or subarchitecture.  To see all uses of unseeded randomness,
developers can enable CONFIG_WARN_ALL_UNSEEDED_RANDOM.

Signed-off-by: Theodore Ts'o 
---
 drivers/char/random.c | 45 ++---
 lib/Kconfig.debug | 24 ++--
 2 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index fa5bbd5a7ca0..7405c914bbcf 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1466,6 +1466,30 @@ static ssize_t extract_entropy_user(struct entropy_store 
*r, void __user *buf,
return ret;
 }
 
+#define warn_unseeded_randomness(previous) \
+   _warn_unseeded_randomness(__func__, (void *) _RET_IP_, (previous))
+
+static void _warn_unseeded_randomness(const char *func_name, void *caller,
+ void **previous)
+{
+#ifdef CONFIG_WARN_ALL_UNSEEDED_RANDOM
+   const bool print_once = false;
+#else
+   static bool print_once __read_mostly;
+#endif
+
+   if (print_once ||
+   crng_ready() ||
+   (previous && (caller == READ_ONCE(*previous
+   return;
+   WRITE_ONCE(*previous, caller);
+#ifndef CONFIG_WARN_ALL_UNSEEDED_RANDOM
+   print_once = true;
+#endif
+   pr_notice("random: %s called from %pF with crng_init=%d\n",
+ func_name, caller, crng_init);
+}
+
 /*
  * This function is the exported kernel interface.  It returns some
  * number of good random numbers, suitable for key generation, seeding
@@ -1479,12 +1503,9 @@ static ssize_t extract_entropy_user(struct entropy_store 
*r, void __user *buf,
 void get_random_bytes(void *buf, int nbytes)
 {
__u8 tmp[CHACHA20_BLOCK_SIZE];
+   static void *previous;
 
-#ifdef CONFIG_WARN_UNSEEDED_RANDOM
-   if (!crng_ready())
-   printk(KERN_NOTICE "random: %pF get_random_bytes called "
-  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
-#endif
+   warn_unseeded_randomness();
trace_get_random_bytes(nbytes, _RET_IP_);
 
while (nbytes >= CHACHA20_BLOCK_SIZE) {
@@ -2064,6 +2085,7 @@ u64 get_random_u64(void)
bool use_lock = READ_ONCE(crng_init) < 2;
unsigned long flags = 0;
struct batched_entropy *batch;
+   static void *previous;
 
 #if BITS_PER_LONG == 64
if (arch_get_random_long((unsigned long *)))
@@ -2074,11 +2096,7 @@ u64 get_random_u64(void)
return ret;
 #endif
 
-#ifdef CONFIG_WARN_UNSEEDED_RANDOM
-   if (!crng_ready())
-   printk(KERN_NOTICE "random: %pF get_random_u64 called "
-  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
-#endif
+   warn_unseeded_randomness();
 
batch = _cpu_var(batched_entropy_u64);
if (use_lock)
@@ -2102,15 +2120,12 @@ u32 get_random_u32(void)
bool use_lock = READ_ONCE(crng_init) < 2;
unsigned long flags = 0;
struct batched_entropy *batch;
+   static void *previous;
 
if (arch_get_random_int())
return ret;
 
-#ifdef CONFIG_WARN_UNSEEDED_RANDOM
-   if (!crng_ready())
-   printk(KERN_NOTICE "random: %pF get_random_u32 called "
-  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);

Re: [kernel-hardening] [PATCH] random: warn when kernel uses unseeded randomness

[Michael Ellerman]

"Jason A. Donenfeld"  writes:

> This enables an important dmesg notification about when drivers have
> used the crng without it being seeded first. Prior, these errors would
> occur silently, and so there hasn't been a great way of diagnosing these
> types of bugs for obscure setups. By adding this as a config option, we
> can leave it on by default, so that we learn where these issues happen,
> in the field, will still allowing some people to turn it off, if they
> really know what they're doing and do not want the log entries.
>
> However, we don't leave it _completely_ by default. An earlier version
> of this patch simply had `default y`. I'd really love that, but it turns
> out, this problem with unseeded randomness being used is really quite
> present and is going to take a long time to fix. Thus, as a compromise
> between log-messages-for-all and nobody-knows, this is `default y`,
> except it is also `depends on DEBUG_KERNEL`. This will ensure that the
> curious see the messages while others don't have to.

All the distro kernels I'm aware of have DEBUG_KERNEL=y.

Where all includes at least RHEL, SLES, Fedora, Ubuntu & Debian.

So it's still essentially default y.

Emitting *one* warning by default would be reasonable. That gives users
who are interested something to chase, they can then turn on the option
to get the full story.

Filling the dmesg buffer with repeated warnings is really not helpful.

cheers

Re: [PATCH] random: warn when kernel uses unseeded randomness

[Kees Cook]

On Tue, Jun 20, 2017 at 5:03 PM, Jason A. Donenfeld  wrote:
> This enables an important dmesg notification about when drivers have
> used the crng without it being seeded first. Prior, these errors would
> occur silently, and so there hasn't been a great way of diagnosing these
> types of bugs for obscure setups. By adding this as a config option, we
> can leave it on by default, so that we learn where these issues happen,
> in the field, will still allowing some people to turn it off, if they
> really know what they're doing and do not want the log entries.
>
> However, we don't leave it _completely_ by default. An earlier version
> of this patch simply had `default y`. I'd really love that, but it turns
> out, this problem with unseeded randomness being used is really quite
> present and is going to take a long time to fix. Thus, as a compromise
> between log-messages-for-all and nobody-knows, this is `default y`,
> except it is also `depends on DEBUG_KERNEL`. This will ensure that the
> curious see the messages while others don't have to.

This commit log needs updating (default DEBUG_KERNEL, not depends).

But otherwise:

Reviewed-by: Kees Cook 

-Kees

>
> Signed-off-by: Jason A. Donenfeld 
> Signed-off-by: Theodore Ts'o 
> ---
> Hi Ted,
>
> This patch is meant to replace d06bfd1989fe97623b32d6df4ffa6e4338c99dc8,
> which is currently in your dev tree. It switches from using `default y`
> and `depends on DEBUG_KERNEL` to using the more simple `default DEBUG_KERNEL`.
> This kind of change I think should satisfy most potential objections, by
> being present for those who might find it useful, but invisble for those
> who don't want the spam.
>
> If you'd like to replace the earlier commit with this one, feel free. If
> not, that's fine too.
>
> Jason
>
>  drivers/char/random.c | 15 +--
>  lib/Kconfig.debug | 15 +++
>  2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 3853dd4f92e7..fa5bbd5a7ca0 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -288,7 +288,6 @@
>  #define SEC_XFER_SIZE  512
>  #define EXTRACT_SIZE   10
>
> -#define DEBUG_RANDOM_BOOT 0
>
>  #define LONGS(x) (((x) + sizeof(unsigned long) - 1)/sizeof(unsigned long))
>
> @@ -1481,7 +1480,7 @@ void get_random_bytes(void *buf, int nbytes)
>  {
> __u8 tmp[CHACHA20_BLOCK_SIZE];
>
> -#if DEBUG_RANDOM_BOOT > 0
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> if (!crng_ready())
> printk(KERN_NOTICE "random: %pF get_random_bytes called "
>"with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> @@ -2075,6 +2074,12 @@ u64 get_random_u64(void)
> return ret;
>  #endif
>
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> +   if (!crng_ready())
> +   printk(KERN_NOTICE "random: %pF get_random_u64 called "
> +  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> +#endif
> +
> batch = _cpu_var(batched_entropy_u64);
> if (use_lock)
> read_lock_irqsave(_entropy_reset_lock, flags);
> @@ -2101,6 +2106,12 @@ u32 get_random_u32(void)
> if (arch_get_random_int())
> return ret;
>
> +#ifdef CONFIG_WARN_UNSEEDED_RANDOM
> +   if (!crng_ready())
> +   printk(KERN_NOTICE "random: %pF get_random_u32 called "
> +  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
> +#endif
> +
> batch = _cpu_var(batched_entropy_u32);
> if (use_lock)
> read_lock_irqsave(_entropy_reset_lock, flags);
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e4587ebe52c7..41cf12288369 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1209,6 +1209,21 @@ config STACKTRACE
>   It is also used by various kernel debugging features that require
>   stack trace generation.
>
> +config WARN_UNSEEDED_RANDOM
> +   bool "Warn when kernel uses unseeded randomness"
> +   default DEBUG_KERNEL
> +   help
> + Some parts of the kernel contain bugs relating to their use of
> + cryptographically secure random numbers before it's actually 
> possible
> + to generate those numbers securely. This setting ensures that these
> + flaws don't go unnoticed, by enabling a message, should this ever
> + occur. This will allow people with obscure setups to know when 
> things
> + are going wrong, so that they might contact developers about fixing
> + it.
> +
> + Say Y here, unless you simply do not care about using unseeded
> + randomness and do not want a potential warning message in your logs.
> +
>  config DEBUG_KOBJECT
> bool "kobject debugging"
> depends on DEBUG_KERNEL
> --
> 2.13.1
>



-- 
Kees Cook
Pixel Security

[PATCH] random: warn when kernel uses unseeded randomness

[Jason A. Donenfeld]

This enables an important dmesg notification about when drivers have
used the crng without it being seeded first. Prior, these errors would
occur silently, and so there hasn't been a great way of diagnosing these
types of bugs for obscure setups. By adding this as a config option, we
can leave it on by default, so that we learn where these issues happen,
in the field, will still allowing some people to turn it off, if they
really know what they're doing and do not want the log entries.

However, we don't leave it _completely_ by default. An earlier version
of this patch simply had `default y`. I'd really love that, but it turns
out, this problem with unseeded randomness being used is really quite
present and is going to take a long time to fix. Thus, as a compromise
between log-messages-for-all and nobody-knows, this is `default y`,
except it is also `depends on DEBUG_KERNEL`. This will ensure that the
curious see the messages while others don't have to.

Signed-off-by: Jason A. Donenfeld 
Signed-off-by: Theodore Ts'o 
---
Hi Ted,

This patch is meant to replace d06bfd1989fe97623b32d6df4ffa6e4338c99dc8,
which is currently in your dev tree. It switches from using `default y`
and `depends on DEBUG_KERNEL` to using the more simple `default DEBUG_KERNEL`.
This kind of change I think should satisfy most potential objections, by
being present for those who might find it useful, but invisble for those
who don't want the spam.

If you'd like to replace the earlier commit with this one, feel free. If
not, that's fine too.

Jason

 drivers/char/random.c | 15 +--
 lib/Kconfig.debug | 15 +++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 3853dd4f92e7..fa5bbd5a7ca0 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -288,7 +288,6 @@
 #define SEC_XFER_SIZE  512
 #define EXTRACT_SIZE   10
 
-#define DEBUG_RANDOM_BOOT 0
 
 #define LONGS(x) (((x) + sizeof(unsigned long) - 1)/sizeof(unsigned long))
 
@@ -1481,7 +1480,7 @@ void get_random_bytes(void *buf, int nbytes)
 {
__u8 tmp[CHACHA20_BLOCK_SIZE];
 
-#if DEBUG_RANDOM_BOOT > 0
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
if (!crng_ready())
printk(KERN_NOTICE "random: %pF get_random_bytes called "
   "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
@@ -2075,6 +2074,12 @@ u64 get_random_u64(void)
return ret;
 #endif
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+   if (!crng_ready())
+   printk(KERN_NOTICE "random: %pF get_random_u64 called "
+  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
batch = _cpu_var(batched_entropy_u64);
if (use_lock)
read_lock_irqsave(_entropy_reset_lock, flags);
@@ -2101,6 +2106,12 @@ u32 get_random_u32(void)
if (arch_get_random_int())
return ret;
 
+#ifdef CONFIG_WARN_UNSEEDED_RANDOM
+   if (!crng_ready())
+   printk(KERN_NOTICE "random: %pF get_random_u32 called "
+  "with crng_init = %d\n", (void *) _RET_IP_, crng_init);
+#endif
+
batch = _cpu_var(batched_entropy_u32);
if (use_lock)
read_lock_irqsave(_entropy_reset_lock, flags);
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index e4587ebe52c7..41cf12288369 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1209,6 +1209,21 @@ config STACKTRACE
  It is also used by various kernel debugging features that require
  stack trace generation.
 
+config WARN_UNSEEDED_RANDOM
+   bool "Warn when kernel uses unseeded randomness"
+   default DEBUG_KERNEL
+   help
+ Some parts of the kernel contain bugs relating to their use of
+ cryptographically secure random numbers before it's actually possible
+ to generate those numbers securely. This setting ensures that these
+ flaws don't go unnoticed, by enabling a message, should this ever
+ occur. This will allow people with obscure setups to know when things
+ are going wrong, so that they might contact developers about fixing
+ it.
+
+ Say Y here, unless you simply do not care about using unseeded
+ randomness and do not want a potential warning message in your logs.
+
 config DEBUG_KOBJECT
bool "kobject debugging"
depends on DEBUG_KERNEL
-- 
2.13.1