Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 4 Apr 2022 11:49:50 -0700
From:   Kees Cook <keescook@chromium.org>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc:     linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, PaX Team <pageexec@freemail.hu>
Subject: Re: [PATCH v2] gcc-plugins: latent_entropy: use /dev/urandom
Message-ID: <202204041144.96FC64A8@keescook>
References: <CAHmME9otYi4pCzZwSGnK40dp1QMRVPxp+DBysVuLXUKkXinAxg@mail.gmail.com>
 <20220403204036.1269562-1-Jason@zx2c4.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220403204036.1269562-1-Jason@zx2c4.com>
Precedence: bulk

On Sun, Apr 03, 2022 at 10:40:36PM +0200, Jason A. Donenfeld wrote:
> While the latent entropy plugin mostly doesn't derive entropy from
> get_random_const() for measuring the call graph, when __latent_entropy is
> applied to a constant, then it's initialized statically to output from
> get_random_const(). In that case, this data is derived from a 64-bit
> seed, which means a buffer of 512 bits doesn't really have that amount
> of compile-time entropy.
> 
> This patch fixes that shortcoming by just buffering chunks of
> /dev/urandom output and doling it out as requested.
> 
> At the same time, it's important that we don't break the use of
> -frandom-seed, for people who want the runtime benefits of the latent
> entropy plugin, while still having compile-time determinism. In that
> case, we detect whether gcc's set_random_seed() has been called by
> making a call to get_random_seed(noinit=true) in the plugin init
> function, which is called after set_random_seed() is called but before
> anything that calls get_random_seed(noinit=false), and seeing if it's
> zero or not. If it's not zero, we're in deterministic mode, and so we
> just generate numbers with a basic xorshift prng.

This mixes two changes: the pRNG change and the "use urandom if
non-deterministic" change. I think these should be split, so the pRNG
change can be explicitly justified.

More notes below...

> 
> Fixes: 38addce8b600 ("gcc-plugins: Add latent_entropy plugin")
> Cc: stable@vger.kernel.org
> Cc: PaX Team <pageexec@freemail.hu>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Changes v1->v2:
> - Pipacs pointed out that using /dev/urandom unconditionally would break
>   the use of -frandom-seed, so now we check for that and keep with
>   something deterministic in that case.
> 
> I'm not super familiar with this plugin or its conventions, so pointers
> would be most welcome if something here looks amiss. The decision to
> buffer 2k at a time is pretty arbitrary too; I haven't measured usage.
> 
>  scripts/gcc-plugins/latent_entropy_plugin.c | 48 +++++++++++++--------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff --git a/scripts/gcc-plugins/latent_entropy_plugin.c b/scripts/gcc-plugins/latent_entropy_plugin.c
> index 589454bce930..042442013ae1 100644
> --- a/scripts/gcc-plugins/latent_entropy_plugin.c
> +++ b/scripts/gcc-plugins/latent_entropy_plugin.c
> @@ -82,29 +82,37 @@ __visible int plugin_is_GPL_compatible;
>  static GTY(()) tree latent_entropy_decl;
>  
>  static struct plugin_info latent_entropy_plugin_info = {
> -	.version	= "201606141920vanilla",
> +	.version	= "202203311920vanilla",

This doesn't really need to be versioned. We can change this to just
"vanilla", IMO.

>  	.help		= "disable\tturn off latent entropy instrumentation\n",
>  };
>  
> -static unsigned HOST_WIDE_INT seed;
> -/*
> - * get_random_seed() (this is a GCC function) generates the seed.
> - * This is a simple random generator without any cryptographic security because
> - * the entropy doesn't come from here.
> - */
> +static unsigned HOST_WIDE_INT deterministic_seed;
> +static unsigned HOST_WIDE_INT rnd_buf[256];
> +static size_t rnd_idx = ARRAY_SIZE(rnd_buf);
> +static int urandom_fd = -1;
> +
>  static unsigned HOST_WIDE_INT get_random_const(void)
>  {
> -	unsigned int i;
> -	unsigned HOST_WIDE_INT ret = 0;
> -
> -	for (i = 0; i < 8 * sizeof(ret); i++) {
> -		ret = (ret << 1) | (seed & 1);
> -		seed >>= 1;
> -		if (ret & 1)
> -			seed ^= 0xD800000000000000ULL;
> +	if (deterministic_seed) {
> +		unsigned HOST_WIDE_INT w = deterministic_seed;
> +		w ^= w << 13;
> +		w ^= w >> 7;
> +		w ^= w << 17;
> +		deterministic_seed = w;
> +		return deterministic_seed;

While seemingly impossible, perhaps don't reset "deterministic_seed",
and just continue to use "seed", so that it can never become "0" again.

>  	}
>  
> -	return ret;
> +	if (urandom_fd < 0) {
> +		urandom_fd = open("/dev/urandom", O_RDONLY);
> +		if (urandom_fd < 0)
> +			abort();
> +	}
> +	if (rnd_idx >= ARRAY_SIZE(rnd_buf)) {
> +		if (read(urandom_fd, rnd_buf, sizeof(rnd_buf)) != sizeof(rnd_buf))
> +			abort();
> +		rnd_idx = 0;
> +	}
> +	return rnd_buf[rnd_idx++];
>  }
>  
>  static tree tree_get_random_const(tree type)
> @@ -537,8 +545,6 @@ static void latent_entropy_start_unit(void *gcc_data __unused,
>  	tree type, id;
>  	int quals;
>  
> -	seed = get_random_seed(false);
> -
>  	if (in_lto_p)
>  		return;
>  
> @@ -573,6 +579,12 @@ __visible int plugin_init(struct plugin_name_args *plugin_info,
>  	const struct plugin_argument * const argv = plugin_info->argv;
>  	int i;
>  
> +	/*
> +	 * Call get_random_seed() with noinit=true, so that this returns
> +	 * 0 in the case where no seed has been passed via -frandom-seed.
> +	 */
> +	deterministic_seed = get_random_seed(true);

i.e. have this be:

	deterministic_seed = get_random_seed(true);
	if (deterministic_seed)
		seed = get_random_seed(false);

> +
>  	static const struct ggc_root_tab gt_ggc_r_gt_latent_entropy[] = {
>  		{
>  			.base = &latent_entropy_decl,
> -- 
> 2.35.1
> 

-- 
Kees Cook