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 <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
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
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
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
On Fri, 7 Feb 2020 09:28:27 -0800
Kees Cook <[email protected]> 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
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
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
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
On Fri, 7 Feb 2020 19:49:22 -0500
"Theodore Y. Ts'o" <[email protected]> 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
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.
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
Hi,
On Fri, 7 Feb 2020 19:53:26 -0500
Steven Rostedt <[email protected]> wrote:
> On Fri, 7 Feb 2020 19:49:22 -0500
> "Theodore Y. Ts'o" <[email protected]> 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 <[email protected]>
On Thu, 13 Feb 2020 20:24:54 +0900
Masami Hiramatsu <[email protected]> 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 <[email protected]>
On 2/13/20 7:03 AM, Masami Hiramatsu wrote:
> On Thu, 13 Feb 2020 20:24:54 +0900
> Masami Hiramatsu <[email protected]> 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();
>
Hi Mark,
On Thu, 13 Feb 2020 10:44:59 -0800
Mark Salyzyn <[email protected]> wrote:
> On 2/13/20 7:03 AM, Masami Hiramatsu wrote:
> > On Thu, 13 Feb 2020 20:24:54 +0900
> > Masami Hiramatsu <[email protected]> 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 <[email protected]>
On 2/13/20 5:16 PM, Masami Hiramatsu wrote:
> Hi Mark,
>
> On Thu, 13 Feb 2020 10:44:59 -0800
> Mark Salyzyn <[email protected]> wrote:
>
>> On 2/13/20 7:03 AM, Masami Hiramatsu wrote:
>>> On Thu, 13 Feb 2020 20:24:54 +0900
>>> Masami Hiramatsu <[email protected]> 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