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

[PATCH] 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 each cpu 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.

Export crng_ready() for x86 and test if the crng is initialized before
calling get_random_bytes().

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 | 3 ++-
 drivers/char/random.c                 | 5 ++++-
 include/linux/random.h                | 1 +
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
index 371b3a4af000..4e2223aa34fc 100644
--- a/arch/x86/include/asm/stackprotector.h
+++ b/arch/x86/include/asm/stackprotector.h
@@ -72,7 +72,8 @@ static __always_inline void boot_init_stack_canary(void)
 	 * there it already has some randomness on most systems. Later
 	 * on during the bootup the random pool has true entropy too.
 	 */
-	get_random_bytes(&canary, sizeof(canary));
+	if (crng_ready())
+		get_random_bytes(&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 cd888d4ee605..003091d104bf 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*CHACHA20_KEY_SIZE)
diff --git a/include/linux/random.h b/include/linux/random.h
index 2ddf13b4281e..45616513abd9 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -196,4 +196,5 @@ static inline u32 next_pseudo_random32(u32 seed)
 	return seed * 1664525 + 1013904223;
 }
 
+extern int crng_ready(void);
 #endif /* _LINUX_RANDOM_H */
-- 
2.14.3

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

[Kees Cook]

On Tue, May 29, 2018 at 5:38 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> After 43838a23a05f ("random: fix crng_ready() test") early boot calls
> to get_random_bytes() will warn on each cpu 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.
>
> Export crng_ready() for x86 and test if the crng is initialized before
> calling get_random_bytes().

NAK. This leaves the stack canary with very little entropy. This needs
to pull from whatever pool is available, not skip it.

-Kees

>
> 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 | 3 ++-
>  drivers/char/random.c                 | 5 ++++-
>  include/linux/random.h                | 1 +
>  3 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
> index 371b3a4af000..4e2223aa34fc 100644
> --- a/arch/x86/include/asm/stackprotector.h
> +++ b/arch/x86/include/asm/stackprotector.h
> @@ -72,7 +72,8 @@ static __always_inline void boot_init_stack_canary(void)
>          * there it already has some randomness on most systems. Later
>          * on during the bootup the random pool has true entropy too.
>          */
> -       get_random_bytes(&canary, sizeof(canary));
> +       if (crng_ready())
> +               get_random_bytes(&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 cd888d4ee605..003091d104bf 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*CHACHA20_KEY_SIZE)
> diff --git a/include/linux/random.h b/include/linux/random.h
> index 2ddf13b4281e..45616513abd9 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -196,4 +196,5 @@ static inline u32 next_pseudo_random32(u32 seed)
>         return seed * 1664525 + 1013904223;
>  }
>
> +extern int crng_ready(void);
>  #endif /* _LINUX_RANDOM_H */
> --
> 2.14.3
>



-- 
Kees Cook
Pixel Security

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

[Prarit Bhargava]

On 05/29/2018 10:49 AM, Kees Cook wrote:
> On Tue, May 29, 2018 at 5:38 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> After 43838a23a05f ("random: fix crng_ready() test") early boot calls
>> to get_random_bytes() will warn on each cpu 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.
>>
>> Export crng_ready() for x86 and test if the crng is initialized before
>> calling get_random_bytes().
> 
> NAK. This leaves the stack canary with very little entropy. This needs
> to pull from whatever pool is available, not skip it.
> 

Kees, in early boot no pool is available so the stack canary is initialized from
the TSC.  Later in boot, the stack canary will use the the crng.

The existing comment in the code (cut-off in the patch below) reads

        /*
         * 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.
         */

ie) in early boot only TSC is okay, and late boot (when crng_ready() is true)
the pool will be used.

P.

