[v8,2/3] fdt: add support for rng-seed
diff mbox series

Message ID 20190819071602.139014-3-hsinyi@chromium.org
State New
Headers show
Series
  • add support for rng-seed
Related show

Commit Message

Hsin-Yi Wang Aug. 19, 2019, 7:16 a.m. UTC
Introducing a chosen node, rng-seed, which is an entropy that can be
passed to kernel called very early to increase initial device
randomness. Bootloader should provide this entropy and the value is
read from /chosen/rng-seed in DT.

Obtain of_fdt_crc32 for CRC check after early_init_dt_scan_nodes(),
since early_init_dt_scan_chosen() would modify fdt to erase rng-seed.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Change from v7:
obtain of_fdt_crc32 after early_init_dt_scan_nodes().
---
 drivers/of/fdt.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Theodore Y. Ts'o Aug. 19, 2019, 6:13 p.m. UTC | #1
On Mon, Aug 19, 2019 at 03:16:04PM +0800, Hsin-Yi Wang wrote:
> Introducing a chosen node, rng-seed, which is an entropy that can be
> passed to kernel called very early to increase initial device
> randomness. Bootloader should provide this entropy and the value is
> read from /chosen/rng-seed in DT.

So it's really cool that you've sent out this patch set.  I've been
wanting this for all platforms / architectures for quite a while.
Question --- are you willing to guarantee that the booloader can be
trusted enough that you *know* the entropy being provided by the
bootloader to be secure?

If so, we could let fdt.c use a different interface, perhaps
add_hwgenerator_randomness(), which allows the bootloader to transfer
trusted entropy for the purposes of initializing the crng and entropy
accounting for /dev/random.

One of the questions is how do we make sure the boot loader is
actually secure, but given that we have to trust the boot loader for
various trusted boot use cases, it seems reasonable to do that.

What do you think?

					- Ted
Hsin-Yi Wang Aug. 20, 2019, 7:42 a.m. UTC | #2
Hi Ted,

Thanks for raising this question.

