[PATCH 0/3] random: add random.rng_seed to bootconfig entry

[PATCH 0/3] random: add random.rng_seed to bootconfig entry

[Masami Hiramatsu]

Hi,

The following series is bootconfig based implementation of
the rng_seed option patch originally from Mark Salyzyn.
Note that I removed unrelated command line fixes from this
series.

To complete the support of UTF-8 for rng_seed, I added [1/3]
to support non-ascii chars on the value (user can use 0x80-
0xff at the value of bootconfig).

For [3/3], I updated to use bootconfig (xbc_find_value)
instead of command line. Also move the documentation under
Documentation/admin-guide/bootconfig.

Thank you,

---

Mark Salyzyn (2):
      random: rng-seed source is utf-8
      random: add random.rng_seed= bootconfig option

Masami Hiramatsu (1):
      bootconfig: Support non-ascii characters in value


 Documentation/admin-guide/bootconfig/random.rst |   21 ++++++++++++
 drivers/char/Kconfig                            |    1 +
 drivers/char/random.c                           |   10 +++++-
 fs/proc/bootconfig.c                            |    4 ++
 include/linux/random.h                          |    7 ++++
 init/main.c                                     |   41 ++++++++++++++++-------
 lib/bootconfig.c                                |    2 +
 tools/bootconfig/samples/good-non-ascii.bconf   |    1 +
 8 files changed, 73 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/admin-guide/bootconfig/random.rst
 create mode 100644 tools/bootconfig/samples/good-non-ascii.bconf

-- 
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/3] bootconfig: Support non-ascii characters in value

[Masami Hiramatsu]

Support non-ascii (u8 code > 128) characters in the
value data. This will allow user to pass utf-8 data
as a value in a bootconfig. (Note that ascii non-
printable characters are still not allowed.)

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 lib/bootconfig.c                              |    2 +-
 tools/bootconfig/samples/good-non-ascii.bconf |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
 create mode 100644 tools/bootconfig/samples/good-non-ascii.bconf

diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 3ea601a2eba5..9f15b8bef3a0 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -466,7 +466,7 @@ static int __init __xbc_parse_value(char **__v, char **__n)
 	}
 	p = v - 1;
 	while ((c = *++p)) {
-		if (!isprint(c) && !isspace(c))
+		if (c >= 0 && !isprint(c) && !isspace(c))
 			return xbc_parse_error("Non printable value", p);
 		if (quotes) {
 			if (c != quotes)
diff --git a/tools/bootconfig/samples/good-non-ascii.bconf b/tools/bootconfig/samples/good-non-ascii.bconf
new file mode 100644
index 000000000000..b6cb4c24fad6
--- /dev/null
+++ b/tools/bootconfig/samples/good-non-ascii.bconf
@@ -0,0 +1 @@
+hello.japanese = "こんにちは"

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

[Masami Hiramatsu]

From: Mark Salyzyn <salyzyn@android.com>

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.

Fixes: 428826f5358c ("fdt: add support for rng-seed")
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 c7f9584de2c8..ee21a6a584b1 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);
 }

[PATCH 3/3] random: add random.rng_seed= bootconfig option

[Masami Hiramatsu]

From: Mark Salyzyn <salyzyn@android.com>

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 bootconfig.

If CONFIG_RANDOM_TRUST_BOOTLOADER is set, then feed the
random.rng_seed bootconfig data length as added trusted
entropy.

Always erase view of the random.rng_seed option from
/proc/bootconfig to prevent leakage to applications or modules,
to eliminate any attack vector.  Note that initcall embedded
code still have a chance to see it, but that will be unsafe
at different level.

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 bootconfig-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>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
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: "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>
---
v4
- Use bootconfig instead of command line
- Move the documentation under Documentation/admin-guide/bootconfig/.
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.
---
 Documentation/admin-guide/bootconfig/random.rst |   21 ++++++++++++
 drivers/char/Kconfig                            |    1 +
 drivers/char/random.c                           |    8 ++++
 fs/proc/bootconfig.c                            |    4 ++
 include/linux/random.h                          |    7 ++++
 init/main.c                                     |   41 ++++++++++++++++-------
 6 files changed, 70 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/admin-guide/bootconfig/random.rst

