[PATCH] random: add rng-seed= command line option

[PATCH] random: add rng-seed= command line option

[Mark Salyzyn]

A followup to commit 428826f5358c922dc378830a1717b682c0823160
("fdt: add support for rng-seed") to extend what was started
with Open Firmware (OF or Device Tree) parsing, but also add
it to the command line.

If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
command line option length as added trusted entropy.

Always rrase all views of the rng-seed option, except early command
line parsing, to prevent leakage to applications or modules, to
eliminate any attack vector.

It is preferred to add rng-seed to the Device Tree, but some
platforms do not have this option, so this adds the ability to
provide some command-line-limited data to the entropy through this
alternate mechanism.  Expect all 8 bits to be used, but must exclude
space to be accounted in the command line.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
---
 drivers/char/random.c  |  8 +++++
 include/linux/random.h |  5 +++
 init/main.c            | 73 +++++++++++++++++++++++++++++++++++-------
 3 files changed, 74 insertions(+), 12 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index c7f9584de2c8b..2f386e411fb7b 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2311,3 +2311,11 @@ void add_bootloader_randomness(const void *buf, unsigned int size)
 		add_device_randomness(buf, size);
 }
 EXPORT_SYMBOL_GPL(add_bootloader_randomness);
+
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+/* caller called add_device_randomness, but it is from a trusted source */
+void __init credit_trusted_entropy(unsigned int size)
+{
+	credit_entropy_bits(&input_pool, size * 8);
+}
+#endif
diff --git a/include/linux/random.h b/include/linux/random.h
index d319f9a1e4290..1e09eeadc613c 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -20,6 +20,11 @@ struct random_ready_callback {
 
 extern void add_device_randomness(const void *, unsigned int);
 extern void add_bootloader_randomness(const void *, unsigned int);
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+extern void __init credit_trusted_entropy(unsigned int b);
+#else
+static inline void credit_trusted_entropy(unsigned int b) {}
+#endif
 
 #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
 static inline void add_latent_entropy(void)
diff --git a/init/main.c b/init/main.c
index cc0ee4873419c..ae976b2dea5dc 100644
--- a/init/main.c
+++ b/init/main.c
@@ -524,24 +524,53 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { }
  * parsing is performed in place, and we should allow a component to
  * store reference of name/value for future reference.
  */
+static const char rng_seed_str[] __initconst = "rng-seed=";
+/* try to clear rng-seed so it won't be found by user applications. */
+static void __init copy_command_line(char *dest, char *src, size_t r)
+{
+	char *rng_seed = strnstr(src, rng_seed_str, r);
+
+	if (rng_seed) {
+		size_t l = rng_seed - src;
+		char *end;
+
+		memcpy(dest, src, l);
+		dest += l;
+		src = rng_seed + strlen(rng_seed_str);
+		r -= l + strlen(rng_seed_str);
+		end = strnchr(src, r, ' ');
+		if (end) {
+			if (l && rng_seed[-1] == ' ')
+				++end;
+			r -= end - src;
+			src = end;
+		}
+	}
+	memcpy(dest, src, r);
+	dest[r] = '\0';
+}
+
 static void __init setup_command_line(char *command_line)
 {
 	size_t len, xlen = 0, ilen = 0;
+	static const char argsep_str[] __initconst = " -- ";
+	static const char alloc_fail_msg[] __initconst =
+		"%s: Failed to allocate %zu bytes\n";
 
 	if (extra_command_line)
 		xlen = strlen(extra_command_line);
 	if (extra_init_args)
-		ilen = strlen(extra_init_args) + 4; /* for " -- " */
+		ilen = strlen(extra_init_args) + strlen(argsep_str);
 
-	len = xlen + strlen(boot_command_line) + 1;
+	len = xlen + strnlen(boot_command_line, sizeof(boot_command_line)) + 1;
 
 	saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES);
 	if (!saved_command_line)
-		panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen);
+		panic(alloc_fail_msg, __func__, len + ilen);
 
 	static_command_line = memblock_alloc(len, SMP_CACHE_BYTES);
 	if (!static_command_line)
-		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
+		panic(alloc_fail_msg, __func__, len);
 
 	if (xlen) {
 		/*
@@ -549,11 +578,14 @@ static void __init setup_command_line(char *command_line)
 		 * lines because there could be dashes (separator of init
 		 * command line) in the command lines.
 		 */
-		strcpy(saved_command_line, extra_command_line);
-		strcpy(static_command_line, extra_command_line);
+		copy_command_line(saved_command_line, extra_command_line, xlen);
+		copy_command_line(static_command_line, extra_command_line,
+				  xlen);
 	}
-	strcpy(saved_command_line + xlen, boot_command_line);
-	strcpy(static_command_line + xlen, command_line);
+	copy_command_line(saved_command_line + xlen, boot_command_line,
+			  len - xlen - 1);
+	copy_command_line(static_command_line + xlen, command_line,
+			  len - xlen - 1);
 
 	if (ilen) {
 		/*
@@ -562,13 +594,15 @@ static void __init setup_command_line(char *command_line)
 		 * to init.
 		 */
 		len = strlen(saved_command_line);
-		if (!strstr(boot_command_line, " -- ")) {
-			strcpy(saved_command_line + len, " -- ");
-			len += 4;
+		if (!strnstr(boot_command_line, argsep_str,
+			     sizeof(boot_command_line))) {
+			strcpy(saved_command_line + len, argsep_str);
+			len += strlen(argsep_str);
 		} else
 			saved_command_line[len++] = ' ';
 
-		strcpy(saved_command_line + len, extra_init_args);
+		copy_command_line(saved_command_line + len, extra_init_args,
+				  ilen - strlen(argsep_str));
 	}
 }
 
@@ -875,6 +909,21 @@ asmlinkage __visible void __init start_kernel(void)
 	rand_initialize();
 	add_latent_entropy();
 	add_device_randomness(command_line, strlen(command_line));
+	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
+		size_t l = strlen(command_line);
+		char *rng_seed = strnstr(command_line, rng_seed_str, l);
+
+		if (rng_seed) {
+			char *end;
+
+			rng_seed += strlen(rng_seed_str);
+			l -= rng_seed - command_line;
+			end = strnchr(rng_seed, l, ' ');
+			if (end)
+				l = end - rng_seed;
+			credit_trusted_entropy(l);
+		}
+	}
 	boot_init_stack_canary();
 
 	time_init();
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH] random: add rng-seed= command line option

[Theodore Y. Ts'o]

What was the base of your patch?  It's not applying on my kernel tree.

On Fri, Feb 07, 2020 at 07:07:59AM -0800, Mark Salyzyn wrote:
> A followup to commit 428826f5358c922dc378830a1717b682c0823160
> ("fdt: add support for rng-seed") to extend what was started
> with Open Firmware (OF or Device Tree) parsing, but also add
> it to the command line.
> 
> If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
> command line option length as added trusted entropy.
> 
> Always rrase all views of the rng-seed option, except early command
> line parsing, to prevent leakage to applications or modules, to
> eliminate any attack vector.

s/rrase/erase/

> 
> It is preferred to add rng-seed to the Device Tree, but some
> platforms do not have this option, so this adds the ability to
> provide some command-line-limited data to the entropy through this
> alternate mechanism.  Expect all 8 bits to be used, but must exclude
> space to be accounted in the command line.

"all 8 bits"?

> @@ -875,6 +909,21 @@ asmlinkage __visible void __init start_kernel(void)
>  	rand_initialize();
>  	add_latent_entropy();
>  	add_device_randomness(command_line, strlen(command_line));
> +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
> +		size_t l = strlen(command_line);
> +		char *rng_seed = strnstr(command_line, rng_seed_str, l);
> +
> +		if (rng_seed) {
> +			char *end;
> +
> +			rng_seed += strlen(rng_seed_str);
> +			l -= rng_seed - command_line;
> +			end = strnchr(rng_seed, l, ' ');
> +			if (end)
> +				l = end - rng_seed;
> +			credit_trusted_entropy(l);
> +		}
> +	}

This doesn't look right at all.  It calls credit_trusted_entropy(),
but it doesn't actually feed the contents of rng_seed where.  Why not
just call add_hwgeneterator_randomness() and drop adding this
credit_trusted_entropy(l)?

						- Ted

Re: [PATCH] random: add rng-seed= command line option

[Kees Cook]

On Fri, Feb 07, 2020 at 07:07:59AM -0800, Mark Salyzyn wrote:
> +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
> +/* caller called add_device_randomness, but it is from a trusted source */
> +void __init credit_trusted_entropy(unsigned int size)
> +{
> +	credit_entropy_bits(&input_pool, size * 8);
> +}
> +#endif

As Ted already mentioned, I was expecting the string contents to actually
get added somewhere. Is the idea that it's already been added via the
add_device_randomness(command_line) call, and you just want to explicitly
credit those bytes? If so, that deserves a comment, and I think it should
likely not use 8 bits per character, as that's not how many bits are
possible for an alphanumeric string character; I would expect 6 bits (~32
standard letter/number) -- this likely needs fixing in the fdt patch too.

