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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>> Cc: x86@xxxxxxxxxx
>> Cc: "Theodore Ts'o" <tytso@xxxxxxx>
>> Cc: Arnd Bergmann <arnd@xxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> Cc: Rik van Riel <riel@xxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: Philippe Ombredanne <pombredanne@xxxxxxxx>
>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>> Cc: Prarit Bhargava <prarit@xxxxxxxxxx>
>> Cc: "Jason A. Donenfeld" <Jason@xxxxxxxxx>
>> Cc: Kate Stewart <kstewart@xxxxxxxxxxxxxxxxxxx>
>> ---
>>  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
>>
> 
> 
>