diff --git a/Documentation/admin-guide/bootconfig/random.rst b/Documentation/admin-guide/bootconfig/random.rst
new file mode 100644
index 000000000000..d4ee513c5136
--- /dev/null
+++ b/Documentation/admin-guide/bootconfig/random.rst
@@ -0,0 +1,21 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===============================
+The Random Subsystem Bootconfig
+===============================
+
+The keys start with "random." configures random number generator subsystem.
+
+Options
+=======
+
+random.rng_seed
+  Provide a trusted seed for the kernel's CRNG. Seed only trusted if
+  CONFIG_RANDOM_TRUST_BOOTLOADER=y.  After collection, this option is not
+  shown in /proc/bootconfig.
+  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.
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 ee21a6a584b1..83c77306e18e 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..6d1a819f2df4 100644
--- a/fs/proc/bootconfig.c
+++ b/fs/proc/bootconfig.c
@@ -8,6 +8,7 @@
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
 #include <linux/bootconfig.h>
+#include <linux/random.h>
 #include <linux/slab.h>
 
 static char *saved_boot_config;
@@ -36,6 +37,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..d0e5a95b4182 100644
--- a/init/main.c
+++ b/init/main.c
@@ -776,6 +776,34 @@ 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) {
+			add_device_randomness(rng_seed, strlen(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 +915,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 0/3] random: add random.rng_seed to bootconfig entry

[Rob Herring]

On Fri, Feb 14, 2020 at 12:10 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi,
>
> The following series is bootconfig based implementation of
> the rng_seed option patch originally from Mark Salyzyn.
> Note that I removed unrelated command line fixes from this
> series.

Why do we need this? There's already multiple other ways to pass
random seed and this doesn't pass the "too complex for the command
line" argument you had for needing bootconfig.

Rob

Re: [PATCH 0/3] random: add random.rng_seed to bootconfig entry

[Mark Salyzyn]

On 2/14/20 5:49 AM, Rob Herring wrote:
> On Fri, Feb 14, 2020 at 12:10 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>> Hi,
>>
>> The following series is bootconfig based implementation of
>> the rng_seed option patch originally from Mark Salyzyn.
>> Note that I removed unrelated command line fixes from this
>> series.
> Why do we need this? There's already multiple other ways to pass
> random seed and this doesn't pass the "too complex for the command
> line" argument you had for needing bootconfig.
>
> Rob

Android is the use case I can vouch for. But also KVM.

Android Cuttlefish is an emulated device used extensively in the testing 
and development infrastructure for In-house, partner, and system and 
application developers for Android. There is no bootloader, per-se. 
Because of the Android GKI distribution, there is also no rng virtual 
driver built in, it is loaded later as a module, too late for many 
aspects of KASLR and networking. There is no Device Tree, it does 
however have access to the content of the initrd image, and to the 
command line for the kernel. The only convenient way to get early 
entropy is going to have to be one of those two places.

In addition, 2B Android devices on the planet, especially in light of 
the Android GKI distribution were everything that is vendor created is 
in a module, needs a way to collect early entropy prior to module load 
and pass it to the kernel. Yes, they do have access to the recently 
added Device Tree approach, and we expect them to use it, as I have an 
active backport for the mechanism into the Android 4.19 and 5.4 kernels. 
There may also be some benefit to allowing the 13000 different 
bootloaders an option to use bootconfig as a way of propagating the much 
needed entropy to their kernels. I could make a case to also allow them 
command line as another option to relieve their development stress to 
deliver product, but we can stop there. Regardless, this early entropy 
has the benefit of greatly improving security and precious boot time.

Sincerely -- Mark Salyzyn

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Hsin-Yi Wang]

On Fri, Feb 14, 2020 at 2:10 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> From: Mark Salyzyn <salyzyn@android.com>
>
> 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.
>
> Fixes: 428826f5358c ("fdt: add support for rng-seed")
> 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 c7f9584de2c8..ee21a6a584b1 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);
Hi,

In the next patch, entropy is added by
+                       add_device_randomness(rng_seed, strlen(rng_seed));
+                       credit_trusted_entropy_bits(strlen(rng_seed) * 6);