For UEFI based system, they have a config table that carries rng seed
and can be passed to device randomness. However, they also use
add_device_randomness (not sure if it's the same reason that they
can't guarantee _all_ bootloader can be trusted)
This patch is to let DT based system also have similar features, which
can make initial random number stronger. (We only care initial
situation here, since more entropy would be added to kernel as time
goes on )

Conservatively, we can use add_device_randomness() as well, which
would pass buffer to crng_slow_load() instead of crng_fast_load().
But I think we should trust bootloader here. Whoever wants to use this
feature should make sure their bootloader can pass valid (random
enough) seeds. If they are not sure, they can just don't add the
property to DT.
Ard Biesheuvel Aug. 20, 2019, 11:14 a.m. UTC | #3
On Tue, 20 Aug 2019 at 10:43, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Hi Ted,
>
> Thanks for raising this question.
>
> For UEFI based system, they have a config table that carries rng seed
> and can be passed to device randomness. However, they also use
> add_device_randomness (not sure if it's the same reason that they
> can't guarantee _all_ bootloader can be trusted)

The config table is actually a Linux invention: it is populated by the
EFI stub code (which is part of the kernel) based on the output of a
call into the EFI_RNG_PROTOCOL, which is defined in the UEFI spec, but
optional and not widely available.

I have opted for add_device_randomness() since there is no way to
establish the quality level of the output of EFI_RNG_PROTOCOL, and so
it is currently only used to prevent the bootup state of the entropy
pool to be too predictable, and the output does not contribute to the
entropy estimate kept by the RNG core.


> This patch is to let DT based system also have similar features, which
> can make initial random number stronger. (We only care initial
> situation here, since more entropy would be added to kernel as time
> goes on )
>
> Conservatively, we can use add_device_randomness() as well, which
> would pass buffer to crng_slow_load() instead of crng_fast_load().
> But I think we should trust bootloader here. Whoever wants to use this
> feature should make sure their bootloader can pass valid (random
> enough) seeds. If they are not sure, they can just don't add the
> property to DT.

It is the firmware that adds the property to the DT, not the user.
Hsin-Yi Wang Aug. 21, 2019, 5:57 a.m. UTC | #4
Then we'd still use add_device_randomness() in case that bootloader
provides weak entropy.

On Tue, Aug 20, 2019 at 7:14 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 20 Aug 2019 at 10:43, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > Hi Ted,
> >
> > Thanks for raising this question.
> >
> > For UEFI based system, they have a config table that carries rng seed
> > and can be passed to device randomness. However, they also use
> > add_device_randomness (not sure if it's the same reason that they
> > can't guarantee _all_ bootloader can be trusted)
>
> The config table is actually a Linux invention: it is populated by the
> EFI stub code (which is part of the kernel) based on the output of a
> call into the EFI_RNG_PROTOCOL, which is defined in the UEFI spec, but
> optional and not widely available.
>
> I have opted for add_device_randomness() since there is no way to
> establish the quality level of the output of EFI_RNG_PROTOCOL, and so
> it is currently only used to prevent the bootup state of the entropy
> pool to be too predictable, and the output does not contribute to the
> entropy estimate kept by the RNG core.
>
>
> > This patch is to let DT based system also have similar features, which
> > can make initial random number stronger. (We only care initial
> > situation here, since more entropy would be added to kernel as time
> > goes on )
> >
> > Conservatively, we can use add_device_randomness() as well, which
> > would pass buffer to crng_slow_load() instead of crng_fast_load().
> > But I think we should trust bootloader here. Whoever wants to use this
> > feature should make sure their bootloader can pass valid (random
> > enough) seeds. If they are not sure, they can just don't add the
> > property to DT.
>
> It is the firmware that adds the property to the DT, not the user.
Ard Biesheuvel Aug. 21, 2019, 6:39 a.m. UTC | #5
On Wed, 21 Aug 2019 at 08:57, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Then we'd still use add_device_randomness() in case that bootloader
> provides weak entropy.
>

(please don't top post)

Whether to trust the firmware provided entropy is a policy decision,
and typically, we try to avoid dictating policy in the kernel, and
instead, we try to provide a sane default but give the user control
over it.

So in this case, we should probably introduce
add_firmware_randomness() with a Kconfig/cmdline option pair to decide
whether it should be trusted or not (or reuse the one we have for
trusting RDRAND etc)


> On Tue, Aug 20, 2019 at 7:14 PM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Tue, 20 Aug 2019 at 10:43, Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >
> > > Hi Ted,
> > >
> > > Thanks for raising this question.
> > >
> > > For UEFI based system, they have a config table that carries rng seed
> > > and can be passed to device randomness. However, they also use
> > > add_device_randomness (not sure if it's the same reason that they
> > > can't guarantee _all_ bootloader can be trusted)
> >
> > The config table is actually a Linux invention: it is populated by the
> > EFI stub code (which is part of the kernel) based on the output of a
> > call into the EFI_RNG_PROTOCOL, which is defined in the UEFI spec, but
> > optional and not widely available.
> >
> > I have opted for add_device_randomness() since there is no way to
> > establish the quality level of the output of EFI_RNG_PROTOCOL, and so
> > it is currently only used to prevent the bootup state of the entropy
> > pool to be too predictable, and the output does not contribute to the
> > entropy estimate kept by the RNG core.
> >
> >
> > > This patch is to let DT based system also have similar features, which
> > > can make initial random number stronger. (We only care initial
> > > situation here, since more entropy would be added to kernel as time
> > > goes on )
> > >
> > > Conservatively, we can use add_device_randomness() as well, which
> > > would pass buffer to crng_slow_load() instead of crng_fast_load().
> > > But I think we should trust bootloader here. Whoever wants to use this
> > > feature should make sure their bootloader can pass valid (random
> > > enough) seeds. If they are not sure, they can just don't add the
> > > property to DT.
> >
> > It is the firmware that adds the property to the DT, not the user.
Theodore Y. Ts'o Aug. 21, 2019, 4:21 p.m. UTC | #6
On Wed, Aug 21, 2019 at 09:39:28AM +0300, Ard Biesheuvel wrote:
> 
> Whether to trust the firmware provided entropy is a policy decision,
> and typically, we try to avoid dictating policy in the kernel, and
> instead, we try to provide a sane default but give the user control
> over it.
> 
> So in this case, we should probably introduce
> add_firmware_randomness() with a Kconfig/cmdline option pair to decide
> whether it should be trusted or not (or reuse the one we have for
> trusting RDRAND etc)

I'd call it add_bootloader_randomness(), since we are trusting the
*bootloader*; it's the bootloader which is vouching for the security /
validity of the passed-in entropy.  Furthermore, the bootloader on
some architectures might be fetching directly from some secure
element.

And for that reason, I'd use a different Kconfig/cmdline option pair
than the one used for trusting CPU-provided randomness.

						- Ted

Patch
diff mbox series

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 9cdf14b9aaab..97a75996993c 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -24,6 +24,7 @@ 
 #include <linux/debugfs.h>
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
+#include <linux/random.h>
 
 #include <asm/setup.h>  /* for COMMAND_LINE_SIZE */
 #include <asm/page.h>
@@ -1044,6 +1045,7 @@  int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 {
 	int l;
 	const char *p;
+	const void *rng_seed;
 
 	pr_debug("search \"chosen\", depth: %d, uname: %s\n", depth, uname);
 
@@ -1078,6 +1080,14 @@  int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 
 	pr_debug("Command line is: %s\n", (char*)data);
 
+	rng_seed = of_get_flat_dt_prop(node, "rng-seed", &l);
+	if (rng_seed && l > 0) {
+		add_device_randomness(rng_seed, l);
+
+		/* try to clear seed so it won't be found. */
+		fdt_nop_property(initial_boot_params, node, "rng-seed");
+	}
+
 	/* break now */
 	return 1;
 }
@@ -1166,8 +1176,6 @@  bool __init early_init_dt_verify(void *params)
 
 	/* Setup flat device-tree pointer */
 	initial_boot_params = params;
-	of_fdt_crc32 = crc32_be(~0, initial_boot_params,
-				fdt_totalsize(initial_boot_params));
 	return true;
 }
 
@@ -1197,6 +1205,8 @@  bool __init early_init_dt_scan(void *params)
 		return false;
 
 	early_init_dt_scan_nodes();
+	of_fdt_crc32 = crc32_be(~0, initial_boot_params,
+				fdt_totalsize(initial_boot_params));
 	return true;
 }