> diff --git a/include/linux/random.h b/include/linux/random.h
> index d319f9a1e4290..1e09eeadc613c 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -20,6 +20,11 @@ struct random_ready_callback {
>  
>  extern void add_device_randomness(const void *, unsigned int);
>  extern void add_bootloader_randomness(const void *, unsigned int);
> +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
> +extern void __init credit_trusted_entropy(unsigned int b);
> +#else
> +static inline void credit_trusted_entropy(unsigned int b) {}
> +#endif
>  
>  #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
>  static inline void add_latent_entropy(void)
> diff --git a/init/main.c b/init/main.c
> index cc0ee4873419c..ae976b2dea5dc 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -524,24 +524,53 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { }
>   * parsing is performed in place, and we should allow a component to
>   * store reference of name/value for future reference.
>   */
> +static const char rng_seed_str[] __initconst = "rng-seed=";
> +/* try to clear rng-seed so it won't be found by user applications. */
> +static void __init copy_command_line(char *dest, char *src, size_t r)
> +{
> +	char *rng_seed = strnstr(src, rng_seed_str, r);
> +
> +	if (rng_seed) {
> +		size_t l = rng_seed - src;
> +		char *end;
> +
> +		memcpy(dest, src, l);
> +		dest += l;
> +		src = rng_seed + strlen(rng_seed_str);
> +		r -= l + strlen(rng_seed_str);
> +		end = strnchr(src, r, ' ');
> +		if (end) {
> +			if (l && rng_seed[-1] == ' ')
> +				++end;
> +			r -= end - src;
> +			src = end;
> +		}
> +	}
> +	memcpy(dest, src, r);
> +	dest[r] = '\0';
> +}
> +
>  static void __init setup_command_line(char *command_line)
>  {
>  	size_t len, xlen = 0, ilen = 0;
> +	static const char argsep_str[] __initconst = " -- ";
> +	static const char alloc_fail_msg[] __initconst =
> +		"%s: Failed to allocate %zu bytes\n";

There's some refactoring in this patch unrelated to the seed logic. Can
you split that out? (I think these changes are good.)

>  
>  	if (extra_command_line)
>  		xlen = strlen(extra_command_line);

Unrelated note: whoa this is based on linux-next which has a massive
change to the boot command line handling and appears to be doing some
bad things. I need to go find that thread...

>  	if (extra_init_args)
> -		ilen = strlen(extra_init_args) + 4; /* for " -- " */
> +		ilen = strlen(extra_init_args) + strlen(argsep_str);
>  
> -	len = xlen + strlen(boot_command_line) + 1;
> +	len = xlen + strnlen(boot_command_line, sizeof(boot_command_line)) + 1;
>  
>  	saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES);
>  	if (!saved_command_line)
> -		panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen);
> +		panic(alloc_fail_msg, __func__, len + ilen);
>  
>  	static_command_line = memblock_alloc(len, SMP_CACHE_BYTES);
>  	if (!static_command_line)
> -		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
> +		panic(alloc_fail_msg, __func__, len);
>  
>  	if (xlen) {
>  		/*
> @@ -549,11 +578,14 @@ static void __init setup_command_line(char *command_line)
>  		 * lines because there could be dashes (separator of init
>  		 * command line) in the command lines.
>  		 */
> -		strcpy(saved_command_line, extra_command_line);
> -		strcpy(static_command_line, extra_command_line);
> +		copy_command_line(saved_command_line, extra_command_line, xlen);
> +		copy_command_line(static_command_line, extra_command_line,
> +				  xlen);
>  	}
> -	strcpy(saved_command_line + xlen, boot_command_line);
> -	strcpy(static_command_line + xlen, command_line);
> +	copy_command_line(saved_command_line + xlen, boot_command_line,
> +			  len - xlen - 1);
> +	copy_command_line(static_command_line + xlen, command_line,
> +			  len - xlen - 1);
>  
>  	if (ilen) {
>  		/*
> @@ -562,13 +594,15 @@ static void __init setup_command_line(char *command_line)
>  		 * to init.
>  		 */
>  		len = strlen(saved_command_line);
> -		if (!strstr(boot_command_line, " -- ")) {
> -			strcpy(saved_command_line + len, " -- ");
> -			len += 4;
> +		if (!strnstr(boot_command_line, argsep_str,
> +			     sizeof(boot_command_line))) {
> +			strcpy(saved_command_line + len, argsep_str);
> +			len += strlen(argsep_str);
>  		} else
>  			saved_command_line[len++] = ' ';
>  
> -		strcpy(saved_command_line + len, extra_init_args);
> +		copy_command_line(saved_command_line + len, extra_init_args,
> +				  ilen - strlen(argsep_str));
>  	}
>  }
>  
> @@ -875,6 +909,21 @@ asmlinkage __visible void __init start_kernel(void)
>  	rand_initialize();
>  	add_latent_entropy();
>  	add_device_randomness(command_line, strlen(command_line));
> +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
> +		size_t l = strlen(command_line);
> +		char *rng_seed = strnstr(command_line, rng_seed_str, l);
> +
> +		if (rng_seed) {
> +			char *end;
> +
> +			rng_seed += strlen(rng_seed_str);
> +			l -= rng_seed - command_line;
> +			end = strnchr(rng_seed, l, ' ');
> +			if (end)
> +				l = end - rng_seed;
> +			credit_trusted_entropy(l);
> +		}
> +	}

Can you pull this out of line and write a new static help that does all
of the rng stuff here? Basically from rand_initialize() through
boot_init_stack_canary(), so it's all in one place and not "open coded"
in start_kernel(). (And then, actually, you don't need a separate
credit_trusted_entropy() function at all -- just call
credit_entropy_bits() directly there (and add a comment about the
command line already getting added).

>  	boot_init_stack_canary();
>  
>  	time_init();
> -- 
> 2.25.0.341.g760bfbb309-goog
> 

-- 
Kees Cook

Re: [PATCH] random: add rng-seed= command line option

[Steven Rostedt]

On Fri, 7 Feb 2020 09:28:27 -0800
Kees Cook <keescook@chromium.org> wrote:

> >  static void __init setup_command_line(char *command_line)
> >  {
> >  	size_t len, xlen = 0, ilen = 0;
> > +	static const char argsep_str[] __initconst = " -- ";
> > +	static const char alloc_fail_msg[] __initconst =
> > +		"%s: Failed to allocate %zu bytes\n";  
> 
> There's some refactoring in this patch unrelated to the seed logic. Can
> you split that out? (I think these changes are good.)
> 
> >  
> >  	if (extra_command_line)
> >  		xlen = strlen(extra_command_line);  
> 
> Unrelated note: whoa this is based on linux-next which has a massive
> change to the boot command line handling and appears to be doing some
> bad things. I need to go find that thread...

This would be Masami's bootconfig work which Linus already pulled in.
What bad things are there? You can see the thread here:

 http://lore.kernel.org/r/157867220019.17873.13377985653744804396.stgit@devnote2
 http://lore.kernel.org/r/CAHk-=wjfjO+h6bQzrTf=YCZA53Y3EDyAs3Z4gEsT7icA3u_Psw@mail.gmail.com

-- Steve

Re: [PATCH] random: add rng-seed= command line option

[Mark Salyzyn]

On 2/7/20 7:58 AM, Theodore Y. Ts'o wrote:
> What was the base of your patch?  It's not applying on my kernel tree.
>
> On Fri, Feb 07, 2020 at 07:07:59AM -0800, Mark Salyzyn wrote:
>> A followup to commit 428826f5358c922dc378830a1717b682c0823160
>> ("fdt: add support for rng-seed") to extend what was started
>> with Open Firmware (OF or Device Tree) parsing, but also add
>> it to the command line.
>>
>> If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
>> command line option length as added trusted entropy.
>>
>> Always rrase all views of the rng-seed option, except early command
>> line parsing, to prevent leakage to applications or modules, to
>> eliminate any attack vector.
> s/rrase/erase/
Noticed that immediately after posting, figured there would be another 
round ;-}
>
>> It is preferred to add rng-seed to the Device Tree, but some
>> platforms do not have this option, so this adds the ability to
>> provide some command-line-limited data to the entropy through this
>> alternate mechanism.  Expect all 8 bits to be used, but must exclude
>> space to be accounted in the command line.
> "all 8 bits"?

Command line (and Device Tree for that matter) can provide 8-bits of 
data, and specifically for the command line as long as they skip space 
and nul characters, we will be stripping the content out of the command 
line because we strip it from view, so that no one gets hot and bothered.

I expected this to be contentious, this is why I call it out. Does 
_anyone_ have a disagreement with allowing raw data (minus nul and space 
characters) to be part of the rng-seed?

>
>> @@ -875,6 +909,21 @@ asmlinkage __visible void __init start_kernel(void)
>>   	rand_initialize();
>>   	add_latent_entropy();
>>   	add_device_randomness(command_line, strlen(command_line));
>> +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
>> +		size_t l = strlen(command_line);
>> +		char *rng_seed = strnstr(command_line, rng_seed_str, l);
>> +
>> +		if (rng_seed) {
>> +			char *end;
>> +
>> +			rng_seed += strlen(rng_seed_str);
>> +			l -= rng_seed - command_line;
>> +			end = strnchr(rng_seed, l, ' ');
>> +			if (end)
>> +				l = end - rng_seed;
>> +			credit_trusted_entropy(l);
>> +		}
>> +	}
> This doesn't look right at all.  It calls credit_trusted_entropy(),
> but it doesn't actually feed the contents of rng_seed where.  Why not
> just call add_hwgeneterator_randomness() and drop adding this
> credit_trusted_entropy(l)?

It is already added (will add comment so that this is clear) just above 
with add_device_randomness(command_line,). So all we need to do is 
_credit_ the entropy increase.

A call to add_hwgenerator_randomness() can block when the minimum 
threshold has been fulfilled resulting in a kernel panic, and would mix 
the bytes a second time when fed.

-- Mark

Re: [PATCH] random: add rng-seed= command line option

[Mark Salyzyn]

On 2/7/20 9:28 AM, Kees Cook wrote:
> On Fri, Feb 07, 2020 at 07:07:59AM -0800, Mark Salyzyn wrote:
>> +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
>> +/* caller called add_device_randomness, but it is from a trusted source */
>> +void __init credit_trusted_entropy(unsigned int size)
>> +{
>> +	credit_entropy_bits(&input_pool, size * 8);
>> +}
>> +#endif
> As Ted already mentioned, I was expecting the string contents to actually
> get added somewhere. Is the idea that it's already been added via the
> add_device_randomness(command_line) call, and you just want to explicitly
> credit those bytes? If so, that deserves a comment, and I think it should
> likely not use 8 bits per character, as that's not how many bits are
> possible for an alphanumeric string character; I would expect 6 bits (~32
> standard letter/number) -- this likely needs fixing in the fdt patch too.

Yup, responded to Ted as such.

Both can have near-raw 8-bit data as long as they stay away from certain 
characters.

For the command line space and nul characters. Since rng-seed is 
stripped out of any views no one needs to get hurt.

For OF some other parse characters need to be skipped. The rng-seed is 
also memset'd out of existence after being read so no one gets hurt.

I see no harm with multiplying by six in both cases as entropy credit 
should be realistic, but generators can be more ambitious ...

