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;
Message-ID: <49fa7d5f484a06f02946afec0688c33849e018de.camel@decadent.org.uk>
Subject: Re: [PATCH 4.9 75/95] random: set up the NUMA crng instances after
 the CRNG is fully initialized
From:   Ben Hutchings <ben@decadent.org.uk>
To:     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, Salvatore Bonaccorso <carnil@debian.org>
Date:   Sun, 22 Apr 2018 23:28:52 +0100
In-Reply-To: <20180422135213.491879480@linuxfoundation.org>
References: <20180422135210.432103639@linuxfoundation.org>
         <20180422135213.491879480@linuxfoundation.org>
Content-Type: multipart/signed; micalg="pgp-sha512";
        protocol="application/pgp-signature"; boundary="=-vihau1uszm3Xy1rY40DC"
Mime-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--=-vihau1uszm3Xy1rY40DC
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 kno=
w.
>=20
> ------------------
>=20
> From: Theodore Ts'o <tytso@mit.edu>
>=20
> commit 8ef35c866f8862df074a49a93b0309725812dea8 upstream.
>=20
> 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.
>=20
> 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.

Ben.

> ---
>  drivers/char/random.c |   46 +++++++++++++++++++++++++++----------------=
---
>  1 file changed, 27 insertions(+), 19 deletions(-)
>=20
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -818,6 +818,32 @@ static int crng_fast_load(const char *cp
>  	return 1;
>  }
> =20
> +#ifdef CONFIG_NUMA
> +static void numa_crng_init(void)
> +{
> +	int i;
> +	struct crng_state *crng;
> +	struct crng_state **pool;
> +
> +	pool =3D kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL|__GFP_NOFAIL);
> +	for_each_online_node(i) {
> +		crng =3D kmalloc_node(sizeof(struct crng_state),
> +				    GFP_KERNEL | __GFP_NOFAIL, i);
> +		spin_lock_init(&crng->lock);
> +		crng_initialize(crng);
> +		pool[i] =3D 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 =3D jiffies;
>  	if (crng =3D=3D &primary_crng && crng_init < 2) {
> +		numa_crng_init();
>  		crng_init =3D 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 =3D kcalloc(nr_node_ids, sizeof(*pool), GFP_KERNEL|__GFP_NOFAIL);
> -	for_each_online_node(i) {
> -		crng =3D kmalloc_node(sizeof(struct crng_state),
> -				    GFP_KERNEL | __GFP_NOFAIL, i);
> -		spin_lock_init(&crng->lock);
> -		crng_initialize(crng);
> -		pool[i] =3D crng;
> -	}
> -	mb();
> -	crng_node_pool =3D pool;
> -#endif
>  	return 0;
>  }
>  early_initcall(rand_initialize);
>=20
>=20
--=20
Ben Hutchings
It is easier to write an incorrect program
than to understand a correct one.

--=-vihau1uszm3Xy1rY40DC
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEErCspvTSmr92z9o8157/I7JWGEQkFAlrdDKQACgkQ57/I7JWG
EQmiTBAAvsz01Ar4TLlzvgLUQSchwqO9Dm18f+pdk/T6CgVKUFdVIfcUl1lIzvsk
aSIuqimF6Tq9KceAzZAuPYPy6gEiMMkst7XGWHSY245fbhgy+ticsnJ2fDUkAbVH
XVaCkJoUd2gYk54IzmZwwuW/9n/EEufoKKSkDNrJkKoijZGIvUyXjsRsRHRHmHKp
SrTYhFdrpci4SnmWp6DcJXgrMcjyj1WmiKkWCkVbDgibC7ul+pF6mfO/ziuQEuK4
Ym3ktFwMwqhuBR/e56CqZ1Te9urkSJuXBSD4mhScL5HWdZUkjfJD2VeXd7xcsgXD
ilknnGXBChHwnwtZwVgwXjDQ/QUHqP9Vso5xiP5OaadZymRm98pF2B1lnobXJM0L
PvbZ/pWzVzDSaby6OaKTz1t1g5RANLoh+cVvWU1Z/9kpOrFv7ZHfTqsusXaFAWTF
vb/HtRCVS9BqB37+5KxT20Bl7B+0NF9WktPCD1+KNGRp5jCmlI7OQMh8Uu9ASO5z
olLG59Fu4DDMw9Jp8qwH9KrKCs8/oCOHYDr6dJTCsKAShTT/43ITmoVMZJSP4/2Q
4tryMtBpZzXBJLBj0H9WPA+2Jv5jMSQDAfa0IRfgRJHqKLIaPJtiIa5IKLcz3SZF
VVc8mR1cZnpSwD8SX4AJLRA7URkVD4hUcWCreATEHVeT87uaoZs=
=zlmL
-----END PGP SIGNATURE-----

--=-vihau1uszm3Xy1rY40DC--