If the add_bootloader_randomness() function is only used for dt, do we
need to shorten the credit bits?

In dt-schema[1] we stated that this is a uint8 array, and dt is able
to generate this. It doesn't need to avoid using space for parameter
splitting.

For some device, asking for random number is time consuming. Shorten
the credit length makes it have to generate longer seed for dt to meet
the CRNG_INIT_CNT_THRESH threshold.

[1] https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml#L55

Thanks

Re: [PATCH 0/3] random: add random.rng_seed to bootconfig entry

[Rob Herring]

On Fri, Feb 14, 2020 at 11:00 AM Mark Salyzyn <salyzyn@android.com> wrote:
>
> On 2/14/20 5:49 AM, Rob Herring wrote:
> > On Fri, Feb 14, 2020 at 12:10 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >> Hi,
> >>
> >> The following series is bootconfig based implementation of
> >> the rng_seed option patch originally from Mark Salyzyn.
> >> Note that I removed unrelated command line fixes from this
> >> series.
> > Why do we need this? There's already multiple other ways to pass
> > random seed and this doesn't pass the "too complex for the command
> > line" argument you had for needing bootconfig.
> >
> > Rob
>
> Android is the use case I can vouch for. But also KVM.
>
> Android Cuttlefish is an emulated device used extensively in the testing
> and development infrastructure for In-house, partner, and system and
> application developers for Android. There is no bootloader, per-se.
> Because of the Android GKI distribution, there is also no rng virtual
> driver built in, it is loaded later as a module, too late for many
> aspects of KASLR and networking. There is no Device Tree, it does
> however have access to the content of the initrd image, and to the
> command line for the kernel. The only convenient way to get early
> entropy is going to have to be one of those two places.

I'm familiar with Cuttlefish somewhat. Guess who got virtio-gpu
working on Android[1]. :) I assume DT doesn't work for you because you
need x86 builds, but doesn't QEMU use UEFI in that case which also has
a mechanism for passing entropy.

To clarify my question: Why do we need random seed in bootconfig
rather than just the kernel command line? I'm not understanding why
things changed from your original patch.

> In addition, 2B Android devices on the planet, especially in light of
> the Android GKI distribution were everything that is vendor created is
> in a module, needs a way to collect early entropy prior to module load
> and pass it to the kernel. Yes, they do have access to the recently
> added Device Tree approach, and we expect them to use it, as I have an
> active backport for the mechanism into the Android 4.19 and 5.4 kernels.
> There may also be some benefit to allowing the 13000 different
> bootloaders an option to use bootconfig as a way of propagating the much
> needed entropy to their kernels. I could make a case to also allow them
> command line as another option to relieve their development stress to
> deliver product, but we can stop there. Regardless, this early entropy
> has the benefit of greatly improving security and precious boot time.

We're going to update 13000 bootloaders to understand bootconfig
rather than use the infrastructure already in place (DT and/or command
line)?

bootconfig is an ftrace feature only IMO. If it's more than that, I
imagine there will be some opinions about that. Adding new
bootloader-kernel interfaces is painful and not something to just add
without much review.

Rob

Re: [PATCH 0/3] random: add random.rng_seed to bootconfig entry

[Mark Salyzyn]

On 2/14/20 10:14 AM, Rob Herring wrote:
> On Fri, Feb 14, 2020 at 11:00 AM Mark Salyzyn <salyzyn@android.com> wrote:
>> On 2/14/20 5:49 AM, Rob Herring wrote:
>>> On Fri, Feb 14, 2020 at 12:10 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>>> Hi,
>>>>
>>>> The following series is bootconfig based implementation of
>>>> the rng_seed option patch originally from Mark Salyzyn.
>>>> Note that I removed unrelated command line fixes from this
>>>> series.
>>> Why do we need this? There's already multiple other ways to pass
>>> random seed and this doesn't pass the "too complex for the command
>>> line" argument you had for needing bootconfig.
>>>
>>> Rob
>> Android is the use case I can vouch for. But also KVM.
. . .
> I'm familiar with Cuttlefish somewhat. Guess who got virtio-gpu
> working on Android[1]. :) I assume DT doesn't work for you because you
> need x86 builds, but doesn't QEMU use UEFI in that case which also has
> a mechanism for passing entropy.
IDK, will have to ask the Cuttlefish Team why UEFI not being used, will 
get back to you.
>
> To clarify my question: Why do we need random seed in bootconfig
> rather than just the kernel command line? I'm not understanding why
> things changed from your original patch.