> . .  .
>> +}
>> +
>>   static void __init setup_command_line(char *command_line)
>>   {
>>   	size_t len, xlen = 0, ilen = 0;
>> +	static const char argsep_str[] __initconst = " -- ";
>> +	static const char alloc_fail_msg[] __initconst =
>> +		"%s: Failed to allocate %zu bytes\n";
> There's some refactoring in this patch unrelated to the seed logic. Can
> you split that out? (I think these changes are good.)

Ok, two patches that come to mind:

- move string constants solely referenced in __init function to __initconst

- boot_command_line is not guaranteed nul terminated, strlen must be 
replaced with strnlen.

>>   
>>   	if (extra_command_line)
>>   		xlen = strlen(extra_command_line);
> Unrelated note: whoa this is based on linux-next which has a massive
> change to the boot command line handling and appears to be doing some
> bad things. I need to go find that thread...

I took top of linus tree, I did not use linux-next (!) Hopefully all is 
good.

> . . .
>> @@ -875,6 +909,21 @@ asmlinkage __visible void __init start_kernel(void)
>>   	rand_initialize();
>>   	add_latent_entropy();
>>   	add_device_randomness(command_line, strlen(command_line));
>> +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
>> +		size_t l = strlen(command_line);
>> +		char *rng_seed = strnstr(command_line, rng_seed_str, l);
>> +
>> +		if (rng_seed) {
>> +			char *end;
>> +
>> +			rng_seed += strlen(rng_seed_str);
>> +			l -= rng_seed - command_line;
>> +			end = strnchr(rng_seed, l, ' ');
>> +			if (end)
>> +				l = end - rng_seed;
>> +			credit_trusted_entropy(l);
>> +		}
>> +	}
> Can you pull this out of line and write a new static help that does all
> of the rng stuff here? Basically from rand_initialize() through
> boot_init_stack_canary(), so it's all in one place and not "open coded"
> in start_kernel(). (And then, actually, you don't need a separate
> credit_trusted_entropy() function at all -- just call
> credit_entropy_bits() directly there (and add a comment about the
> command line already getting added).

sgtm, will do.

Thanks both -- Mark

Re: [PATCH] random: add rng-seed= command line option

[Theodore Y. Ts'o]

On Fri, Feb 07, 2020 at 09:49:17AM -0800, Mark Salyzyn wrote:
> > > It is preferred to add rng-seed to the Device Tree, but some
> > > platforms do not have this option, so this adds the ability to
> > > provide some command-line-limited data to the entropy through this
> > > alternate mechanism.  Expect all 8 bits to be used, but must exclude
> > > space to be accounted in the command line.
> > "all 8 bits"?
> 
> Command line (and Device Tree for that matter) can provide 8-bits of data,
> and specifically for the command line as long as they skip space and nul
> characters, we will be stripping the content out of the command line because
> we strip it from view, so that no one gets hot and bothered.

What wasn't obvious to me initially (and should be clearly documented
in the commit description as well as elsewhere) is that we are already
adding the entire boot command-line string using
"add_device_randomness()" and so what this commit is doing is simply
counting the length of xxx in "rng_seed=xxx" and assuming that those
bytes are 100% entropy and simply crediting the trusted entropy by
length of xxx.  If xxx happened to be a hex string, or worse, was
hard-coded in /etc/grub.conf as "rng_seed=supercalifragilisticexpialidocious"
with this commit (and CONFIG_RANDOM_TRUST_BOOTLOADER), it would assume
that it is safe to credit the boot command line has having sufficient
entropy to fully initialize the CRNG.

> I expected this to be contentious, this is why I call it out. Does _anyone_
> have a disagreement with allowing raw data (minus nul and space characters)
> to be part of the rng-seed?

There are two parts of this that might be controverisial.  The first
is that there isn't actually *fully* eight bits; it's really
log_2(254) bits per caharacter, since NUL and SPC aren't valid.

The second is that we're treating rng_seed as being magic, and if
someone tries to pass in something like rng_seed=0x7932dca76b51
because they didn't understand how rng_seed was going to work, it
would be surprising.

My preference would be to pass in the random seed *not* on the
command-line at all, but as a separate parameter which is passed to
the bootloader, just as we pass in the device-tree, the initrd and the
command-line as separate things.  The problem is that how we pass in
extra boot parameters is architecture specific, and how we might do it
for x86 is different than for arm64.  So yeah, it's a bit more
inconvenient to do things that way; but I think it's also much
cleaner.

					- Ted

Re: [PATCH] random: add rng-seed= command line option

[Steven Rostedt]

On Fri, 7 Feb 2020 19:49:22 -0500
"Theodore Y. Ts'o" <tytso@mit.edu> wrote:


> My preference would be to pass in the random seed *not* on the
> command-line at all, but as a separate parameter which is passed to
> the bootloader, just as we pass in the device-tree, the initrd and the
> command-line as separate things.  The problem is that how we pass in
> extra boot parameters is architecture specific, and how we might do it
> for x86 is different than for arm64.  So yeah, it's a bit more
> inconvenient to do things that way; but I think it's also much
> cleaner.

Hmm, if the boot loader could add on to the bootconfig that Masami just
added, then it could add some "random" seed for each boot! The
bootconfig is just an appended file at the end of the initrd.

-- Steve

Re: [PATCH] random: add rng-seed= command line option

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]

On Fri, Feb 07, 2020 at 07:49:22PM -0500, Theodore Y. Ts'o wrote:

> "add_device_randomness()" and so what this commit is doing is simply
> counting the length of xxx in "rng_seed=xxx" and assuming that those
> bytes are 100% entropy and simply crediting the trusted entropy by
> length of xxx.  If xxx happened to be a hex string, or worse, was

That'd been what I'd intially read the commit message as saying :/

> The second is that we're treating rng_seed as being magic, and if
> someone tries to pass in something like rng_seed=0x7932dca76b51
> because they didn't understand how rng_seed was going to work, it
> would be surprising.

We already have a kaslr-seed property on arm64 since we need a seed for
KASLR *super* early, we could generalize that I guess but it's not clear
to me that it's a good idea.  One fun thing here is that the kernel
command line is visible to userspace so we go and erase the seed from
the command line after reading it.

> My preference would be to pass in the random seed *not* on the
> command-line at all, but as a separate parameter which is passed to
> the bootloader, just as we pass in the device-tree, the initrd and the
> command-line as separate things.  The problem is that how we pass in
> extra boot parameters is architecture specific, and how we might do it
> for x86 is different than for arm64.  So yeah, it's a bit more
> inconvenient to do things that way; but I think it's also much
> cleaner.

Anything that requires boot protocol updates is going to be rather
difficult to deploy for the use it'll likely get - as far as I can see
we're basically just talking about the cases where there's some entropy
source available to the bootloader that the kernel can't get at
directly.  With the arm64 kaslr-seed it's not clear that people are
feeding actual entropy in there, they could be using something like the
device serial number to give different layouts on different devices even
if they can't get any useful entropy for boot to boot variation.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 0/4 v2] random add rng-seed to command line option

[Mark Salyzyn]

A followup to commit 428826f5358c922dc378830a1717b682c0823160
("fdt: add support for rng-seed") to extend what was started
with Open Firmware (OF or Device Tree) parsing, but also add
it to the command line.

If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
command line option length as added trusted entropy.

Always erase all views of the rng-seed option, except early command
line parsing, to prevent leakage to applications or modules, to
eliminate any attack vector.

It is preferred to add rng-seed to the Device Tree, but some
platforms do not have this option, so this adds the ability to
provide some command-line-limited data to the entropy through this
alternate mechanism.  Expect on average 6 bits of useful entropy
per character.

Mark Salyzyn (4):
  init: move string constants to __initconst section
  init: boot_command_line can be truncated
  random: rng-seed source is utf-8
  random: add rng-seed= command line option

---
v2
- Split into four bite sized patches.
- Correct spelling in commit message.
- rng-seed is assumed to be utf-8, so correct both to 6 bits/character
  of collected entropy.
- Move entropy collection to a static __always_inline helper function.

 drivers/char/random.c  |  10 +++-
 include/linux/random.h |   5 ++
 init/main.c            | 115 ++++++++++++++++++++++++++++++-----------
 3 files changed, 100 insertions(+), 30 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog

[PATCH 1/4 v2] init: move string constants to __initconst section

[Mark Salyzyn]

A space-saving measure is to move string constants to __initconst.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Kees Cook <keescook@chromium.org>
---
 init/main.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/init/main.c b/init/main.c
index cc0ee4873419c..a58b72c9433e7 100644
--- a/init/main.c
+++ b/init/main.c
@@ -524,24 +524,27 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { }
  * parsing is performed in place, and we should allow a component to
  * store reference of name/value for future reference.
  */
+static const char alloc_fail_msg[] __initconst =
+	"%s: Failed to allocate %zu bytes\n";
 static void __init setup_command_line(char *command_line)
 {
 	size_t len, xlen = 0, ilen = 0;
+	static const char argsep_str[] __initconst = " -- ";
 
 	if (extra_command_line)
 		xlen = strlen(extra_command_line);
 	if (extra_init_args)
-		ilen = strlen(extra_init_args) + 4; /* for " -- " */
+		ilen = strlen(extra_init_args) + strlen(argsep_str);
 
 	len = xlen + strlen(boot_command_line) + 1;
 
 	saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES);
 	if (!saved_command_line)
-		panic("%s: Failed to allocate %zu bytes\n", __func__, len + ilen);
+		panic(alloc_fail_msg, __func__, len + ilen);
 
 	static_command_line = memblock_alloc(len, SMP_CACHE_BYTES);
 	if (!static_command_line)
