Re: [PATCH 4.9 75/95] random: set up the NUMA crng instances after the CRNG is fully initialized

[Ben Hutchings]

On Sun, 2018-04-22 at 15:53 +0200, Greg Kroah-Hartman wrote:
> 4.9-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Theodore Ts'o <tytso@xxxxxxx>
> 
> commit 8ef35c866f8862df074a49a93b0309725812dea8 upstream.
> 
> Until the primary_crng is fully initialized, don't initialize the NUMA
> crng nodes.  Otherwise users of /dev/urandom on NUMA systems before
> the CRNG is fully initialized can get very bad quality randomness.  Of
> course everyone should move to getrandom(2) where this won't be an
> issue, but there's a lot of legacy code out there.  This related to
> CVE-2018-1108.
> 
> Reported-by: Jann Horn <jannh@xxxxxxxxxx>
> Fixes: 1e7f583af67b ("random: make /dev/urandom scalable for silly...")
> Cc: stable@xxxxxxxxxx # 4.8+
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

In 4.9 (and probably older branches too) this leads to a deadlock:

crng_reseed(primary_crng, ...) takes primary_crng.lock
-> numa_rcng_init()
   -> crng_initialize()
      -> get_random_bytes()
         -> extract_crng()
            -> _extract_crng(primary_crng, ...) tries to take primary_crng.lock

I think this can be fixed by backporting commit 4a072c71f49b
"random: silence compiler warnings and fix race" but I'm not sure
whether that depends on other changes.

Ben.

> ---
>  drivers/char/random.c |   46 +++++++++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -818,6 +818,32 @@ static int crng_fast_load(const char *cp
>  	return 1;
>  }
>  
> +#ifdef CONFIG_NUMA
> +static void numa_crng_init(void)
> +{
> +	int i;
> +	struct crng_state *crng;
> +	struct crng_state **pool;
> +
> +	pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL|__GFP_NOFAIL);
> +	for_each_online_node(i) {
> +		crng = kmalloc_node(sizeof(struct crng_state),
> +				    GFP_KERNEL | __GFP_NOFAIL, i);
> +		spin_lock_init(&crng->lock);
> +		crng_initialize(crng);
> +		pool[i] = crng;
> +	}
> +	mb();
> +	if (cmpxchg(&crng_node_pool, NULL, pool)) {
> +		for_each_node(i)
> +			kfree(pool[i]);
> +		kfree(pool);
> +	}
> +}
> +#else
> +static void numa_crng_init(void) {}
> +#endif
> +
>  static void crng_reseed(struct crng_state *crng, struct entropy_store *r)
>  {
>  	unsigned long	flags;
> @@ -847,6 +873,7 @@ static void crng_reseed(struct crng_stat
>  	memzero_explicit(&buf, sizeof(buf));
>  	crng->init_time = jiffies;
>  	if (crng == &primary_crng && crng_init < 2) {
> +		numa_crng_init();
>  		crng_init = 2;
>  		process_random_ready_list();
>  		wake_up_interruptible(&crng_init_wait);
> @@ -1659,28 +1686,9 @@ static void init_std_data(struct entropy
>   */
>  static int rand_initialize(void)
>  {
> -#ifdef CONFIG_NUMA
> -	int i;
> -	struct crng_state *crng;
> -	struct crng_state **pool;
> -#endif
> -
>  	init_std_data(&input_pool);
>  	init_std_data(&blocking_pool);
>  	crng_initialize(&primary_crng);
> -
> -#ifdef CONFIG_NUMA
> -	pool = kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL|__GFP_NOFAIL);
> -	for_each_online_node(i) {
> -		crng = kmalloc_node(sizeof(struct crng_state),
> -				    GFP_KERNEL | __GFP_NOFAIL, i);
> -		spin_lock_init(&crng->lock);
> -		crng_initialize(crng);
> -		pool[i] = crng;
> -	}
> -	mb();
> -	crng_node_pool = pool;
> -#endif
>  	return 0;
>  }
>  early_initcall(rand_initialize);
> 
> 
-- 
Ben Hutchings
It is easier to write an incorrect program
than to understand a correct one.

Attachment: signature.asc
Description: This is a digitally signed message part