Command line was identified as the simplest for them to implement 
generically for the x86 and arm64 Cuttlefish instances and hence my 
original patch series.

However, it also is limited in the size of the entropy string that can 
be provided, so we flipped a coin and decided to accept the bootconfig 
mechanism as a viable alternative; that BTW appeared to be simpler to 
implement (mainly because rubbing out the entropy command line argument 
is not easy).

-- Mark

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Rob Herring]

On Fri, Feb 14, 2020 at 12:10 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> From: Mark Salyzyn <salyzyn@android.com>
>
> 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

Typically? Why is that?

> characters which has a bitness of roughly 6 to give appropriate
> credit due for the entropy.

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Theodore Y. Ts'o]

On Fri, Feb 14, 2020 at 01:58:35PM -0600, Rob Herring wrote:
> On Fri, Feb 14, 2020 at 12:10 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > From: Mark Salyzyn <salyzyn@android.com>
> >
> > 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
> 
> Typically? Why is that?
> 
> > characters which has a bitness of roughly 6 to give appropriate
> > credit due for the entropy.

This is why I really think what gets specified via the boot command
line, or bootconfig, should specify the bits of entropy and the
entropy seed *separately*, so it can be specified explicitly, instead
of assuming that *everyone knows* that rng-seed is either (a) a binary
string, or (b) utf-8, or (c) a hex string.  The fact is, everyone does
*not* know, or everyone will have a different implementation, which
everyone will say is *obviously* the only way to go....

	      	     		     	  - Ted

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Mark Salyzyn]

On 2/14/20 2:47 PM, Theodore Y. Ts'o wrote:
> On Fri, Feb 14, 2020 at 01:58:35PM -0600, Rob Herring wrote:
>> On Fri, Feb 14, 2020 at 12:10 AM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>> From: Mark Salyzyn <salyzyn@android.com>
>>>
>>> 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
>> Typically? Why is that?
>>
>>> characters which has a bitness of roughly 6 to give appropriate
>>> credit due for the entropy.
> This is why I really think what gets specified via the boot command
> line, or bootconfig, should specify the bits of entropy and the
> entropy seed *separately*, so it can be specified explicitly, instead
> of assuming that *everyone knows* that rng-seed is either (a) a binary
> string, or (b) utf-8, or (c) a hex string.  The fact is, everyone does
> *not* know, or everyone will have a different implementation, which
> everyone will say is *obviously* the only way to go....
>
> 	      	     		     	  - Ted

Given that the valid option are between 4 (hex), 6 (utf-8) or 8 
(binary), we can either split the difference and accept 6; or take a 
pass at the values and determine which of the set they belong to 
[0-9a-fA-F], [!-~] or [\000-\377]  nor need to separately specify.

-- Mark

Re: [PATCH 0/3] random: add random.rng_seed to bootconfig entry

[Masami Hiramatsu]

Hi Rob,

On Fri, 14 Feb 2020 12:14:53 -0600
Rob Herring <robh@kernel.org> wrote:

> To clarify my question: Why do we need random seed in bootconfig
> rather than just the kernel command line? I'm not understanding why
> things changed from your original patch.

I recommended to use it in the previous thread, because of simplicity.
Since it has to hide from userspace and modules, it needs to modify
kernel command line. But the bootconfig can make it simple, and it
also architecture independent.

> > In addition, 2B Android devices on the planet, especially in light of
> > the Android GKI distribution were everything that is vendor created is
> > in a module, needs a way to collect early entropy prior to module load
> > and pass it to the kernel. Yes, they do have access to the recently
> > added Device Tree approach, and we expect them to use it, as I have an
> > active backport for the mechanism into the Android 4.19 and 5.4 kernels.

FYI, I backported bootconfig with boot-time tracer for 4.19 stable kernel
recently.