-		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
+		panic(alloc_fail_msg, __func__, len);
 
 	if (xlen) {
 		/*
@@ -562,9 +565,9 @@ static void __init setup_command_line(char *command_line)
 		 * to init.
 		 */
 		len = strlen(saved_command_line);
-		if (!strstr(boot_command_line, " -- ")) {
-			strcpy(saved_command_line + len, " -- ");
-			len += 4;
+		if (!strstr(boot_command_line, argsep_str)) {
+			strcpy(saved_command_line + len, argsep_str);
+			len += strlen(argsep_str);
 		} else
 			saved_command_line[len++] = ' ';
 
@@ -1001,12 +1004,11 @@ static int __init initcall_blacklist(char *str)
 			entry = memblock_alloc(sizeof(*entry),
 					       SMP_CACHE_BYTES);
 			if (!entry)
-				panic("%s: Failed to allocate %zu bytes\n",
-				      __func__, sizeof(*entry));
+				panic(alloc_fail_msg, __func__, sizeof(*entry));
 			entry->buf = memblock_alloc(strlen(str_entry) + 1,
 						    SMP_CACHE_BYTES);
 			if (!entry->buf)
-				panic("%s: Failed to allocate %zu bytes\n",
+				panic(alloc_fail_msg,
 				      __func__, strlen(str_entry) + 1);
 			strcpy(entry->buf, str_entry);
 			list_add(&entry->next, &blacklisted_initcalls);
@@ -1204,7 +1206,7 @@ static void __init do_initcalls(void)
 
 	command_line = kzalloc(len, GFP_KERNEL);
 	if (!command_line)
-		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
+		panic(alloc_fail_msg, __func__, len);
 
 	for (level = 0; level < ARRAY_SIZE(initcall_levels) - 1; level++) {
 		/* Parser modifies command_line, restore it each time */
-- 
2.25.0.341.g760bfbb309-goog

[PATCH 2/4 v2] init: boot_command_line can be truncated

[Mark Salyzyn]

boot_command_line may be truncated, use strnlen, strnstr and strlcpy
to handle it.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Kees Cook <keescook@chromium.org>
---
 init/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/init/main.c b/init/main.c
index a58b72c9433e7..9f4ce0356057e 100644
--- a/init/main.c
+++ b/init/main.c
@@ -536,7 +536,7 @@ static void __init setup_command_line(char *command_line)
 	if (extra_init_args)
 		ilen = strlen(extra_init_args) + strlen(argsep_str);
 
-	len = xlen + strlen(boot_command_line) + 1;
+	len = xlen + strnlen(boot_command_line, sizeof(boot_command_line)) + 1;
 
 	saved_command_line = memblock_alloc(len + ilen, SMP_CACHE_BYTES);
 	if (!saved_command_line)
@@ -555,7 +555,7 @@ static void __init setup_command_line(char *command_line)
 		strcpy(saved_command_line, extra_command_line);
 		strcpy(static_command_line, extra_command_line);
 	}
-	strcpy(saved_command_line + xlen, boot_command_line);
+	strlcpy(saved_command_line + xlen, boot_command_line, len - xlen);
 	strcpy(static_command_line + xlen, command_line);
 
 	if (ilen) {
@@ -565,7 +565,8 @@ static void __init setup_command_line(char *command_line)
 		 * to init.
 		 */
 		len = strlen(saved_command_line);
-		if (!strstr(boot_command_line, argsep_str)) {
+		if (!strnstr(boot_command_line, argsep_str,
+			     sizeof(boot_command_line))) {
 			strcpy(saved_command_line + len, argsep_str);
 			len += strlen(argsep_str);
 		} else
@@ -669,7 +670,7 @@ void __init parse_early_param(void)
 		return;
 
 	/* All fall through to do_early_param. */
-	strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+	strlcpy(tmp_cmdline, boot_command_line, sizeof(boot_command_line));
 	parse_early_options(tmp_cmdline);
 	done = 1;
 }
-- 
2.25.0.341.g760bfbb309-goog

[PATCH 3/4 v2] random: rng-seed source is utf-8

[Mark Salyzyn]

commit 428826f5358c922dc378830a1717b682c0823160
("fdt: add support for rng-seed") makes the assumption that the data
in rng-seed is binary, when it is typically constructed of utf-8
characters which has a bitness of roughly 6 to give appropriate
credit due for the entropy.

Signed-off-by: MArk Salyzyn <salyzyn@android.com>
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: Kees Cook <keescook@chromium.org>
Cc: Theodore Y. Ts'o <tytso@mit.edu>
---
 drivers/char/random.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index c7f9584de2c8b..ee21a6a584b15 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2306,7 +2306,7 @@ EXPORT_SYMBOL_GPL(add_hwgenerator_randomness);
 void add_bootloader_randomness(const void *buf, unsigned int size)
 {
 	if (IS_ENABLED(CONFIG_RANDOM_TRUST_BOOTLOADER))
-		add_hwgenerator_randomness(buf, size, size * 8);
+		add_hwgenerator_randomness(buf, size, size * 6);
 	else
 		add_device_randomness(buf, size);
 }
-- 
2.25.0.341.g760bfbb309-goog

[PATCH 4/4 v2] random: add rng-seed= command line option

[Mark Salyzyn]

A followup to commit 428826f5358c922dc378830a1717b682c0823160
("fdt: add support for rng-seed") to extend what was started
with Open Firmware (OF or Device Tree) parsing, but also add
it to the command line.

If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
command line option length as added trusted entropy.

Always erase view of the rng-seed option, except our early command
line parsing, to prevent leakage to applications or modules, to
eliminate any attack vector.

It is preferred to add rng-seed to the Device Tree, but some
platforms do not have this option, so this adds the ability to
provide some command-line-limited data to the entropy through this
alternate mechanism.  Expect on average 6 bits of useful entropy
per character.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alexander Potapenko <glider@google.com>
---
v2
- Split into four bite sized patches.
- Correct spelling in commit message.
- rng-seed is assumed to be utf-8, so correct both to 6 bits/character
  of collected entropy.
- Move entropy collection to a static __always_inline helper function.
---
 drivers/char/random.c  |  8 ++++
 include/linux/random.h |  5 +++
 init/main.c            | 88 ++++++++++++++++++++++++++++++++++--------
 3 files changed, 84 insertions(+), 17 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index ee21a6a584b15..83c77306e18e7 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2311,3 +2311,11 @@ void add_bootloader_randomness(const void *buf, unsigned int size)
 		add_device_randomness(buf, size);
 }
 EXPORT_SYMBOL_GPL(add_bootloader_randomness);
+
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+/* caller called add_device_randomness, but it is from a trusted source */
+void __init credit_trusted_entropy_bits(unsigned int nbits)
+{
+	credit_entropy_bits(&input_pool, nbits);
+}
+#endif
diff --git a/include/linux/random.h b/include/linux/random.h
index d319f9a1e4290..efe8cbe2255ab 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -20,6 +20,11 @@ struct random_ready_callback {
 
 extern void add_device_randomness(const void *, unsigned int);
 extern void add_bootloader_randomness(const void *, unsigned int);
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+extern void __init credit_trusted_entropy_bits(unsigned int nbits);
+#else
+static inline void credit_trusted_entropy_bits(unsigned int nbits) {}
+#endif
 
 #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
 static inline void add_latent_entropy(void)
diff --git a/init/main.c b/init/main.c
index 9f4ce0356057e..ad52f03fb8de4 100644
--- a/init/main.c
+++ b/init/main.c
@@ -524,6 +524,31 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { }
  * parsing is performed in place, and we should allow a component to
  * store reference of name/value for future reference.
  */
+static const char rng_seed_str[] __initconst = "rng-seed=";
+/* try to clear rng-seed so it won't be found by user applications. */
+static void __init copy_command_line(char *dest, char *src, size_t r)
+{
+	char *rng_seed = strnstr(src, rng_seed_str, r);
+
+	if (rng_seed) {
+		size_t l = rng_seed - src;
+		char *end;
+
+		memcpy(dest, src, l);
+		dest += l;
+		src = rng_seed + strlen(rng_seed_str);
+		r -= l + strlen(rng_seed_str);
+		end = strnchr(src, r, ' ');
+		if (end) {
+			if (l && rng_seed[-1] == ' ')
+				++end;
+			r -= end - src;
+			src = end;
+		}
+	}
+	strlcpy(dest, src, r);
+}
+
 static const char alloc_fail_msg[] __initconst =
 	"%s: Failed to allocate %zu bytes\n";
 static void __init setup_command_line(char *command_line)
@@ -552,11 +577,15 @@ static void __init setup_command_line(char *command_line)
 		 * lines because there could be dashes (separator of init
 		 * command line) in the command lines.
 		 */
-		strcpy(saved_command_line, extra_command_line);
-		strcpy(static_command_line, extra_command_line);
+		copy_command_line(saved_command_line, extra_command_line,
+				  xlen + 1);
+		copy_command_line(static_command_line, extra_command_line,
+				  xlen + 1);
 	}
-	strlcpy(saved_command_line + xlen, boot_command_line, len - xlen);
-	strcpy(static_command_line + xlen, command_line);
+	copy_command_line(saved_command_line + xlen, boot_command_line,
+			  len - xlen);
+	copy_command_line(static_command_line + xlen, command_line,
+			  len - xlen);
 
 	if (ilen) {
 		/*
@@ -572,7 +601,8 @@ static void __init setup_command_line(char *command_line)
 		} else
 			saved_command_line[len++] = ' ';
 
-		strcpy(saved_command_line + len, extra_init_args);
+		copy_command_line(saved_command_line + len, extra_init_args,
+				  ilen - strlen(argsep_str) + 1);
 	}
 }
 
@@ -757,6 +787,41 @@ void __init __weak arch_call_rest_init(void)
 	rest_init();
 }
 
+static __always_inline void __init collect_entropy(const char *command_line)
+{
+	/*
+	 * For best initial stack canary entropy, prepare it after:
+	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
+	 * - timekeeping_init() for ktime entropy used in rand_initialize()
+	 * - rand_initialize() to get any arch-specific entropy like RDRAND
+	 * - add_latent_entropy() to get any latent entropy
+	 * - adding command line entropy
+	 */
+	rand_initialize();
+	add_latent_entropy();
+	add_device_randomness(command_line, strlen(command_line));
+	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
+		/*
+		 * Added command line device randomness above,
+		 * now add entropy credit for just rng-seed=<data>
+		 */
+		size_t l = strlen(command_line);
+		char *rng_seed = strnstr(command_line, rng_seed_str, l);
+
+		if (rng_seed) {
+			char *end;
+
+			rng_seed += strlen(rng_seed_str);
+			l -= rng_seed - command_line;
+			end = strnchr(rng_seed, l, ' ');
+			if (end)
+				l = end - rng_seed;
+			credit_trusted_entropy_bits(l * 6);
+		}
+	}
+	boot_init_stack_canary();
+}
+
 asmlinkage __visible void __init start_kernel(void)
 {
 	char *command_line;
@@ -868,18 +933,7 @@ asmlinkage __visible void __init start_kernel(void)
 	softirq_init();
 	timekeeping_init();
 
-	/*
-	 * For best initial stack canary entropy, prepare it after:
-	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
-	 * - timekeeping_init() for ktime entropy used in rand_initialize()
-	 * - rand_initialize() to get any arch-specific entropy like RDRAND
-	 * - add_latent_entropy() to get any latent entropy
-	 * - adding command line entropy
-	 */
-	rand_initialize();
-	add_latent_entropy();
-	add_device_randomness(command_line, strlen(command_line));
-	boot_init_stack_canary();
+	collect_entropy(command_line);
 
 	time_init();
 	printk_safe_init();
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH 4/4 v2] random: add rng-seed= command line option

[Randy Dunlap]

On 2/10/20 6:45 AM, Mark Salyzyn wrote:
> A followup to commit 428826f5358c922dc378830a1717b682c0823160
> ("fdt: add support for rng-seed") to extend what was started
> with Open Firmware (OF or Device Tree) parsing, but also add
> it to the command line.
> 
> If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
> command line option length as added trusted entropy.
> 
> Always erase view of the rng-seed option, except our early command
> line parsing, to prevent leakage to applications or modules, to
> eliminate any attack vector.
> 
> It is preferred to add rng-seed to the Device Tree, but some
> platforms do not have this option, so this adds the ability to
> provide some command-line-limited data to the entropy through this
> alternate mechanism.  Expect on average 6 bits of useful entropy
> per character.
> 

> ---
>  drivers/char/random.c  |  8 ++++
>  include/linux/random.h |  5 +++
>  init/main.c            | 88 ++++++++++++++++++++++++++++++++++--------
>  3 files changed, 84 insertions(+), 17 deletions(-)


> diff --git a/init/main.c b/init/main.c
> index 9f4ce0356057e..ad52f03fb8de4 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -524,6 +524,31 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { }
>   * parsing is performed in place, and we should allow a component to
>   * store reference of name/value for future reference.
>   */
> +static const char rng_seed_str[] __initconst = "rng-seed=";
> +/* try to clear rng-seed so it won't be found by user applications. */
> +static void __init copy_command_line(char *dest, char *src, size_t r)
> +{

Please add this command line option to
Documentation/admin-guide/kernel-parameters.txt.

thanks.
-- 
~Randy

[PATCH 4/4 v3] random: add rng-seed= command line option

[Mark Salyzyn]

A followup to commit 428826f5358c922dc378830a1717b682c0823160
("fdt: add support for rng-seed") to extend what was started
with Open Firmware (OF or Device Tree) parsing, but also add
it to the command line.

If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the rng-seed
command line option length as added trusted entropy.

Always erase view of the rng-seed option, except our early command
line parsing, to prevent leakage to applications or modules, to
eliminate any attack vector.

It is preferred to add rng-seed to the Device Tree, but some
platforms do not have this option, so this adds the ability to
provide some command-line-limited data to the entropy through this
alternate mechanism.  Expect on average 6 bits of useful entropy
per character.

Signed-off-by: Mark Salyzyn <salyzyn@android.com>
Cc: linux-kernel@vger.kernel.org
Cc: kernel-team@android.com
Cc: "Theodore Ts'o" <tytso@mit.edu>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: Vasily Gorbik <gor@linux.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Arvind Sankar <nivedita@alum.mit.edu>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alexander Potapenko <glider@google.com>
---
v3
- Add Documentation (all other new v2 patches unchanged)

v2
- Split into four bite sized patches.
- Correct spelling in commit message.
- rng-seed is assumed to be utf-8, so correct both to 6 bits/character
  of collected entropy.
- Move entropy collection to a static __always_inline helper function.
---
 .../admin-guide/kernel-parameters.txt         | 11 +++
 drivers/char/random.c                         |  8 ++
 include/linux/random.h                        |  5 ++
 init/main.c                                   | 88 +++++++++++++++----
 4 files changed, 95 insertions(+), 17 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index dbc22d6846275..f3c373cc40f9a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4334,6 +4334,17 @@
 			[KNL] Disable ring 3 MONITOR/MWAIT feature on supported
 			CPUs.
 
+	rng-seed=	[KNL] Provide a trusted seed for the kernel's CRNG.
+			Seed only trusted if CONFIG_RANDOM_TRUST_BOOTLOADER.
+			After collection, this option is wiped from the command
+			line views.  The seed is given a weight of 6 bits per
+			character with the assumption that it is a printable
+			utf8 string.  It is expected that the supplier of the
+			seed, typically a bootloader or virtualization, will
+			supply a new random seed for each kernel instance.
+			A fixed serial number is typically not appropriate
+			for security features like ASLR.
+
 	ro		[KNL] Mount root device read-only on boot
 
 	rodata=		[KNL]
diff --git a/drivers/char/random.c b/drivers/char/random.c
index ee21a6a584b15..83c77306e18e7 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2311,3 +2311,11 @@ void add_bootloader_randomness(const void *buf, unsigned int size)
 		add_device_randomness(buf, size);
 }
 EXPORT_SYMBOL_GPL(add_bootloader_randomness);
+
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+/* caller called add_device_randomness, but it is from a trusted source */
+void __init credit_trusted_entropy_bits(unsigned int nbits)
+{
+	credit_entropy_bits(&input_pool, nbits);
+}
+#endif
diff --git a/include/linux/random.h b/include/linux/random.h
index d319f9a1e4290..efe8cbe2255ab 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -20,6 +20,11 @@ struct random_ready_callback {
 
 extern void add_device_randomness(const void *, unsigned int);
 extern void add_bootloader_randomness(const void *, unsigned int);
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+extern void __init credit_trusted_entropy_bits(unsigned int nbits);
+#else
+static inline void credit_trusted_entropy_bits(unsigned int nbits) {}
+#endif
 
 #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
 static inline void add_latent_entropy(void)
diff --git a/init/main.c b/init/main.c
index 9f4ce0356057e..ad52f03fb8de4 100644
--- a/init/main.c
+++ b/init/main.c
@@ -524,6 +524,31 @@ static inline void smp_prepare_cpus(unsigned int maxcpus) { }
  * parsing is performed in place, and we should allow a component to
  * store reference of name/value for future reference.
  */
+static const char rng_seed_str[] __initconst = "rng-seed=";
+/* try to clear rng-seed so it won't be found by user applications. */
+static void __init copy_command_line(char *dest, char *src, size_t r)
+{
+	char *rng_seed = strnstr(src, rng_seed_str, r);
+
+	if (rng_seed) {
+		size_t l = rng_seed - src;
+		char *end;
+
+		memcpy(dest, src, l);
+		dest += l;
+		src = rng_seed + strlen(rng_seed_str);
+		r -= l + strlen(rng_seed_str);
+		end = strnchr(src, r, ' ');
+		if (end) {
+			if (l && rng_seed[-1] == ' ')
+				++end;
+			r -= end - src;
+			src = end;
+		}
+	}
+	strlcpy(dest, src, r);
+}
+
 static const char alloc_fail_msg[] __initconst =
 	"%s: Failed to allocate %zu bytes\n";
 static void __init setup_command_line(char *command_line)
@@ -552,11 +577,15 @@ static void __init setup_command_line(char *command_line)
 		 * lines because there could be dashes (separator of init
 		 * command line) in the command lines.
 		 */
-		strcpy(saved_command_line, extra_command_line);
-		strcpy(static_command_line, extra_command_line);
+		copy_command_line(saved_command_line, extra_command_line,
+				  xlen + 1);
+		copy_command_line(static_command_line, extra_command_line,
+				  xlen + 1);
 	}
-	strlcpy(saved_command_line + xlen, boot_command_line, len - xlen);
-	strcpy(static_command_line + xlen, command_line);
+	copy_command_line(saved_command_line + xlen, boot_command_line,
+			  len - xlen);
+	copy_command_line(static_command_line + xlen, command_line,
+			  len - xlen);
 
 	if (ilen) {
 		/*
@@ -572,7 +601,8 @@ static void __init setup_command_line(char *command_line)
 		} else
 			saved_command_line[len++] = ' ';
 
-		strcpy(saved_command_line + len, extra_init_args);
+		copy_command_line(saved_command_line + len, extra_init_args,
+				  ilen - strlen(argsep_str) + 1);
 	}
 }
 
@@ -757,6 +787,41 @@ void __init __weak arch_call_rest_init(void)
 	rest_init();
 }
 
+static __always_inline void __init collect_entropy(const char *command_line)
+{
+	/*
+	 * For best initial stack canary entropy, prepare it after:
+	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
+	 * - timekeeping_init() for ktime entropy used in rand_initialize()
+	 * - rand_initialize() to get any arch-specific entropy like RDRAND
+	 * - add_latent_entropy() to get any latent entropy
+	 * - adding command line entropy
+	 */
+	rand_initialize();
+	add_latent_entropy();
+	add_device_randomness(command_line, strlen(command_line));
+	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
+		/*
+		 * Added command line device randomness above,
+		 * now add entropy credit for just rng-seed=<data>
+		 */
+		size_t l = strlen(command_line);
+		char *rng_seed = strnstr(command_line, rng_seed_str, l);
+
+		if (rng_seed) {
+			char *end;
+
+			rng_seed += strlen(rng_seed_str);
+			l -= rng_seed - command_line;
+			end = strnchr(rng_seed, l, ' ');
+			if (end)
+				l = end - rng_seed;
+			credit_trusted_entropy_bits(l * 6);
+		}
+	}
+	boot_init_stack_canary();
+}
+
 asmlinkage __visible void __init start_kernel(void)
 {
 	char *command_line;
@@ -868,18 +933,7 @@ asmlinkage __visible void __init start_kernel(void)
 	softirq_init();
 	timekeeping_init();
 
-	/*
-	 * For best initial stack canary entropy, prepare it after:
-	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
-	 * - timekeeping_init() for ktime entropy used in rand_initialize()
-	 * - rand_initialize() to get any arch-specific entropy like RDRAND
-	 * - add_latent_entropy() to get any latent entropy
-	 * - adding command line entropy
-	 */
-	rand_initialize();
-	add_latent_entropy();
-	add_device_randomness(command_line, strlen(command_line));
-	boot_init_stack_canary();
+	collect_entropy(command_line);
 
 	time_init();
 	printk_safe_init();
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH] random: add rng-seed= command line option

[Theodore Y. Ts'o]

On Mon, Feb 10, 2020 at 12:13:25PM +0000, Mark Brown wrote:
> > The second is that we're treating rng_seed as being magic, and if
> > someone tries to pass in something like rng_seed=0x7932dca76b51
> > because they didn't understand how rng_seed was going to work, it
> > would be surprising.
> 
> We already have a kaslr-seed property on arm64 since we need a seed for
> KASLR *super* early, we could generalize that I guess but it's not clear
> to me that it's a good idea.  One fun thing here is that the kernel
> command line is visible to userspace so we go and erase the seed from
> the command line after reading it.

This is exactly what this patch is doing, in fact (it is erasing the
seed from the command line).

> > My preference would be to pass in the random seed *not* on the
> > command-line at all, but as a separate parameter which is passed to
> > the bootloader, just as we pass in the device-tree, the initrd and the
> > command-line as separate things.  The problem is that how we pass in
> > extra boot parameters is architecture specific, and how we might do it
> > for x86 is different than for arm64.  So yeah, it's a bit more
> > inconvenient to do things that way; but I think it's also much
> > cleaner.
> 
> Anything that requires boot protocol updates is going to be rather
> difficult to deploy for the use it'll likely get - as far as I can see
> we're basically just talking about the cases where there's some entropy
> source available to the bootloader that the kernel can't get at
> directly.  With the arm64 kaslr-seed it's not clear that people are
> feeding actual entropy in there, they could be using something like the
> device serial number to give different layouts on different devices even
> if they can't get any useful entropy for boot to boot variation.

So here's one thing we could do; we could require something like:

rng_seed=<nr_bits>,<base-64 encoded string of 32 bytes>

... where the kernel parses rng_seed, and errors out if nr_bits is
greater than 256, and errors out if the base-64 encoded string is not
valid, and then replaces it with the SHA-256 hash of the rng seed,
base-64 encoded.

That way if there is a crappy handset which is just encoding the
device serial number, it becomes painfully obvious that someone is
cheating.

Is that overkill?  Well, from my perspective, we're talking about an
industry that was willing to turn the CPU thermal safety limits when
certain benchmark applications were detected to be running, just to
gain a commercial advantage.  So trust doesn't come easily to me, when
it comes to corporate desires of expediency.  :-)

	       	     	     	  	    - Ted

Re: [PATCH] random: add rng-seed= command line option

[Masami Hiramatsu]

Hi,

On Fri, 7 Feb 2020 19:53:26 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 7 Feb 2020 19:49:22 -0500
> "Theodore Y. Ts'o" <tytso@mit.edu> wrote:
> 
> 
> > My preference would be to pass in the random seed *not* on the
> > command-line at all, but as a separate parameter which is passed to
> > the bootloader, just as we pass in the device-tree, the initrd and the
> > command-line as separate things.  The problem is that how we pass in
> > extra boot parameters is architecture specific, and how we might do it
> > for x86 is different than for arm64.  So yeah, it's a bit more
> > inconvenient to do things that way; but I think it's also much
> > cleaner.
> 
> Hmm, if the boot loader could add on to the bootconfig that Masami just
> added, then it could add some "random" seed for each boot! The
> bootconfig is just an appended file at the end of the initrd.

Yeah, it is easy to add bootconfig support to a bootloader. It can add
a entropy number as "rng.seed=XXX" text after initrd image with size
and checksum. That is architecutre independent way to pass such hidden
parameter.
(hidden key must be filtered out when printing out the /proc/bootconfig,
but that is very easy too, just need a strncmp)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] random: add rng-seed= command line option