> -Kees
> 
>>
>> 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 | 3 ++-
>>  drivers/char/random.c                 | 5 ++++-
>>  include/linux/random.h                | 1 +
>>  3 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/stackprotector.h b/arch/x86/include/asm/stackprotector.h
>> index 371b3a4af000..4e2223aa34fc 100644
>> --- a/arch/x86/include/asm/stackprotector.h
>> +++ b/arch/x86/include/asm/stackprotector.h
>> @@ -72,7 +72,8 @@ static __always_inline void boot_init_stack_canary(void)
>>          * there it already has some randomness on most systems. Later
>>          * on during the bootup the random pool has true entropy too.
>>          */
>> -       get_random_bytes(&canary, sizeof(canary));
>> +       if (crng_ready())
>> +               get_random_bytes(&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 cd888d4ee605..003091d104bf 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*CHACHA20_KEY_SIZE)
>> diff --git a/include/linux/random.h b/include/linux/random.h
>> index 2ddf13b4281e..45616513abd9 100644
>> --- a/include/linux/random.h
>> +++ b/include/linux/random.h
>> @@ -196,4 +196,5 @@ static inline u32 next_pseudo_random32(u32 seed)
>>         return seed * 1664525 + 1013904223;
>>  }
>>
>> +extern int crng_ready(void);
>>  #endif /* _LINUX_RANDOM_H */
>> --
>> 2.14.3
>>
> 
> 
> 

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

[Theodore Y. Ts'o]

On Tue, May 29, 2018 at 11:01:07AM -0400, Prarit Bhargava wrote:
> Kees, in early boot no pool is available so the stack canary is initialized from
> the TSC.  Later in boot, the stack canary will use the the crng.
> 
> ie) in early boot only TSC is okay, and late boot (when crng_ready() is true)
> the pool will be used.

But that means all of the kernel threads (e.g., workqueues, et. al)
would not be well protected by the stack canary.  That
seems.... rather unfortunate.

      	     	  	      	   	     - Ted

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

[Prarit Bhargava]

On 05/29/2018 12:07 PM, Theodore Y. Ts'o wrote:
> On Tue, May 29, 2018 at 11:01:07AM -0400, Prarit Bhargava wrote:
>> Kees, in early boot no pool is available so the stack canary is initialized from
>> the TSC.  Later in boot, the stack canary will use the the crng.
>>
>> ie) in early boot only TSC is okay, and late boot (when crng_ready() is true)
>> the pool will be used.
> 
> But that means all of the kernel threads (e.g., workqueues, et. al)
> would not be well protected by the stack canary.  That
> seems.... rather unfortunate.

Well, as stated the TSC is used as a source of entropy in early boot.  It's
always been that way and get_random_bytes() AFAICT has always returned 0.  CPUs
added later on via hotplug do use get_random_bytes().

Does anyone cc'd have a better idea on how to get another source of entropy this
early in boot?

P.

> 
>       	     	  	      	   	     - Ted
> 

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

[hpa]

On May 29, 2018 9:58:10 AM PDT, Prarit Bhargava <prarit@redhat.com> wrote:
>
>
>On 05/29/2018 12:07 PM, Theodore Y. Ts'o wrote:
>> On Tue, May 29, 2018 at 11:01:07AM -0400, Prarit Bhargava wrote:
>>> Kees, in early boot no pool is available so the stack canary is
>initialized from
>>> the TSC.  Later in boot, the stack canary will use the the crng.
>>>
>>> ie) in early boot only TSC is okay, and late boot (when crng_ready()
>is true)
>>> the pool will be used.
>> 
>> But that means all of the kernel threads (e.g., workqueues, et. al)
>> would not be well protected by the stack canary.  That
>> seems.... rather unfortunate.
>
>Well, as stated the TSC is used as a source of entropy in early boot. 
>It's
>always been that way and get_random_bytes() AFAICT has always returned
>0.  CPUs
>added later on via hotplug do use get_random_bytes().
>
>Does anyone cc'd have a better idea on how to get another source of
>entropy this
>early in boot?
>
>P.
>
>> 
>>       	     	  	      	   	     - Ted
>> 

RDRAND/RDSEED for newer x86 processors.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.