https://github.com/mhiramat/linux/commits/ftrace-boottrace-4.19

You can check what commits are related.

> > There may also be some benefit to allowing the 13000 different
> > bootloaders an option to use bootconfig as a way of propagating the much
> > needed entropy to their kernels. I could make a case to also allow them
> > command line as another option to relieve their development stress to
> > deliver product, but we can stop there. Regardless, this early entropy
> > has the benefit of greatly improving security and precious boot time.
> 
> We're going to update 13000 bootloaders to understand bootconfig
> rather than use the infrastructure already in place (DT and/or command
> line)?
> 
> bootconfig is an ftrace feature only IMO. If it's more than that, I
> imagine there will be some opinions about that. Adding new
> bootloader-kernel interfaces is painful and not something to just add
> without much review.

The bootconfig itself is designed as a generic feature. I had tried to use
devicetree, but that was rejected. Thus I made it as a "software
configuration tree" (but far simpler.)
 
If you have any review comment on the bootconfig, always welcome!
Seriously, I would like to have more comments. I want to make it better.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Theodore Y. Ts'o]

On Fri, Feb 14, 2020 at 02:55:36PM -0800, Mark Salyzyn wrote:
> > This is why I really think what gets specified via the boot command
> > line, or bootconfig, should specify the bits of entropy and the
> > entropy seed *separately*, so it can be specified explicitly, instead
> > of assuming that *everyone knows* that rng-seed is either (a) a binary
> > string, or (b) utf-8, or (c) a hex string.  The fact is, everyone does
> > *not* know, or everyone will have a different implementation, which
> > everyone will say is *obviously* the only way to go....
> > 
> Given that the valid option are between 4 (hex), 6 (utf-8) or 8 (binary), we
> can either split the difference and accept 6; or take a pass at the values
> and determine which of the set they belong to [0-9a-fA-F], [!-~] or
> [\000-\377]  nor need to separately specify.

So let's split this up into separate issues.  First of all, from an
architectural issue, I really think we need to change
add_bootloader_randomness() in drivers/char/random.c so it looks like this:

void add_bootloader_randomness(const void *buf, unsigned int size, unsigned int entropy_bits)

That's because this is a general function that could be used by any
number of bootloaders.  For example, for the UEFI bootloader, it can
use the UEFI call that will return binary bits.  Some other bootloader
might use utf-8, etc.  So it would be an abstraction violation to have
code in drivers/char/random.c make assumption about how a particular
bootloader might be behaving.

The second question is we are going to be parsing an rng_seed
parameter it shows up in bootconfig or in the boot command line, how
do we decide how many bits of entropy it actually has.  The advantage
of using the boot command line is we don't need to change the rest of
the bootloader ecosystem.  But that's also a massive weakness, since
apparently some people are already using it, and perhaps not in the
same way.

So what I'd really prefer is if we use something new, and we define it
in a way that makes as close as possible to "impossible to misuse".
(See Rusty Russell's API design manifesto[1]).  So I'm going to
propose something different.  Let's use something new, say
entropy_seed_hex, and let's say that it *must* be a hex string:

    entropy_seed_hex=7337db91a4824e3480ba6d2dfaa60bec

If it is not a valid hex string, it gets zero entropy credit.

I don't think we really need to worry about efficient encoding of the
seed, since 256 bits is only 64 characters using a hex string.  And
whether it's 32 characters or 64 characters, the max command line
string is 32k, so it's probably not worth it to try to do something
more complex.  (And only 128 bits is needed to declare the CRNG to be
fully initialized, in which case we're talking about 16 characters
versus 32 charaters.)

[1] http://sweng.the-davies.net/Home/rustys-api-design-manifesto

						- Ted

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Mark Salyzyn]