[Masami Hiramatsu]

On Thu, 13 Feb 2020 20:24:54 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > My preference would be to pass in the random seed *not* on the
> > > command-line at all, but as a separate parameter which is passed to
> > > the bootloader, just as we pass in the device-tree, the initrd and the
> > > command-line as separate things.  The problem is that how we pass in
> > > extra boot parameters is architecture specific, and how we might do it
> > > for x86 is different than for arm64.  So yeah, it's a bit more
> > > inconvenient to do things that way; but I think it's also much
> > > cleaner.
> > 
> > Hmm, if the boot loader could add on to the bootconfig that Masami just
> > added, then it could add some "random" seed for each boot! The
> > bootconfig is just an appended file at the end of the initrd.
> 
> Yeah, it is easy to add bootconfig support to a bootloader. It can add
> a entropy number as "rng.seed=XXX" text after initrd image with size
> and checksum. That is architecutre independent way to pass such hidden
> parameter.
> (hidden key must be filtered out when printing out the /proc/bootconfig,
> but that is very easy too, just need a strncmp)
> 

And here is the patch to support "random.rng_seed = XXX" option by
bootconfig. Now you can focus on what you want to do. No need to
modify command line strings.

BTW, if you think you need to pass UTF-8 code as a data, I'm happy to
update the bootconfig to support it. Just for the safeness, I have
limited it by isprint() || isspace().

