linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel
@ 2018-05-29 12:38 Prarit Bhargava
  2018-05-29 14:49 ` Kees Cook
  0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2018-05-29 12:38 UTC (permalink / raw)
  To: linux-kernel
  Cc: Prarit Bhargava, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Theodore Ts'o, Arnd Bergmann, Greg Kroah-Hartman,
	Rik van Riel, Andrew Morton, Philippe Ombredanne, Kees Cook,
	Jason A. Donenfeld, Kate Stewart

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel
  2018-05-29 12:38 [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel Prarit Bhargava
@ 2018-05-29 14:49 ` Kees Cook
  2018-05-29 15:01   ` Prarit Bhargava
  0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2018-05-29 14:49 UTC (permalink / raw)
  To: Prarit Bhargava
  Cc: LKML, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML,
	Theodore Ts'o, Arnd Bergmann, Greg Kroah-Hartman,
	Rik van Riel, Andrew Morton, Philippe Ombredanne,
	Jason A. Donenfeld, Kate Stewart

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel
  2018-05-29 14:49 ` Kees Cook
@ 2018-05-29 15:01   ` Prarit Bhargava
  2018-05-29 16:07     ` Theodore Y. Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2018-05-29 15:01 UTC (permalink / raw)
  To: Kees Cook
  Cc: LKML, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, X86 ML,
	Theodore Ts'o, Arnd Bergmann, Greg Kroah-Hartman,
	Rik van Riel, Andrew Morton, Philippe Ombredanne,
	Jason A. Donenfeld, Kate Stewart



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
>>
> 
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel
  2018-05-29 15:01   ` Prarit Bhargava
@ 2018-05-29 16:07     ` Theodore Y. Ts'o
  2018-05-29 16:58       ` Prarit Bhargava
  0 siblings, 1 reply; 6+ messages in thread
From: Theodore Y. Ts'o @ 2018-05-29 16:07 UTC (permalink / raw)
  To: Prarit Bhargava
  Cc: Kees Cook, LKML, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	X86 ML, Arnd Bergmann, Greg Kroah-Hartman, Rik van Riel,
	Andrew Morton, Philippe Ombredanne, Jason A. Donenfeld,
	Kate Stewart

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel
  2018-05-29 16:07     ` Theodore Y. Ts'o
@ 2018-05-29 16:58       ` Prarit Bhargava
  2018-05-29 18:19         ` hpa
  0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2018-05-29 16:58 UTC (permalink / raw)
  To: Theodore Y. Ts'o, Kees Cook, LKML, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, X86 ML, Arnd Bergmann,
	Greg Kroah-Hartman, Rik van Riel, Andrew Morton,
	Philippe Ombredanne, Jason A. Donenfeld, Kate Stewart



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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel
  2018-05-29 16:58       ` Prarit Bhargava
@ 2018-05-29 18:19         ` hpa
  0 siblings, 0 replies; 6+ messages in thread
From: hpa @ 2018-05-29 18:19 UTC (permalink / raw)
  To: Prarit Bhargava, Theodore Y. Ts'o, Kees Cook, LKML,
	Thomas Gleixner, Ingo Molnar, X86 ML, Arnd Bergmann,
	Greg Kroah-Hartman, Rik van Riel, Andrew Morton,
	Philippe Ombredanne, Jason A. Donenfeld, Kate Stewart

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-29 18:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-29 12:38 [PATCH] x86, random: Fix get_random_bytes() warning in x86 start_kernel Prarit Bhargava
2018-05-29 14:49 ` Kees Cook
2018-05-29 15:01   ` Prarit Bhargava
2018-05-29 16:07     ` Theodore Y. Ts'o
2018-05-29 16:58       ` Prarit Bhargava
2018-05-29 18:19         ` hpa

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).