[PATCH v2] x86, random: Fix get_random_bytes() warning in x86 start_kernel

[PATCH v2] x86, random: Fix get_random_bytes() warning in x86 start_kernel

[Prarit Bhargava]

After 43838a23a05f ("random: fix crng_ready() test") early boot calls to
get_random_bytes() will warn on x86 because the crng is not initialized.
For example,

random: get_random_bytes called from start_kernel+0x8e/0x587 with crng_init=0

x86 only uses get_random_bytes() for better randomization of the stack
canary value so the warning is of no consequence.

Test if the crng is initialized before calling get_random_bytes().  If it
is not available then attempt to read from the hardware random generator,
before finally using the TSC.

v2: Add HW random read based on feedback fro hpa@zytor.com & tytso@mit.edu

Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rik van Riel <riel@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Philippe Ombredanne <pombredanne@nexb.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
---
 arch/x86/include/asm/stackprotector.h | 14 +++++++++-----
 drivers/char/random.c                 |  5 ++++-
 include/linux/random.h                |  1 +
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
index 8ec97a62c245..082100608d18 100644
--- a/arch/x86/include/asm/stackprotector.h
+++ b/arch/x86/include/asm/stackprotector.h
@@ -62,17 +62,21 @@ static __always_inline void boot_init_stack_canary(void)
 {
 	u64 canary;
 	u64 tsc;
+	int ret;
 
 #ifdef CONFIG_X86_64
 	BUILD_BUG_ON(offsetof(union irq_stack_union, stack_canary) != 40);
 #endif
 	/*
-	 * We both use the random pool and the current TSC as a source
-	 * of randomness. The TSC only matters for very early init,
-	 * there it already has some randomness on most systems. Later
-	 * on during the bootup the random pool has true entropy too.
+	 * During early boot the entropy pool may not be initialized.  As an
+	 * alternative and if one is available, try to use the hardware random
+	 * generator.  On most systems the TSC will have some randomness so it
+	 * can also be used for entropy during early boot.
 	 */
-	get_random_bytes(&canary, sizeof(canary));
+	if (crng_ready())
+		get_random_bytes(&canary, sizeof(canary));
+	else
+		ret = get_random_bytes_arch(&canary, sizeof(canary));
 	tsc = rdtsc();
 	canary += tsc + (tsc << 32UL);
 	canary &= CANARY_MASK;
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 38c6d1af6d1c..ea6466a3ab14 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -428,7 +428,10 @@ struct crng_state primary_crng = {
  * its value (from 0->1->2).
  */
 static int crng_init = 0;
-#define crng_ready() (likely(crng_init > 1))
+int crng_ready(void)
+{
+	return likely(crng_init > 1);
+}
 static int crng_init_cnt = 0;
 static unsigned long crng_global_init_time = 0;
 #define CRNG_INIT_CNT_THRESH (2*CHACHA_KEY_SIZE)
diff --git a/include/linux/random.h b/include/linux/random.h
index 445a0ea4ff49..3b5919cb62ca 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -197,4 +197,5 @@ static inline u32 next_pseudo_random32(u32 seed)
 	return seed * 1664525 + 1013904223;
 }
 
+extern int crng_ready(void);
 #endif /* _LINUX_RANDOM_H */
-- 
2.17.2

Re: [PATCH v2] x86, random: Fix get_random_bytes() warning in x86 start_kernel

[Theodore Y. Ts'o]

On Fri, Feb 01, 2019 at 01:08:31PM -0500, Prarit Bhargava wrote:
> After 43838a23a05f ("random: fix crng_ready() test") early boot calls to
> get_random_bytes() will warn on x86 because the crng is not initialized.
> For example,
> 
> random: get_random_bytes called from start_kernel+0x8e/0x587 with crng_init=0
> 
> x86 only uses get_random_bytes() for better randomization of the stack
> canary value so the warning is of no consequence.
> 
> Test if the crng is initialized before calling get_random_bytes().  If it
> is not available then attempt to read from the hardware random generator,
> before finally using the TSC.

If you want to trust the CPU's hardware number generator, there is a
way to do this already.  Simply enable CONFIG_RANDOM_TRUST_CPU, or set
the boot command line option "random.trust_cpu=on".

Also, relying on the TSC for entropy is not something we should be
recommending.

So, NAK.

						- Ted

Re: [PATCH v2] x86, random: Fix get_random_bytes() warning in x86 start_kernel

[Prarit Bhargava]

On 2/1/19 10:02 PM, Theodore Y. Ts'o wrote:
> On Fri, Feb 01, 2019 at 01:08:31PM -0500, Prarit Bhargava wrote:
>> After 43838a23a05f ("random: fix crng_ready() test") early boot calls to
>> get_random_bytes() will warn on x86 because the crng is not initialized.
>> For example,
>>
>> random: get_random_bytes called from start_kernel+0x8e/0x587 with crng_init=0
>>
>> x86 only uses get_random_bytes() for better randomization of the stack
>> canary value so the warning is of no consequence.
>>
>> Test if the crng is initialized before calling get_random_bytes().  If it
>> is not available then attempt to read from the hardware random generator,
>> before finally using the TSC.
> 
> If you want to trust the CPU's hardware number generator, there is a
> way to do this already.  Simply enable CONFIG_RANDOM_TRUST_CPU, or set
> the boot command line option "random.trust_cpu=on".
> 
Ted, the bug I'm trying to fix is the warning:

random: get_random_bytes called from start_kernel+0x8e/0x587 with crng_init=0

during early boot.  Even with the kernel parameter the warning appears.


> Also, relying on the TSC for entropy is not something we should be
> recommending.

The current code uses the TSC.  It is not something new I'm introducing.

P.

> 
> So, NAK.
> 
> 						- Ted
> 

Re: [PATCH v2] x86, random: Fix get_random_bytes() warning in x86 start_kernel

[Theodore Y. Ts'o]

On Sun, Feb 03, 2019 at 08:09:37AM -0500, Prarit Bhargava wrote:
> Ted, the bug I'm trying to fix is the warning:
> 
> random: get_random_bytes called from start_kernel+0x8e/0x587 with crng_init=0
> 
> during early boot.  Even with the kernel parameter the warning appears.

Sometimes the warnings are real, and shouldn't be suppressed.  A
Debian maintainer once tried to suppress a compile-time warning, and
it was disastrous for security.  :-)

What line number is that corresponding to?  It sounds like something
is trying to use get_random_bytes() before the random driver was
initialized, and so the first question is does it really need to call
get_random_bytes() then or can it be moved?

> > Also, relying on the TSC for entropy is not something we should be
> > recommending.
> 
> The current code uses the TSC.  It is not something new I'm introducing.

But we don't *rely* on it.  That's a big difference.

       	     	       	    	     - Ted

Re: [PATCH v2] x86, random: Fix get_random_bytes() warning in x86 start_kernel

[Prarit Bhargava]

On 2/4/19 10:55 AM, Theodore Y. Ts'o wrote:
> On Sun, Feb 03, 2019 at 08:09:37AM -0500, Prarit Bhargava wrote:
>> Ted, the bug I'm trying to fix is the warning:
>>
>> random: get_random_bytes called from start_kernel+0x8e/0x587 with crng_init=0
>>
>> during early boot.  Even with the kernel parameter the warning appears.
> 
> Sometimes the warnings are real, and shouldn't be suppressed.  A
> Debian maintainer once tried to suppress a compile-time warning, and
> it was disastrous for security.  :-)
> 
> What line number is that corresponding to?  It sounds like something
> is trying to use get_random_bytes() before the random driver was
> initialized, and so the first question is does it really need to call
> get_random_bytes() then or can it be moved?
> 

Yes, that's exactly the case.  During early boot we initialize the boot cpu's
stack canary at arch/x86/include/asm/stackprotector.h:75 which is well before
the random driver is initialized.  The same code is called for all other cpus,
so perhaps not calling get_random_bytes() for the boot cpu is another option.

>>> Also, relying on the TSC for entropy is not something we should be
>>> recommending.
>>
>> The current code uses the TSC.  It is not something new I'm introducing.
> 
> But we don't *rely* on it.  That's a big difference.

Perhaps I'm confusing you by changing the comment.  I'm not changing any
behaviour wrt TSC.  The current code "relies" on the TSC as much as it did
before (all the way back through the git history).

P.

> 
>        	     	       	    	     - Ted
> 

Re: [PATCH v2] x86, random: Fix get_random_bytes() warning in x86 start_kernel

[Theodore Y. Ts'o]

On Fri, Feb 08, 2019 at 08:14:45AM -0500, Prarit Bhargava wrote:
> 
> Yes, that's exactly the case.  During early boot we initialize the boot cpu's
> stack canary at arch/x86/include/asm/stackprotector.h:75 which is well before
> the random driver is initialized.  The same code is called for all other cpus,
> so perhaps not calling get_random_bytes() for the boot cpu is another option.

The other thing we can do is to build some method of seeding the
random number generator into the boot loader.  This is what OpenBSD
does, and the advantage of solving this problem in that way it also
means that we have good randomness for things like kASLR.

There are two ways this can be done.  One is read entropy seed from
some kind of secure store (or at least, trusted store).  This couldt t
just be teaching grub to read from /var/local/lib/random-state.  (But
if we do this, as soon as possible, we should overwrite random-state
with new randomness the moment the CRNG is initialized.)  This is
basically what OpenBSD does.

The other thing that the bootloader might do is to get secure
cryptographic randomness from the boot environment.  For example, UEFI
has such a resource that can be tapped.

Either in the bootloader, or in the kernel, we could create stub
device drivers that try to connect to a TPM or some equivalent secure
chip (e.g., such as Gotogle Titan chip on certain Google hardware
platforms) that doesn't require bringing up full the kernel
infrastructure (especially if it's in the kernel, it can't depend on
kmalloc, or workqueues, etc.)  This is because it needs to run before
we relocate the kernel (since kASLR needs secure randomness).  So
effectively even if it's in the kernel, so it can more easily updated
to support new hardware, these stub drivers needs to be able to
function in a very similar limited set of runtime infrastructure, much
like something that might be in a bootloader.

> Perhaps I'm confusing you by changing the comment.  I'm not changing any
> behaviour wrt TSC.  The current code "relies" on the TSC as much as it did
> before (all the way back through the git history).

If we are only relying on TSC, we *must* not silence the warning.  To
the extent that your commit description talking about silencing
warnings, that's just raised all sorts of red flags.  Maybe I
misinterpreted your intent, but talking about relying on TSC is just
the wrong attitude if it's going to silence warnings.

Warnings are not necessarily bad.  Warnings can spur people to make
the kernel better, and in the case of the random driver, it may mean
doing work so we can improve the quality of the entropy that we
provide, especially at early boot.

So it's not about silencing warnigs.  The warning is a hint that we
should strive to do better.  It's much like pain.  Pain is gooid; it
tells you that something is wrong.  If someone has broken their leg,
the focus should not be on silencing the pain with morphine.  Maybe we
do need to do that, but if morphine is given without also setting the
broken leg and fixing the underlying problem that triggered the pain,
the painkillers can be worse the useless.

Sorry for really harping on this, but the philosophical bent which is
implied by the commit description is something really wrong, and it's
especially scary if it drives how we modify the code.  Please, let's
focus on improving the quality and the security of the randomness we
provide to the kernel --- not silencing warnings.

Thanks,

					- Ted