Thank you,

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 26956c006987..43fbbd307204 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -554,6 +554,7 @@ config RANDOM_TRUST_CPU
 
 config RANDOM_TRUST_BOOTLOADER
 	bool "Trust the bootloader to initialize Linux's CRNG"
+	select BOOT_CONFIG
 	help
 	Some bootloaders can provide entropy to increase the kernel's initial
 	device randomness. Say Y here to assume the entropy provided by the
diff --git a/drivers/char/random.c b/drivers/char/random.c
index c7f9584de2c8..0ae33bbbd338 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -2311,3 +2311,11 @@ void add_bootloader_randomness(const void *buf, unsigned int size)
 		add_device_randomness(buf, size);
 }
 EXPORT_SYMBOL_GPL(add_bootloader_randomness);
+
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+/* caller called add_device_randomness, but it is from a trusted source */
+void __init credit_trusted_entropy_bits(unsigned int nbits)
+{
+	credit_entropy_bits(&input_pool, nbits);
+}
+#endif
diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
index 9955d75c0585..aace466c56ed 100644
--- a/fs/proc/bootconfig.c
+++ b/fs/proc/bootconfig.c
@@ -36,6 +36,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
 		ret = xbc_node_compose_key(leaf, key, XBC_KEYLEN_MAX);
 		if (ret < 0)
 			break;
+		/* For keeping security reason, remove randomness key */
+		if (!strcmp(key, RANDOM_SEED_XBC_KEY))
+			continue;
 		ret = snprintf(dst, rest(dst, end), "%s = ", key);
 		if (ret < 0)
 			break;
diff --git a/include/linux/random.h b/include/linux/random.h
index d319f9a1e429..c8f41ab4f342 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -20,6 +20,13 @@ struct random_ready_callback {
 
 extern void add_device_randomness(const void *, unsigned int);
 extern void add_bootloader_randomness(const void *, unsigned int);
+#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
+extern void __init credit_trusted_entropy_bits(unsigned int nbits);
+#else
+static inline void credit_trusted_entropy_bits(unsigned int nbits) {}
+#endif
+
+#define RANDOM_SEED_XBC_KEY "random.rng_seed"
 
 #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
 static inline void add_latent_entropy(void)
diff --git a/init/main.c b/init/main.c
index f95b014a5479..6c3f51bc76d5 100644
--- a/init/main.c
+++ b/init/main.c
@@ -776,6 +776,32 @@ void __init __weak arch_call_rest_init(void)
 	rest_init();
 }
 
+static __always_inline void __init collect_entropy(const char *command_line)
+{
+	/*
+	 * For best initial stack canary entropy, prepare it after:
+	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
+	 * - timekeeping_init() for ktime entropy used in rand_initialize()
+	 * - rand_initialize() to get any arch-specific entropy like RDRAND
+	 * - add_latent_entropy() to get any latent entropy
+	 * - adding command line entropy
+	 */
+	rand_initialize();
+	add_latent_entropy();
+	add_device_randomness(command_line, strlen(command_line));
+	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
+		/*
+		 * Added bootconfig device randomness above,
+		 * now add entropy credit for just random.rng_seed=<data>
+		 */
+		const char *rng_seed = xbc_find_value(RANDOM_SEED_XBC_KEY, NULL);
+
+		if (rng_seed)
+			credit_trusted_entropy_bits(strlen(rng_seed) * 6);
+	}
+	boot_init_stack_canary();
+}
+
 asmlinkage __visible void __init start_kernel(void)
 {
 	char *command_line;
@@ -887,18 +913,7 @@ asmlinkage __visible void __init start_kernel(void)
 	softirq_init();
 	timekeeping_init();
 
-	/*
-	 * For best initial stack canary entropy, prepare it after:
-	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
-	 * - timekeeping_init() for ktime entropy used in rand_initialize()
-	 * - rand_initialize() to get any arch-specific entropy like RDRAND
-	 * - add_latent_entropy() to get any latent entropy
-	 * - adding command line entropy
-	 */
-	rand_initialize();
-	add_latent_entropy();
-	add_device_randomness(command_line, strlen(command_line));
-	boot_init_stack_canary();
+	collect_entropy(command_line);
 
 	time_init();
 	printk_safe_init();

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] random: add rng-seed= command line option

[Mark Salyzyn]

On 2/13/20 7:03 AM, Masami Hiramatsu wrote:
> On Thu, 13 Feb 2020 20:24:54 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
>>>> My preference would be to pass in the random seed *not* on the
>>>> command-line at all, but as a separate parameter which is passed to
>>>> the bootloader, just as we pass in the device-tree, the initrd and the
>>>> command-line as separate things.  The problem is that how we pass in
>>>> extra boot parameters is architecture specific, and how we might do it
>>>> for x86 is different than for arm64.  So yeah, it's a bit more
>>>> inconvenient to do things that way; but I think it's also much
>>>> cleaner.
>>> Hmm, if the boot loader could add on to the bootconfig that Masami just
>>> added, then it could add some "random" seed for each boot! The
>>> bootconfig is just an appended file at the end of the initrd.
>> Yeah, it is easy to add bootconfig support to a bootloader. It can add
>> a entropy number as "rng.seed=XXX" text after initrd image with size
>> and checksum. That is architecutre independent way to pass such hidden
>> parameter.
>> (hidden key must be filtered out when printing out the /proc/bootconfig,
>> but that is very easy too, just need a strncmp)
>>
> And here is the patch to support "random.rng_seed = XXX" option by
> bootconfig. Now you can focus on what you want to do. No need to
> modify command line strings.

LGTM, our virtualized emulator (cuttlefish) folks believe they can do it 
this way. Albeit keep in mind that there are _thousands_ of embedded 
bootloaders out there that are comfortable updating DT and kernel 
command line, but few that add boot configs, so this may add complexity. 
For our use case that caused us to need this, the cuttlefish Android 
emulated device, not a problem though.

However, the entropy _data_ has not been added (see below)

Could you please formally re-submit your patch mhiramet@ with a change 
to push the _data_ as well to the entropy?

-- Mark

