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

[Greg Kroah-Hartman]

On Mon, Apr 23, 2018 at 09:21:48AM +0200, Salvatore Bonaccorso wrote:
> Hi
> 
> On Sun, Apr 22, 2018 at 11:28:52PM +0100, Ben Hutchings wrote:
> > 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.
> 
> That is, the following test patch on top of the 4.9-stable review
> queue seem to resolve the issue. The commit message of the original
> commit 4a072c71f49b0a0e495ea13423bdb850da73c58c would though not match
> anymore.
> 
> Regards,
> Salvatore

> From: "Jason A. Donenfeld" <Jason@xxxxxxxxx>
> Date: Thu, 15 Jun 2017 00:45:26 +0200
> Subject: random: silence compiler warnings and fix race
> Origin: https://git.kernel.org/linus/4a072c71f49b0a0e495ea13423bdb850da73c58c
> 
> Odd versions of gcc for the sh4 architecture will actually warn about
> flags being used while uninitialized, so we set them to zero. Non crazy
> gccs will optimize that out again, so it doesn't make a difference.
> 
> Next, over aggressive gccs could inline the expression that defines
> use_lock, which could then introduce a race resulting in a lock
> imbalance. By using READ_ONCE, we prevent that fate. Finally, we make
> that assignment const, so that gcc can still optimize a nice amount.
> 
> Finally, we fix a potential deadlock between primary_crng.lock and
> batched_entropy_reset_lock, where they could be called in opposite
> order. Moving the call to invalidate_batched_entropy to outside the lock
> rectifies this issue.
> 
> Fixes: b169c13de473a85b3c859bb36216a4cb5f00a54a
> Signed-off-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> [Salvatore Bonaccorso: backport to 4.9: context changes, only apply change to
> address potential deadlock]
> ---
>  drivers/char/random.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Index: linux-stable/drivers/char/random.c
> ===================================================================
> --- linux-stable.orig/drivers/char/random.c
> +++ linux-stable/drivers/char/random.c
> @@ -810,12 +810,12 @@ static int crng_fast_load(const char *cp
>  		p[crng_init_cnt % CHACHA20_KEY_SIZE] ^= *cp;
>  		cp++; crng_init_cnt++; len--;
>  	}
> +	spin_unlock_irqrestore(&primary_crng.lock, flags);
>  	if (crng_init_cnt >= CRNG_INIT_CNT_THRESH) {
>  		crng_init = 1;
>  		wake_up_interruptible(&crng_init_wait);
>  		pr_notice("random: fast init done\n");
>  	}
> -	spin_unlock_irqrestore(&primary_crng.lock, flags);
>  	return 1;
>  }
>  
> @@ -873,6 +873,7 @@ static void crng_reseed(struct crng_stat
>  	}
>  	memzero_explicit(&buf, sizeof(buf));
>  	crng->init_time = jiffies;
> +	spin_unlock_irqrestore(&crng->lock, flags);
>  	if (crng == &primary_crng && crng_init < 2) {
>  		numa_crng_init();
>  		crng_init = 2;
> @@ -880,7 +881,6 @@ static void crng_reseed(struct crng_stat
>  		wake_up_interruptible(&crng_init_wait);
>  		pr_notice("random: crng init done\n");
>  	}
> -	spin_unlock_irqrestore(&crng->lock, flags);
>  }
>  
>  static inline void maybe_reseed_primary_crng(void)

As this depends on other patches, that are not in 4.9, I don't think
this will solve the problem correctly.  I've dropped the offending patch
for now, hopefully that should be good for the moment.

thanks,

greg k-h