Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 23 Apr 2018 09:21:48 +0200
From:   Salvatore Bonaccorso <carnil@debian.org>
To:     Ben Hutchings <ben@decadent.org.uk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, Theodore Tso <tytso@mit.edu>
Cc:     stable@vger.kernel.org, Jann Horn <jannh@google.com>,
        stable@kernel.org
Subject: Re: [PATCH 4.9 75/95] random: set up the NUMA crng instances after
 the CRNG is fully initialized
Message-ID: <20180423072148.jbnd2dodf47ffwej@lorien.valinor.li>
References: <20180422135210.432103639@linuxfoundation.org>
 <20180422135213.491879480@linuxfoundation.org>
 <49fa7d5f484a06f02946afec0688c33849e018de.camel@decadent.org.uk>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="cgtqqot7eikssngs"
Content-Disposition: inline
In-Reply-To: <49fa7d5f484a06f02946afec0688c33849e018de.camel@decadent.org.uk>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--cgtqqot7eikssngs
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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@mit.edu>
> > 
> > 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@google.com>
> > Fixes: 1e7f583af67b ("random: make /dev/urandom scalable for silly...")
> > Cc: stable@kernel.org # 4.8+
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> 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

--cgtqqot7eikssngs
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="random-silence-compiler-warnings-and-fix-race.patch"

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
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@zx2c4.com>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
[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)

--cgtqqot7eikssngs--