>
> BTW, if you think you need to pass UTF-8 code as a data, I'm happy to
> update the bootconfig to support it. Just for the safeness, I have
> limited it by isprint() || isspace().
>
> Thank you,
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 26956c006987..43fbbd307204 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -554,6 +554,7 @@ config RANDOM_TRUST_CPU
>   
>   config RANDOM_TRUST_BOOTLOADER
>   	bool "Trust the bootloader to initialize Linux's CRNG"
> +	select BOOT_CONFIG
>   	help
>   	Some bootloaders can provide entropy to increase the kernel's initial
>   	device randomness. Say Y here to assume the entropy provided by the
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index c7f9584de2c8..0ae33bbbd338 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -2311,3 +2311,11 @@ void add_bootloader_randomness(const void *buf, unsigned int size)
>   		add_device_randomness(buf, size);
>   }
>   EXPORT_SYMBOL_GPL(add_bootloader_randomness);
> +
> +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
> +/* caller called add_device_randomness, but it is from a trusted source */
> +void __init credit_trusted_entropy_bits(unsigned int nbits)
> +{
> +	credit_entropy_bits(&input_pool, nbits);
> +}
> +#endif
> diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
> index 9955d75c0585..aace466c56ed 100644
> --- a/fs/proc/bootconfig.c
> +++ b/fs/proc/bootconfig.c
> @@ -36,6 +36,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
>   		ret = xbc_node_compose_key(leaf, key, XBC_KEYLEN_MAX);
>   		if (ret < 0)
>   			break;
> +		/* For keeping security reason, remove randomness key */
> +		if (!strcmp(key, RANDOM_SEED_XBC_KEY))
> +			continue;
>   		ret = snprintf(dst, rest(dst, end), "%s = ", key);
>   		if (ret < 0)
>   			break;
> diff --git a/include/linux/random.h b/include/linux/random.h
> index d319f9a1e429..c8f41ab4f342 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -20,6 +20,13 @@ struct random_ready_callback {
>   
>   extern void add_device_randomness(const void *, unsigned int);
>   extern void add_bootloader_randomness(const void *, unsigned int);
> +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
> +extern void __init credit_trusted_entropy_bits(unsigned int nbits);
> +#else
> +static inline void credit_trusted_entropy_bits(unsigned int nbits) {}
> +#endif
> +
> +#define RANDOM_SEED_XBC_KEY "random.rng_seed"
>   
>   #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
>   static inline void add_latent_entropy(void)
> diff --git a/init/main.c b/init/main.c
> index f95b014a5479..6c3f51bc76d5 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -776,6 +776,32 @@ void __init __weak arch_call_rest_init(void)
>   	rest_init();
>   }
>   
> +static __always_inline void __init collect_entropy(const char *command_line)
> +{
> +	/*
> +	 * For best initial stack canary entropy, prepare it after:
> +	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
> +	 * - timekeeping_init() for ktime entropy used in rand_initialize()
> +	 * - rand_initialize() to get any arch-specific entropy like RDRAND
> +	 * - add_latent_entropy() to get any latent entropy
> +	 * - adding command line entropy
> +	 */
> +	rand_initialize();
> +	add_latent_entropy();
> +	add_device_randomness(command_line, strlen(command_line));
> +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
> +		/*
> +		 * Added bootconfig device randomness above,

This part is incorrect, the rng_seed collected below was _not_ added to 
the device_randomness.

add_device_randomness(rng_seed, strlen(rng_seed)) needs to be pushed 
below along with the credit.

> +		 * now add entropy credit for just random.rng_seed=<data>
> +		 */
> +		const char *rng_seed = xbc_find_value(RANDOM_SEED_XBC_KEY, NULL);
> +
> +		if (rng_seed)
> +			credit_trusted_entropy_bits(strlen(rng_seed) * 6);
> +	}
> +	boot_init_stack_canary();
> +}
> +
>   asmlinkage __visible void __init start_kernel(void)
>   {
>   	char *command_line;
> @@ -887,18 +913,7 @@ asmlinkage __visible void __init start_kernel(void)
>   	softirq_init();
>   	timekeeping_init();
>   
> -	/*
> -	 * For best initial stack canary entropy, prepare it after:
> -	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
> -	 * - timekeeping_init() for ktime entropy used in rand_initialize()
> -	 * - rand_initialize() to get any arch-specific entropy like RDRAND
> -	 * - add_latent_entropy() to get any latent entropy
> -	 * - adding command line entropy
> -	 */
> -	rand_initialize();
> -	add_latent_entropy();
> -	add_device_randomness(command_line, strlen(command_line));
> -	boot_init_stack_canary();
> +	collect_entropy(command_line);
>   
>   	time_init();
>   	printk_safe_init();
>

Re: [PATCH] random: add rng-seed= command line option

[Masami Hiramatsu]

Hi Mark,

On Thu, 13 Feb 2020 10:44:59 -0800
Mark Salyzyn <salyzyn@android.com> wrote:

> On 2/13/20 7:03 AM, Masami Hiramatsu wrote:
> > On Thu, 13 Feb 2020 20:24:54 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> >>>> My preference would be to pass in the random seed *not* on the
> >>>> command-line at all, but as a separate parameter which is passed to
> >>>> the bootloader, just as we pass in the device-tree, the initrd and the
> >>>> command-line as separate things.  The problem is that how we pass in
> >>>> extra boot parameters is architecture specific, and how we might do it
> >>>> for x86 is different than for arm64.  So yeah, it's a bit more
> >>>> inconvenient to do things that way; but I think it's also much
> >>>> cleaner.
> >>> Hmm, if the boot loader could add on to the bootconfig that Masami just
> >>> added, then it could add some "random" seed for each boot! The
> >>> bootconfig is just an appended file at the end of the initrd.
> >> Yeah, it is easy to add bootconfig support to a bootloader. It can add
> >> a entropy number as "rng.seed=XXX" text after initrd image with size
> >> and checksum. That is architecutre independent way to pass such hidden
> >> parameter.
> >> (hidden key must be filtered out when printing out the /proc/bootconfig,
> >> but that is very easy too, just need a strncmp)
> >>
> > And here is the patch to support "random.rng_seed = XXX" option by
> > bootconfig. Now you can focus on what you want to do. No need to
> > modify command line strings.
> 
> LGTM, our virtualized emulator (cuttlefish) folks believe they can do it 
> this way. Albeit keep in mind that there are _thousands_ of embedded 
> bootloaders out there that are comfortable updating DT and kernel 
> command line, but few that add boot configs, so this may add complexity. 

I see, since the bootconfig is a new feature, it will take a time to
be supported widely. Even though, maybe they can use DT for that
purpose.

> For our use case that caused us to need this, the cuttlefish Android 
> emulated device, not a problem though.
> 
> However, the entropy _data_ has not been added (see below)

Oh, I missed that.

> 
> Could you please formally re-submit your patch mhiramet@ with a change 
> to push the _data_ as well to the entropy?

Yes, I'll do.

> 
> -- Mark
> 
> >
> > BTW, if you think you need to pass UTF-8 code as a data, I'm happy to
> > update the bootconfig to support it. Just for the safeness, I have
> > limited it by isprint() || isspace().
> >
> > Thank you,
> >
> > diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> > index 26956c006987..43fbbd307204 100644
> > --- a/drivers/char/Kconfig
> > +++ b/drivers/char/Kconfig
> > @@ -554,6 +554,7 @@ config RANDOM_TRUST_CPU
> >   
> >   config RANDOM_TRUST_BOOTLOADER
> >   	bool "Trust the bootloader to initialize Linux's CRNG"
> > +	select BOOT_CONFIG
> >   	help
> >   	Some bootloaders can provide entropy to increase the kernel's initial
> >   	device randomness. Say Y here to assume the entropy provided by the
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > index c7f9584de2c8..0ae33bbbd338 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -2311,3 +2311,11 @@ void add_bootloader_randomness(const void *buf, unsigned int size)
> >   		add_device_randomness(buf, size);
> >   }
> >   EXPORT_SYMBOL_GPL(add_bootloader_randomness);
> > +
> > +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
> > +/* caller called add_device_randomness, but it is from a trusted source */
> > +void __init credit_trusted_entropy_bits(unsigned int nbits)
> > +{
> > +	credit_entropy_bits(&input_pool, nbits);
> > +}
> > +#endif
> > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
> > index 9955d75c0585..aace466c56ed 100644
> > --- a/fs/proc/bootconfig.c
> > +++ b/fs/proc/bootconfig.c
> > @@ -36,6 +36,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> >   		ret = xbc_node_compose_key(leaf, key, XBC_KEYLEN_MAX);
> >   		if (ret < 0)
> >   			break;
> > +		/* For keeping security reason, remove randomness key */
> > +		if (!strcmp(key, RANDOM_SEED_XBC_KEY))
> > +			continue;
> >   		ret = snprintf(dst, rest(dst, end), "%s = ", key);
> >   		if (ret < 0)
> >   			break;
> > diff --git a/include/linux/random.h b/include/linux/random.h
> > index d319f9a1e429..c8f41ab4f342 100644
> > --- a/include/linux/random.h
> > +++ b/include/linux/random.h
> > @@ -20,6 +20,13 @@ struct random_ready_callback {
> >   
> >   extern void add_device_randomness(const void *, unsigned int);
> >   extern void add_bootloader_randomness(const void *, unsigned int);
> > +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
> > +extern void __init credit_trusted_entropy_bits(unsigned int nbits);
> > +#else
> > +static inline void credit_trusted_entropy_bits(unsigned int nbits) {}
> > +#endif
> > +
> > +#define RANDOM_SEED_XBC_KEY "random.rng_seed"
> >   
> >   #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
> >   static inline void add_latent_entropy(void)
> > diff --git a/init/main.c b/init/main.c
> > index f95b014a5479..6c3f51bc76d5 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -776,6 +776,32 @@ void __init __weak arch_call_rest_init(void)
> >   	rest_init();
> >   }
> >   
> > +static __always_inline void __init collect_entropy(const char *command_line)
> > +{
> > +	/*
> > +	 * For best initial stack canary entropy, prepare it after:
> > +	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
> > +	 * - timekeeping_init() for ktime entropy used in rand_initialize()
> > +	 * - rand_initialize() to get any arch-specific entropy like RDRAND
> > +	 * - add_latent_entropy() to get any latent entropy
> > +	 * - adding command line entropy
> > +	 */
> > +	rand_initialize();
> > +	add_latent_entropy();
> > +	add_device_randomness(command_line, strlen(command_line));
> > +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
> > +		/*
> > +		 * Added bootconfig device randomness above,
> 
> This part is incorrect, the rng_seed collected below was _not_ added to 
> the device_randomness.
> 
> add_device_randomness(rng_seed, strlen(rng_seed)) needs to be pushed 
> below along with the credit.

OK, as same as above command_line, I'll add that.

Thank you,

> 
> > +		 * now add entropy credit for just random.rng_seed=<data>
> > +		 */
> > +		const char *rng_seed = xbc_find_value(RANDOM_SEED_XBC_KEY, NULL);
> > +
> > +		if (rng_seed)
> > +			credit_trusted_entropy_bits(strlen(rng_seed) * 6);
> > +	}
> > +	boot_init_stack_canary();
> > +}
> > +
> >   asmlinkage __visible void __init start_kernel(void)
> >   {
> >   	char *command_line;
> > @@ -887,18 +913,7 @@ asmlinkage __visible void __init start_kernel(void)
> >   	softirq_init();
> >   	timekeeping_init();
> >   
> > -	/*
> > -	 * For best initial stack canary entropy, prepare it after:
> > -	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
> > -	 * - timekeeping_init() for ktime entropy used in rand_initialize()
> > -	 * - rand_initialize() to get any arch-specific entropy like RDRAND
> > -	 * - add_latent_entropy() to get any latent entropy
> > -	 * - adding command line entropy
> > -	 */
> > -	rand_initialize();
> > -	add_latent_entropy();
> > -	add_device_randomness(command_line, strlen(command_line));
> > -	boot_init_stack_canary();
> > +	collect_entropy(command_line);
> >   
> >   	time_init();
> >   	printk_safe_init();
> >
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] random: add rng-seed= command line option

