[PATCH] of: fdt: parse early params before adding bootloader randomness

[PATCH] of: fdt: parse early params before adding bootloader randomness

[Jason A. Donenfeld]

FDT is examined so early that it's before the first incidental call to
parse_early_param(). This is similar to EFI, except EFI actually added
an explicitly call to parse_early_param(). Let's do the same here, so
that specifying `random.trust_bootloader=0` is not ignored.

Fixes: d97c68d178fb ("random: treat bootloader trust toggle the same way as cpu trust toggle")
Cc: Rob Herring <robh@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 drivers/of/fdt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 7b571a631639..6d959117fd4f 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1195,6 +1195,9 @@ int __init early_init_dt_scan_chosen(char *cmdline)
 
 	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
 	if (rng_seed && l > 0) {
+		/* Parse random.trust_bootloader if it's in command line. */
+		parse_early_param();
+
 		add_bootloader_randomness(rng_seed, l);
 
 		/* try to clear seed so it won't be found. */
-- 
2.38.1

Re: [PATCH] of: fdt: parse early params before adding bootloader randomness

[Rob Herring]

On Fri, Nov 4, 2022 at 8:46 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> FDT is examined so early that it's before the first incidental call to
> parse_early_param(). This is similar to EFI, except EFI actually added
> an explicitly call to parse_early_param(). Let's do the same here, so
> that specifying `random.trust_bootloader=0` is not ignored.
>
> Fixes: d97c68d178fb ("random: treat bootloader trust toggle the same way as cpu trust toggle")
> Cc: Rob Herring <robh@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
>  drivers/of/fdt.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 7b571a631639..6d959117fd4f 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1195,6 +1195,9 @@ int __init early_init_dt_scan_chosen(char *cmdline)
>
>         rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
>         if (rng_seed && l > 0) {
> +               /* Parse random.trust_bootloader if it's in command line. */
> +               parse_early_param();

I don't think it's good that the timing of calling this is dependent
on "rng-seed" being present or not. So perhaps move it up to after the
cmdline is set.

Either way, the other issue is the cmdline is not necessarily fixed at
this point with some architectures doing their own
append/prepend/override of the cmdline. We can't seem to get common
implementation there finished. I'm doubtful that corner case would
actually be hit though.

Rob

Re: [PATCH] of: fdt: parse early params before adding bootloader randomness

[Jason A. Donenfeld]

On Mon, Nov 07, 2022 at 11:28:20AM -0600, Rob Herring wrote:
> On Fri, Nov 4, 2022 at 8:46 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > FDT is examined so early that it's before the first incidental call to
> > parse_early_param(). This is similar to EFI, except EFI actually added
> > an explicitly call to parse_early_param(). Let's do the same here, so
> > that specifying `random.trust_bootloader=0` is not ignored.
> >
> > Fixes: d97c68d178fb ("random: treat bootloader trust toggle the same way as cpu trust toggle")
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> >  drivers/of/fdt.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 7b571a631639..6d959117fd4f 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -1195,6 +1195,9 @@ int __init early_init_dt_scan_chosen(char *cmdline)
> >
> >         rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
> >         if (rng_seed && l > 0) {
> > +               /* Parse random.trust_bootloader if it's in command line. */
> > +               parse_early_param();
> 
> I don't think it's good that the timing of calling this is dependent
> on "rng-seed" being present or not. So perhaps move it up to after the
> cmdline is set.
> 
> Either way, the other issue is the cmdline is not necessarily fixed at
> this point with some architectures doing their own
> append/prepend/override of the cmdline. We can't seem to get common
> implementation there finished. I'm doubtful that corner case would
> actually be hit though.

Hm, yea. I'm actually now having second thoughts about this one too for
other reasons: FDT isn't the only arch that has this issue. It's also a
problem on x86 and m68k. Maybe the random.trust_bootloader toggle should
just go away, since already your bootloader can do whatever it wants to
the kernel it executes? Not sure; I'll think on it a bit I guess...

Jason

Re: [PATCH] of: fdt: parse early params before adding bootloader randomness

[Rasmus Villemoes]

On 07/11/2022 18.34, Jason A. Donenfeld wrote:
> On Mon, Nov 07, 2022 at 11:28:20AM -0600, Rob Herring wrote:
>> On Fri, Nov 4, 2022 at 8:46 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>>
>>> FDT is examined so early that it's before the first incidental call to
>>> parse_early_param(). This is similar to EFI, except EFI actually added
>>> an explicitly call to parse_early_param(). Let's do the same here, so
>>> that specifying `random.trust_bootloader=0` is not ignored.
>>>
>>> Fixes: d97c68d178fb ("random: treat bootloader trust toggle the same way as cpu trust toggle")
>>> Cc: Rob Herring <robh@kernel.org>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>> ---
>>>  drivers/of/fdt.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 7b571a631639..6d959117fd4f 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -1195,6 +1195,9 @@ int __init early_init_dt_scan_chosen(char *cmdline)
>>>
>>>         rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
>>>         if (rng_seed && l > 0) {
>>> +               /* Parse random.trust_bootloader if it's in command line. */
>>> +               parse_early_param();
>>
>> I don't think it's good that the timing of calling this is dependent
>> on "rng-seed" being present or not. So perhaps move it up to after the
>> cmdline is set.
>>
>> Either way, the other issue is the cmdline is not necessarily fixed at
>> this point with some architectures doing their own
>> append/prepend/override of the cmdline. We can't seem to get common
>> implementation there finished. I'm doubtful that corner case would
>> actually be hit though.
> 
> Hm, yea. I'm actually now having second thoughts about this one too for
> other reasons: FDT isn't the only arch that has this issue. It's also a
> problem on x86 and m68k. Maybe the random.trust_bootloader toggle should
> just go away, since already your bootloader can do whatever it wants to
> the kernel it executes? Not sure; I'll think on it a bit I guess...

Well, for the FDT case it least one could preserve the ability for the
bootloader to say "hey, here's a bunch of bytes, but I have reason to
believe they're not as random as you'd like" by setting a boolean
"rng-seed-is-crap" [pick a better name] property alongside the rng-seed
property. Then the kernel logic could be CONFIG_TRUST_BOOT_LOADER &&
!of_get_bool("rng-seed-is-crap"). Currently, the only thing the
bootloader could do in that case is to just elide the rng-seed
completely (or set the not-actually-working cmdline flag).

I don't think it's a loss to not have a way for the bootloader to turn
on a trust-me flag.

Rasmus

Re: [PATCH] of: fdt: parse early params before adding bootloader randomness

[Jason A. Donenfeld]

On Fri, Nov 18, 2022 at 09:33:53AM +0100, Rasmus Villemoes wrote:
> On 07/11/2022 18.34, Jason A. Donenfeld wrote:
> > On Mon, Nov 07, 2022 at 11:28:20AM -0600, Rob Herring wrote:
> >> On Fri, Nov 4, 2022 at 8:46 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >>>
> >>> FDT is examined so early that it's before the first incidental call to
> >>> parse_early_param(). This is similar to EFI, except EFI actually added
> >>> an explicitly call to parse_early_param(). Let's do the same here, so
> >>> that specifying `random.trust_bootloader=0` is not ignored.
> >>>
> >>> Fixes: d97c68d178fb ("random: treat bootloader trust toggle the same way as cpu trust toggle")
> >>> Cc: Rob Herring <robh@kernel.org>
> >>> Cc: Frank Rowand <frowand.list@gmail.com>
> >>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> >>> ---
> >>>  drivers/of/fdt.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>> index 7b571a631639..6d959117fd4f 100644
> >>> --- a/drivers/of/fdt.c
> >>> +++ b/drivers/of/fdt.c
> >>> @@ -1195,6 +1195,9 @@ int __init early_init_dt_scan_chosen(char *cmdline)
> >>>
> >>>         rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
> >>>         if (rng_seed && l > 0) {
> >>> +               /* Parse random.trust_bootloader if it's in command line. */
> >>> +               parse_early_param();
> >>
> >> I don't think it's good that the timing of calling this is dependent
> >> on "rng-seed" being present or not. So perhaps move it up to after the
> >> cmdline is set.
> >>
> >> Either way, the other issue is the cmdline is not necessarily fixed at
> >> this point with some architectures doing their own
> >> append/prepend/override of the cmdline. We can't seem to get common
> >> implementation there finished. I'm doubtful that corner case would
> >> actually be hit though.
> > 
> > Hm, yea. I'm actually now having second thoughts about this one too for
> > other reasons: FDT isn't the only arch that has this issue. It's also a
> > problem on x86 and m68k. Maybe the random.trust_bootloader toggle should
> > just go away, since already your bootloader can do whatever it wants to
> > the kernel it executes? Not sure; I'll think on it a bit I guess...
> 
> Well, for the FDT case it least one could preserve the ability for the
> bootloader to say "hey, here's a bunch of bytes, but I have reason to
> believe they're not as random as you'd like" by setting a boolean
> "rng-seed-is-crap" [pick a better name] property alongside the rng-seed
> property. Then the kernel logic could be CONFIG_TRUST_BOOT_LOADER &&
> !of_get_bool("rng-seed-is-crap"). Currently, the only thing the
> bootloader could do in that case is to just elide the rng-seed
> completely (or set the not-actually-working cmdline flag).

Yea I don't think we want to introduce that kind of complexity here. No
more knobs :)

Jason