On 2/14/20 4:53 PM, Theodore Y. Ts'o wrote:
> On Fri, Feb 14, 2020 at 02:55:36PM -0800, Mark Salyzyn wrote:
>>> This is why I really think what gets specified via the boot command
>>> line, or bootconfig, should specify the bits of entropy and the
>>> entropy seed *separately*, so it can be specified explicitly, instead
>>> of assuming that *everyone knows* that rng-seed is either (a) a binary
>>> string, or (b) utf-8, or (c) a hex string.  The fact is, everyone does
>>> *not* know, or everyone will have a different implementation, which
>>> everyone will say is *obviously* the only way to go....
>>>
>> Given that the valid option are between 4 (hex), 6 (utf-8) or 8 (binary), we
>> can either split the difference and accept 6; or take a pass at the values
>> and determine which of the set they belong to [0-9a-fA-F], [!-~] or
>> [\000-\377]  nor need to separately specify.
> So let's split this up into separate issues.  First of all, from an
> architectural issue, I really think we need to change
> add_bootloader_randomness() in drivers/char/random.c so it looks like this:
>
> void add_bootloader_randomness(const void *buf, unsigned int size, unsigned int entropy_bits)
>
> That's because this is a general function that could be used by any
> number of bootloaders.  For example, for the UEFI bootloader, it can
> use the UEFI call that will return binary bits.  Some other bootloader
> might use utf-8, etc.  So it would be an abstraction violation to have
> code in drivers/char/random.c make assumption about how a particular
> bootloader might be behaving.
>
> The second question is we are going to be parsing an rng_seed
> parameter it shows up in bootconfig or in the boot command line, how
> do we decide how many bits of entropy it actually has.  The advantage
> of using the boot command line is we don't need to change the rest of
> the bootloader ecosystem.  But that's also a massive weakness, since
> apparently some people are already using it, and perhaps not in the
> same way.
>
> So what I'd really prefer is if we use something new, and we define it
> in a way that makes as close as possible to "impossible to misuse".
> (See Rusty Russell's API design manifesto[1]).  So I'm going to
> propose something different.  Let's use something new, say
> entropy_seed_hex, and let's say that it *must* be a hex string:
>
>      entropy_seed_hex=7337db91a4824e3480ba6d2dfaa60bec
>
> If it is not a valid hex string, it gets zero entropy credit.
>
> I don't think we really need to worry about efficient encoding of the
> seed, since 256 bits is only 64 characters using a hex string.  An
> whether it's 32 characters or 64 characters, the max command line
> string is 32k, so it's probably not worth it to try to do something
> more complex.  (And only 128 bits is needed to declare the CRNG to be
> fully initialized, in which case we're talking about 16 characters
> versus 32 charaters.)
>
> [1] http://sweng.the-davies.net/Home/rustys-api-design-manifesto
>
> 						- Ted
>
I am additionally concerned about add_bootloader_randomness() because it 
is possible for it to sleep because of add_hwgenerator_randomness() as 
once it hits the entropy threshold. As-is it can not be used inside 
start_kernel() because the sleep would result in a kernel panic, and I 
suspect its use inside early_init_dt_scan_chosen() for the commit "fdt: 
add support for rng-seed" might also be problematic since it is 
effectively called underneath start_kernel() is it not?

If add_bootloader_randomness was rewritten to call 
add_device_randomness() always, and when trusted also called the 
functionality of the new credit_trusted_entropy_bits (no longer needing 
to be exported if so), then the function could be used in both 
start_kernel() and early_init_dt_scan_chosen().


-- Mark

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Hsin-Yi Wang]

