| Message ID | 20221105014613.113503-1-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | of: fdt: parse early params before adding bootloader randomness | expand |
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
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
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
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
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. */
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(+)