[Mark Salyzyn]

On 2/13/20 5:16 PM, Masami Hiramatsu wrote:
> Hi Mark,
>
> On Thu, 13 Feb 2020 10:44:59 -0800
> Mark Salyzyn <salyzyn@android.com> wrote:
>
>> On 2/13/20 7:03 AM, Masami Hiramatsu wrote:
>>> On Thu, 13 Feb 2020 20:24:54 +0900
>>> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>>
>>>>>> My preference would be to pass in the random seed *not* on the
>>>>>> command-line at all, but as a separate parameter which is passed to
>>>>>> the bootloader, just as we pass in the device-tree, the initrd and the
>>>>>> command-line as separate things.  The problem is that how we pass in
>>>>>> extra boot parameters is architecture specific, and how we might do it
>>>>>> for x86 is different than for arm64.  So yeah, it's a bit more
>>>>>> inconvenient to do things that way; but I think it's also much
>>>>>> cleaner.
>>>>> Hmm, if the boot loader could add on to the bootconfig that Masami just
>>>>> added, then it could add some "random" seed for each boot! The
>>>>> bootconfig is just an appended file at the end of the initrd.
>>>> Yeah, it is easy to add bootconfig support to a bootloader. It can add
>>>> a entropy number as "rng.seed=XXX" text after initrd image with size
>>>> and checksum. That is architecutre independent way to pass such hidden
>>>> parameter.
>>>> (hidden key must be filtered out when printing out the /proc/bootconfig,
>>>> but that is very easy too, just need a strncmp)
>>>>
>>> And here is the patch to support "random.rng_seed = XXX" option by
>>> bootconfig. Now you can focus on what you want to do. No need to
>>> modify command line strings.
>> LGTM, our virtualized emulator (cuttlefish) folks believe they can do it
>> this way. Albeit keep in mind that there are _thousands_ of embedded
>> bootloaders out there that are comfortable updating DT and kernel
>> command line, but few that add boot configs, so this may add complexity.
> I see, since the bootconfig is a new feature, it will take a time to
> be supported widely. Even though, maybe they can use DT for that
> purpose.
No for cuttlefish and KVM, there is no DT. WE will backport to 4.19 and 
5.4 Android kernels to grab bootconfig.
>
>> For our use case that caused us to need this, the cuttlefish Android
>> emulated device, not a problem though.
>>
>> However, the entropy _data_ has not been added (see below)
> Oh, I missed that.
>
>> Could you please formally re-submit your patch mhiramet@ with a change
>> to push the _data_ as well to the entropy?
> Yes, I'll do.

Thanks!

>
>> -- Mark
>>
>>> BTW, if you think you need to pass UTF-8 code as a data, I'm happy to
>>> update the bootconfig to support it. Just for the safeness, I have
>>> limited it by isprint() || isspace().
>>>
>>> Thank you,
>>>
>>> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
>>> index 26956c006987..43fbbd307204 100644
>>> --- a/drivers/char/Kconfig
>>> +++ b/drivers/char/Kconfig
>>> @@ -554,6 +554,7 @@ config RANDOM_TRUST_CPU
>>>    
>>>    config RANDOM_TRUST_BOOTLOADER
>>>    	bool "Trust the bootloader to initialize Linux's CRNG"
>>> +	select BOOT_CONFIG
>>>    	help
>>>    	Some bootloaders can provide entropy to increase the kernel's initial
>>>    	device randomness. Say Y here to assume the entropy provided by the
>>> diff --git a/drivers/char/random.c b/drivers/char/random.c
>>> index c7f9584de2c8..0ae33bbbd338 100644
>>> --- a/drivers/char/random.c
>>> +++ b/drivers/char/random.c
>>> @@ -2311,3 +2311,11 @@ void add_bootloader_randomness(const void *buf, unsigned int size)
>>>    		add_device_randomness(buf, size);
>>>    }
>>>    EXPORT_SYMBOL_GPL(add_bootloader_randomness);
>>> +
>>> +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
>>> +/* caller called add_device_randomness, but it is from a trusted source */
>>> +void __init credit_trusted_entropy_bits(unsigned int nbits)
>>> +{
>>> +	credit_entropy_bits(&input_pool, nbits);
>>> +}
>>> +#endif
>>> diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
>>> index 9955d75c0585..aace466c56ed 100644
>>> --- a/fs/proc/bootconfig.c
>>> +++ b/fs/proc/bootconfig.c
>>> @@ -36,6 +36,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
>>>    		ret = xbc_node_compose_key(leaf, key, XBC_KEYLEN_MAX);
>>>    		if (ret < 0)
>>>    			break;
>>> +		/* For keeping security reason, remove randomness key */
>>> +		if (!strcmp(key, RANDOM_SEED_XBC_KEY))
>>> +			continue;
>>>    		ret = snprintf(dst, rest(dst, end), "%s = ", key);
>>>    		if (ret < 0)
>>>    			break;
>>> diff --git a/include/linux/random.h b/include/linux/random.h
>>> index d319f9a1e429..c8f41ab4f342 100644
>>> --- a/include/linux/random.h
>>> +++ b/include/linux/random.h
>>> @@ -20,6 +20,13 @@ struct random_ready_callback {
>>>    
>>>    extern void add_device_randomness(const void *, unsigned int);
>>>    extern void add_bootloader_randomness(const void *, unsigned int);
>>> +#if defined(CONFIG_RANDOM_TRUST_BOOTLOADER)
>>> +extern void __init credit_trusted_entropy_bits(unsigned int nbits);
>>> +#else
>>> +static inline void credit_trusted_entropy_bits(unsigned int nbits) {}
>>> +#endif
>>> +
>>> +#define RANDOM_SEED_XBC_KEY "random.rng_seed"
>>>    
>>>    #if defined(LATENT_ENTROPY_PLUGIN) && !defined(__CHECKER__)
>>>    static inline void add_latent_entropy(void)
>>> diff --git a/init/main.c b/init/main.c
>>> index f95b014a5479..6c3f51bc76d5 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -776,6 +776,32 @@ void __init __weak arch_call_rest_init(void)
>>>    	rest_init();
>>>    }
>>>    
>>> +static __always_inline void __init collect_entropy(const char *command_line)
>>> +{
>>> +	/*
>>> +	 * For best initial stack canary entropy, prepare it after:
>>> +	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
>>> +	 * - timekeeping_init() for ktime entropy used in rand_initialize()
>>> +	 * - rand_initialize() to get any arch-specific entropy like RDRAND
>>> +	 * - add_latent_entropy() to get any latent entropy
>>> +	 * - adding command line entropy
>>> +	 */
>>> +	rand_initialize();
>>> +	add_latent_entropy();
>>> +	add_device_randomness(command_line, strlen(command_line));
>>> +	if (IS_BUILTIN(CONFIG_RANDOM_TRUST_BOOTLOADER)) {
>>> +		/*
>>> +		 * Added bootconfig device randomness above,
>> This part is incorrect, the rng_seed collected below was _not_ added to
>> the device_randomness.
>>
>> add_device_randomness(rng_seed, strlen(rng_seed)) needs to be pushed
>> below along with the credit.
> OK, as same as above command_line, I'll add that.
>
> Thank you,
>
>>> +		 * now add entropy credit for just random.rng_seed=<data>
>>> +		 */
>>> +		const char *rng_seed = xbc_find_value(RANDOM_SEED_XBC_KEY, NULL);
>>> +
>>> +		if (rng_seed)
>>> +			credit_trusted_entropy_bits(strlen(rng_seed) * 6);
>>> +	}
>>> +	boot_init_stack_canary();
>>> +}
>>> +
>>>    asmlinkage __visible void __init start_kernel(void)
>>>    {
>>>    	char *command_line;
>>> @@ -887,18 +913,7 @@ asmlinkage __visible void __init start_kernel(void)
>>>    	softirq_init();
>>>    	timekeeping_init();
>>>    
>>> -	/*
>>> -	 * For best initial stack canary entropy, prepare it after:
>>> -	 * - setup_arch() for any UEFI RNG entropy and boot cmdline access
>>> -	 * - timekeeping_init() for ktime entropy used in rand_initialize()
>>> -	 * - rand_initialize() to get any arch-specific entropy like RDRAND
>>> -	 * - add_latent_entropy() to get any latent entropy
>>> -	 * - adding command line entropy
>>> -	 */
>>> -	rand_initialize();
>>> -	add_latent_entropy();
>>> -	add_device_randomness(command_line, strlen(command_line));
>>> -	boot_init_stack_canary();
>>> +	collect_entropy(command_line);
>>>    
>>>    	time_init();
>>>    	printk_safe_init();
>>>
>
-- MArk