On Wed, Feb 19, 2020 at 12:01 AM Mark Salyzyn <salyzyn@android.com> wrote:
>
> On 2/14/20 4:53 PM, Theodore Y. Ts'o wrote:
> > On Fri, Feb 14, 2020 at 02:55:36PM -0800, Mark Salyzyn wrote:
> >>> This is why I really think what gets specified via the boot command
> >>> line, or bootconfig, should specify the bits of entropy and the
> >>> entropy seed *separately*, so it can be specified explicitly, instead
> >>> of assuming that *everyone knows* that rng-seed is either (a) a binary
> >>> string, or (b) utf-8, or (c) a hex string.  The fact is, everyone does
> >>> *not* know, or everyone will have a different implementation, which
> >>> everyone will say is *obviously* the only way to go....
> >>>
> >> Given that the valid option are between 4 (hex), 6 (utf-8) or 8 (binary), we
> >> can either split the difference and accept 6; or take a pass at the values
> >> and determine which of the set they belong to [0-9a-fA-F], [!-~] or
> >> [\000-\377]  nor need to separately specify.
> > So let's split this up into separate issues.  First of all, from an
> > architectural issue, I really think we need to change
> > add_bootloader_randomness() in drivers/char/random.c so it looks like this:
> >
> > void add_bootloader_randomness(const void *buf, unsigned int size, unsigned int entropy_bits)
> >
> > That's because this is a general function that could be used by any
> > number of bootloaders.  For example, for the UEFI bootloader, it can
> > use the UEFI call that will return binary bits.  Some other bootloader
> > might use utf-8, etc.  So it would be an abstraction violation to have
> > code in drivers/char/random.c make assumption about how a particular
> > bootloader might be behaving.
> >
> > The second question is we are going to be parsing an rng_seed
> > parameter it shows up in bootconfig or in the boot command line, how
> > do we decide how many bits of entropy it actually has.  The advantage
> > of using the boot command line is we don't need to change the rest of
> > the bootloader ecosystem.  But that's also a massive weakness, since
> > apparently some people are already using it, and perhaps not in the
> > same way.
> >
> > So what I'd really prefer is if we use something new, and we define it
> > in a way that makes as close as possible to "impossible to misuse".
> > (See Rusty Russell's API design manifesto[1]).  So I'm going to
> > propose something different.  Let's use something new, say
> > entropy_seed_hex, and let's say that it *must* be a hex string:
> >
> >      entropy_seed_hex=7337db91a4824e3480ba6d2dfaa60bec
> >
> > If it is not a valid hex string, it gets zero entropy credit.
> >
> > I don't think we really need to worry about efficient encoding of the
> > seed, since 256 bits is only 64 characters using a hex string.  An
> > whether it's 32 characters or 64 characters, the max command line
> > string is 32k, so it's probably not worth it to try to do something
> > more complex.  (And only 128 bits is needed to declare the CRNG to be
> > fully initialized, in which case we're talking about 16 characters
> > versus 32 charaters.)
> >
> > [1] http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> >
> >                                               - Ted
> >
> I am additionally concerned about add_bootloader_randomness() because it
> is possible for it to sleep because of add_hwgenerator_randomness() as
> once it hits the entropy threshold. As-is it can not be used inside
> start_kernel() because the sleep would result in a kernel panic, and I
> suspect its use inside early_init_dt_scan_chosen() for the commit "fdt:
> add support for rng-seed" might also be problematic since it is
> effectively called underneath start_kernel() is it not?
>
> If add_bootloader_randomness was rewritten to call
> add_device_randomness() always, and when trusted also called the
> functionality of the new credit_trusted_entropy_bits (no longer needing
> to be exported if so), then the function could be used in both
> start_kernel() and early_init_dt_scan_chosen().
>
I tested 64 bytes rng-seed previously so didn't hit the threshold that
makes it suspend. Thanks for pointing this out.
+1 for changing the add_bootloader_randomness() function as you
suggested to avoid this issue. But besides credit_entropy_bits(), they
are also different on crng_init (crng_fast_load/crng_slow_load).

Re: [PATCH 2/3] random: rng-seed source is utf-8

[Theodore Y. Ts'o]

On Tue, Feb 18, 2020 at 08:01:51AM -0800, Mark Salyzyn wrote:
> I am additionally concerned about add_bootloader_randomness() because it is
> possible for it to sleep because of add_hwgenerator_randomness() as once it
> hits the entropy threshold. As-is it can not be used inside start_kernel()
> because the sleep would result in a kernel panic, and I suspect its use
> inside early_init_dt_scan_chosen() for the commit "fdt: add support for
> rng-seed" might also be problematic since it is effectively called
> underneath start_kernel() is it not?
> 
> If add_bootloader_randomness was rewritten to call add_device_randomness()
> always, and when trusted also called the functionality of the new
> credit_trusted_entropy_bits (no longer needing to be exported if so), then
> the function could be used in both start_kernel() and
> early_init_dt_scan_chosen().

That's a good point, and it's a bug in add_bootloader_randomness().
That should be easily fixed by simply having it call mix_pool_bytes()
and credit_entropy_bits() directly.  I'll create a patch...

    			  	     	  	   - Ted