[PATCH] arm64: add support for rng-seed

[PATCH] arm64: add support for rng-seed

[Hsin-Yi Wang]

Introducing a chosen node, rng-seed, which is an 64 bytes entropy
that can be passed to kernel called very early to increase device
randomness. Bootloader should provide this entropy and the value is
read from /chosen/rng-seed in DT.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>

---
 Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
 arch/arm64/kernel/setup.c                    |  2 ++
 drivers/of/fdt.c                             | 33 ++++++++++++++++++++
 include/linux/of_fdt.h                       |  1 +
 4 files changed, 50 insertions(+)

diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
index 45e79172a646..bfd360691650 100644
--- a/Documentation/devicetree/bindings/chosen.txt
+++ b/Documentation/devicetree/bindings/chosen.txt
@@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
 the Linux EFI stub (which will populate the property itself, using
 EFI_RNG_PROTOCOL).
 
+rng-seed
+-----------
+
+This property served as an entropy to add device randomness. It is parsed
+as a 64 byte value, e.g.
+
+/ {
+	chosen {
+		rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
+	};
+};
+
+This random value should be provided by bootloader.
+
 stdout-path
 -----------
 
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 413d566405d1..ade4261516dd 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
 	early_fixmap_init();
 	early_ioremap_init();
 
+	early_init_dt_rng_seed(__fdt_pointer);
+
 	setup_machine_fdt(__fdt_pointer);
 
 	parse_early_param();
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index de893c9616a1..74e2c0c80b91 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/libfdt.h>
 #include <linux/debugfs.h>
+#include <linux/random.h>
 #include <linux/serial_core.h>
 #include <linux/sysfs.h>
 
@@ -1117,6 +1118,38 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
 	return 1;
 }
 
+extern void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size,
+				       pgprot_t prot);
+
+void __init early_init_dt_rng_seed(u64 dt_phys)
+{
+	void *fdt;
+	int node, size, i;
+	fdt64_t *prop;
+	u64 rng_seed[8];
+
+	fdt = __fixmap_remap_fdt(dt_phys, &size, PAGE_KERNEL);
+	if (!fdt)
+		return;
+
+	node = fdt_path_offset(fdt, "/chosen");
+	if (node < 0)
+		return;
+
+	prop = fdt_getprop_w(fdt, node, "rng-seed", &size);
+	if (!prop || size != sizeof(u64) * 8)
+		return;
+
+	for (i = 0; i < 8; i++) {
+		rng_seed[i] = fdt64_to_cpu(*(prop + i));
+		/* clear seed so it won't be found. */
+		*(prop + i) = 0;
+	}
+	add_device_randomness(rng_seed, size);
+
+	return;
+}
+
 #ifndef MIN_MEMBLOCK_ADDR
 #define MIN_MEMBLOCK_ADDR	__pa(PAGE_OFFSET)
 #endif
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index a713e5d156d8..a4548dd6351e 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -71,6 +71,7 @@ extern uint32_t of_get_flat_dt_phandle(unsigned long node);
 
 extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
 				     int depth, void *data);
+extern void early_init_dt_rng_seed(u64 dt_phys);
 extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
 				     int depth, void *data);
 extern int early_init_dt_scan_chosen_stdout(void);
-- 
2.20.1

Re: [PATCH] arm64: add support for rng-seed

[Hsin-Yi Wang]

On Tue, May 7, 2019 at 12:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> that can be passed to kernel called very early to increase device
> randomness. Bootloader should provide this entropy and the value is
> read from /chosen/rng-seed in DT.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
>
> ---
>  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
>  arch/arm64/kernel/setup.c                    |  2 ++
>  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
>  include/linux/of_fdt.h                       |  1 +
>  4 files changed, 50 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 45e79172a646..bfd360691650 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
>  the Linux EFI stub (which will populate the property itself, using
>  EFI_RNG_PROTOCOL).
>
> +rng-seed
> +-----------
> +
> +This property served as an entropy to add device randomness. It is parsed
> +as a 64 byte value, e.g.
> +
> +/ {
> +       chosen {
> +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> +       };
> +};
> +
> +This random value should be provided by bootloader.
> +
>  stdout-path
>  -----------
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 413d566405d1..ade4261516dd 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
>         early_fixmap_init();
>         early_ioremap_init();
>
> +       early_init_dt_rng_seed(__fdt_pointer);
Currently this can only be called before setup_machine_fdt(). Since
setup_machine_fdt() called fixmap_remap_fdt() //
__fixmap_remap_fdt(dt_phys, &size, PAGE_KERNEL_RO), we can't modify DT
after that. And rng-seed needs to be wiped out after read.
Another option is to called earlier, at arch/arm64/kernel/head.S,
similar to kaslr_early_init.

> +
>         setup_machine_fdt(__fdt_pointer);
>
>         parse_early_param();
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index de893c9616a1..74e2c0c80b91 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/libfdt.h>
>  #include <linux/debugfs.h>
> +#include <linux/random.h>
>  #include <linux/serial_core.h>
>  #include <linux/sysfs.h>
>
> @@ -1117,6 +1118,38 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>         return 1;
>  }
>
> +extern void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size,
> +                                      pgprot_t prot);
> +
> +void __init early_init_dt_rng_seed(u64 dt_phys)
> +{
> +       void *fdt;
> +       int node, size, i;
> +       fdt64_t *prop;
> +       u64 rng_seed[8];
> +
> +       fdt = __fixmap_remap_fdt(dt_phys, &size, PAGE_KERNEL);
> +       if (!fdt)
> +               return;
> +
> +       node = fdt_path_offset(fdt, "/chosen");
> +       if (node < 0)
> +               return;
> +
> +       prop = fdt_getprop_w(fdt, node, "rng-seed", &size);
> +       if (!prop || size != sizeof(u64) * 8)
> +               return;
> +
> +       for (i = 0; i < 8; i++) {
> +               rng_seed[i] = fdt64_to_cpu(*(prop + i));
> +               /* clear seed so it won't be found. */
> +               *(prop + i) = 0;
> +       }
> +       add_device_randomness(rng_seed, size);
> +
> +       return;
> +}
> +
>  #ifndef MIN_MEMBLOCK_ADDR
>  #define MIN_MEMBLOCK_ADDR      __pa(PAGE_OFFSET)
>  #endif
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index a713e5d156d8..a4548dd6351e 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -71,6 +71,7 @@ extern uint32_t of_get_flat_dt_phandle(unsigned long node);
>
>  extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
>                                      int depth, void *data);
> +extern void early_init_dt_rng_seed(u64 dt_phys);
>  extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
>                                      int depth, void *data);
>  extern int early_init_dt_scan_chosen_stdout(void);
> --
> 2.20.1
>

Re: [PATCH] arm64: add support for rng-seed

[Rob Herring]

+boot-architecture list as there was some discussion about this IIRC.

On Mon, May 6, 2019 at 11:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> that can be passed to kernel called very early to increase device
> randomness. Bootloader should provide this entropy and the value is
> read from /chosen/rng-seed in DT.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
>
> ---
>  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++

Actually, this file has been converted to json-schema and lives
here[1]. I need to remove this one (or leave it with a reference to
the new one).

>  arch/arm64/kernel/setup.c                    |  2 ++
>  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
>  include/linux/of_fdt.h                       |  1 +
>  4 files changed, 50 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> index 45e79172a646..bfd360691650 100644
> --- a/Documentation/devicetree/bindings/chosen.txt
> +++ b/Documentation/devicetree/bindings/chosen.txt
> @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
>  the Linux EFI stub (which will populate the property itself, using
>  EFI_RNG_PROTOCOL).
>
> +rng-seed
> +-----------
> +
> +This property served as an entropy to add device randomness. It is parsed
> +as a 64 byte value, e.g.

Why only 64-bytes?

> +
> +/ {
> +       chosen {
> +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> +       };
> +};
> +
> +This random value should be provided by bootloader.
> +
>  stdout-path
>  -----------
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 413d566405d1..ade4261516dd 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
>         early_fixmap_init();
>         early_ioremap_init();
>
> +       early_init_dt_rng_seed(__fdt_pointer);
> +

I'm trying to reduce or eliminate all these early_init_dt_* calls.

Why is this arch specific and why can't this be done after
unflattening? It doesn't look like add_device_randomness() needs
anything early.

>         setup_machine_fdt(__fdt_pointer);
>
>         parse_early_param();
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index de893c9616a1..74e2c0c80b91 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/libfdt.h>
>  #include <linux/debugfs.h>
> +#include <linux/random.h>
>  #include <linux/serial_core.h>
>  #include <linux/sysfs.h>
>
> @@ -1117,6 +1118,38 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
>         return 1;
>  }
>
> +extern void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size,
> +                                      pgprot_t prot);
> +
> +void __init early_init_dt_rng_seed(u64 dt_phys)
> +{
> +       void *fdt;
> +       int node, size, i;
> +       fdt64_t *prop;
> +       u64 rng_seed[8];
> +
> +       fdt = __fixmap_remap_fdt(dt_phys, &size, PAGE_KERNEL);
> +       if (!fdt)
> +               return;
> +
> +       node = fdt_path_offset(fdt, "/chosen");
> +       if (node < 0)
> +               return;
> +
> +       prop = fdt_getprop_w(fdt, node, "rng-seed", &size);
> +       if (!prop || size != sizeof(u64) * 8)
> +               return;
> +
> +       for (i = 0; i < 8; i++) {
> +               rng_seed[i] = fdt64_to_cpu(*(prop + i));
> +               /* clear seed so it won't be found. */
> +               *(prop + i) = 0;
> +       }
> +       add_device_randomness(rng_seed, size);
> +
> +       return;
> +}
> +
>  #ifndef MIN_MEMBLOCK_ADDR
>  #define MIN_MEMBLOCK_ADDR      __pa(PAGE_OFFSET)
>  #endif
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index a713e5d156d8..a4548dd6351e 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -71,6 +71,7 @@ extern uint32_t of_get_flat_dt_phandle(unsigned long node);
>
>  extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
>                                      int depth, void *data);
> +extern void early_init_dt_rng_seed(u64 dt_phys);
>  extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
>                                      int depth, void *data);
>  extern int early_init_dt_scan_chosen_stdout(void);
> --
> 2.20.1
>

Re: [PATCH] arm64: add support for rng-seed

[Hsin-Yi Wang]

On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> +boot-architecture list as there was some discussion about this IIRC.
>
> On Mon, May 6, 2019 at 11:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> > that can be passed to kernel called very early to increase device
> > randomness. Bootloader should provide this entropy and the value is
> > read from /chosen/rng-seed in DT.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> >
> > ---
> >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
>
> Actually, this file has been converted to json-schema and lives
> here[1]. I need to remove this one (or leave it with a reference to
> the new one).
>
> >  arch/arm64/kernel/setup.c                    |  2 ++
> >  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
> >  include/linux/of_fdt.h                       |  1 +
> >  4 files changed, 50 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > index 45e79172a646..bfd360691650 100644
> > --- a/Documentation/devicetree/bindings/chosen.txt
> > +++ b/Documentation/devicetree/bindings/chosen.txt
> > @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
> >  the Linux EFI stub (which will populate the property itself, using
> >  EFI_RNG_PROTOCOL).
> >
> > +rng-seed
> > +-----------
> > +
> > +This property served as an entropy to add device randomness. It is parsed
> > +as a 64 byte value, e.g.
>
> Why only 64-bytes?
We can also not specify size and read what bootloader can provide.
>
> > +
> > +/ {
> > +       chosen {
> > +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> > +       };
> > +};
> > +
> > +This random value should be provided by bootloader.
> > +
> >  stdout-path
> >  -----------
> >
> > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > index 413d566405d1..ade4261516dd 100644
> > --- a/arch/arm64/kernel/setup.c
> > +++ b/arch/arm64/kernel/setup.c
> > @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
> >         early_fixmap_init();
> >         early_ioremap_init();
> >
> > +       early_init_dt_rng_seed(__fdt_pointer);
> > +
>
> I'm trying to reduce or eliminate all these early_init_dt_* calls.
>
> Why is this arch specific and why can't this be done after
> unflattening? It doesn't look like add_device_randomness() needs
> anything early.
Currently unflattening is called after setup_machine_fdt(), which
called fixmap_remap_fdt() //__fixmap_remap_fdt(dt_phys, &size,
PAGE_KERNEL_RO), and we can't modify DT after that since it's read
only. But we need to clear (eg. write 0 to it) the rng-seed after
reading from DT.
>
> >         setup_machine_fdt(__fdt_pointer);
> >
> >         parse_early_param();
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index de893c9616a1..74e2c0c80b91 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/libfdt.h>
> >  #include <linux/debugfs.h>
> > +#include <linux/random.h>
> >  #include <linux/serial_core.h>
> >  #include <linux/sysfs.h>
> >
> > @@ -1117,6 +1118,38 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
> >         return 1;
> >  }
> >
> > +extern void *__init __fixmap_remap_fdt(phys_addr_t dt_phys, int *size,
> > +                                      pgprot_t prot);
> > +
> > +void __init early_init_dt_rng_seed(u64 dt_phys)
> > +{
> > +       void *fdt;
> > +       int node, size, i;
> > +       fdt64_t *prop;
> > +       u64 rng_seed[8];
> > +
> > +       fdt = __fixmap_remap_fdt(dt_phys, &size, PAGE_KERNEL);
> > +       if (!fdt)
> > +               return;
> > +
> > +       node = fdt_path_offset(fdt, "/chosen");
> > +       if (node < 0)
> > +               return;
> > +
> > +       prop = fdt_getprop_w(fdt, node, "rng-seed", &size);
> > +       if (!prop || size != sizeof(u64) * 8)
> > +               return;
> > +
> > +       for (i = 0; i < 8; i++) {
> > +               rng_seed[i] = fdt64_to_cpu(*(prop + i));
> > +               /* clear seed so it won't be found. */
> > +               *(prop + i) = 0;
> > +       }
> > +       add_device_randomness(rng_seed, size);
> > +
> > +       return;
> > +}
> > +
> >  #ifndef MIN_MEMBLOCK_ADDR
> >  #define MIN_MEMBLOCK_ADDR      __pa(PAGE_OFFSET)
> >  #endif
> > diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> > index a713e5d156d8..a4548dd6351e 100644
> > --- a/include/linux/of_fdt.h
> > +++ b/include/linux/of_fdt.h
> > @@ -71,6 +71,7 @@ extern uint32_t of_get_flat_dt_phandle(unsigned long node);
> >
> >  extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
> >                                      int depth, void *data);
> > +extern void early_init_dt_rng_seed(u64 dt_phys);
> >  extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
> >                                      int depth, void *data);
> >  extern int early_init_dt_scan_chosen_stdout(void);
> > --
> > 2.20.1
> >

Re: [PATCH] arm64: add support for rng-seed

[Rob Herring]

On Tue, May 7, 2019 at 11:08 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > +boot-architecture list as there was some discussion about this IIRC.
> >
> > On Mon, May 6, 2019 at 11:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >
> > > Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> > > that can be passed to kernel called very early to increase device
> > > randomness. Bootloader should provide this entropy and the value is
> > > read from /chosen/rng-seed in DT.
> > >
> > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > >
> > > ---
> > >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
> >
> > Actually, this file has been converted to json-schema and lives
> > here[1]. I need to remove this one (or leave it with a reference to
> > the new one).
> >
> > >  arch/arm64/kernel/setup.c                    |  2 ++
> > >  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
> > >  include/linux/of_fdt.h                       |  1 +
> > >  4 files changed, 50 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > > index 45e79172a646..bfd360691650 100644
> > > --- a/Documentation/devicetree/bindings/chosen.txt
> > > +++ b/Documentation/devicetree/bindings/chosen.txt
> > > @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
> > >  the Linux EFI stub (which will populate the property itself, using
> > >  EFI_RNG_PROTOCOL).
> > >
> > > +rng-seed
> > > +-----------
> > > +
> > > +This property served as an entropy to add device randomness. It is parsed
> > > +as a 64 byte value, e.g.
> >
> > Why only 64-bytes?
> We can also not specify size and read what bootloader can provide.
> >
> > > +
> > > +/ {
> > > +       chosen {
> > > +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> > > +       };
> > > +};
> > > +
> > > +This random value should be provided by bootloader.
> > > +
> > >  stdout-path
> > >  -----------
> > >
> > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > > index 413d566405d1..ade4261516dd 100644
> > > --- a/arch/arm64/kernel/setup.c
> > > +++ b/arch/arm64/kernel/setup.c
> > > @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
> > >         early_fixmap_init();
> > >         early_ioremap_init();
> > >
> > > +       early_init_dt_rng_seed(__fdt_pointer);
> > > +
> >
> > I'm trying to reduce or eliminate all these early_init_dt_* calls.
> >
> > Why is this arch specific and why can't this be done after
> > unflattening? It doesn't look like add_device_randomness() needs
> > anything early.
> Currently unflattening is called after setup_machine_fdt(), which
> called fixmap_remap_fdt() //__fixmap_remap_fdt(dt_phys, &size,
> PAGE_KERNEL_RO), and we can't modify DT after that since it's read
> only. But we need to clear (eg. write 0 to it) the rng-seed after
> reading from DT.

Why do you need to clear it? That wasn't necessary for kaslr-seed.

Why not change the mapping to RW? It would be nice if this worked on
more than one arch.

Rob

Re: [PATCH] arm64: add support for rng-seed

[Hsin-Yi Wang]

On Wed, May 8, 2019 at 10:04 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, May 7, 2019 at 11:08 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > +boot-architecture list as there was some discussion about this IIRC.
> > >
> > > On Mon, May 6, 2019 at 11:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > > >
> > > > Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> > > > that can be passed to kernel called very early to increase device
> > > > randomness. Bootloader should provide this entropy and the value is
> > > > read from /chosen/rng-seed in DT.
> > > >
> > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > >
> > > > ---
> > > >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
> > >
> > > Actually, this file has been converted to json-schema and lives
> > > here[1]. I need to remove this one (or leave it with a reference to
> > > the new one).
> > >
> > > >  arch/arm64/kernel/setup.c                    |  2 ++
> > > >  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
> > > >  include/linux/of_fdt.h                       |  1 +
> > > >  4 files changed, 50 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > > > index 45e79172a646..bfd360691650 100644
> > > > --- a/Documentation/devicetree/bindings/chosen.txt
> > > > +++ b/Documentation/devicetree/bindings/chosen.txt
> > > > @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
> > > >  the Linux EFI stub (which will populate the property itself, using
> > > >  EFI_RNG_PROTOCOL).
> > > >
> > > > +rng-seed
> > > > +-----------
> > > > +
> > > > +This property served as an entropy to add device randomness. It is parsed
> > > > +as a 64 byte value, e.g.
> > >
> > > Why only 64-bytes?
> > We can also not specify size and read what bootloader can provide.
> > >
> > > > +
> > > > +/ {
> > > > +       chosen {
> > > > +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> > > > +       };
> > > > +};
> > > > +
> > > > +This random value should be provided by bootloader.
> > > > +
> > > >  stdout-path
> > > >  -----------
> > > >
> > > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > > > index 413d566405d1..ade4261516dd 100644
> > > > --- a/arch/arm64/kernel/setup.c
> > > > +++ b/arch/arm64/kernel/setup.c
> > > > @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
> > > >         early_fixmap_init();
> > > >         early_ioremap_init();
> > > >
> > > > +       early_init_dt_rng_seed(__fdt_pointer);
> > > > +
> > >
> > > I'm trying to reduce or eliminate all these early_init_dt_* calls.
> > >
> > > Why is this arch specific and why can't this be done after
> > > unflattening? It doesn't look like add_device_randomness() needs
> > > anything early.
> > Currently unflattening is called after setup_machine_fdt(), which
> > called fixmap_remap_fdt() //__fixmap_remap_fdt(dt_phys, &size,
> > PAGE_KERNEL_RO), and we can't modify DT after that since it's read
> > only. But we need to clear (eg. write 0 to it) the rng-seed after
> > reading from DT.
>
> Why do you need to clear it? That wasn't necessary for kaslr-seed.
I think it's for security purpose. If we know the random seed, it's
more likely we can predict randomness.
Currently on arm64, kaslr-seed will be wiped out (in
arch/arm64/kernel/kaslr.c#get_kaslr_seed(), it's set to 0) so we can't
read from sysfs (eg. /sys/firmware/devicetree/.../kaslr-seed)
I'm not sure on other arch if it will be wiped out.
>
> Why not change the mapping to RW? It would be nice if this worked on
> more than one arch.
>
> Rob

Re: [PATCH] arm64: add support for rng-seed

[Rob Herring]

On Wed, May 8, 2019 at 10:06 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Wed, May 8, 2019 at 10:04 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, May 7, 2019 at 11:08 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > >
> > > On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > +boot-architecture list as there was some discussion about this IIRC.
> > > >
> > > > On Mon, May 6, 2019 at 11:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > > > >
> > > > > Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> > > > > that can be passed to kernel called very early to increase device
> > > > > randomness. Bootloader should provide this entropy and the value is
> > > > > read from /chosen/rng-seed in DT.
> > > > >
> > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > > >
> > > > > ---
> > > > >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
> > > >
> > > > Actually, this file has been converted to json-schema and lives
> > > > here[1]. I need to remove this one (or leave it with a reference to
> > > > the new one).
> > > >
> > > > >  arch/arm64/kernel/setup.c                    |  2 ++
> > > > >  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
> > > > >  include/linux/of_fdt.h                       |  1 +
> > > > >  4 files changed, 50 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > > > > index 45e79172a646..bfd360691650 100644
> > > > > --- a/Documentation/devicetree/bindings/chosen.txt
> > > > > +++ b/Documentation/devicetree/bindings/chosen.txt
> > > > > @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
> > > > >  the Linux EFI stub (which will populate the property itself, using
> > > > >  EFI_RNG_PROTOCOL).
> > > > >
> > > > > +rng-seed
> > > > > +-----------
> > > > > +
> > > > > +This property served as an entropy to add device randomness. It is parsed
> > > > > +as a 64 byte value, e.g.
> > > >
> > > > Why only 64-bytes?
> > > We can also not specify size and read what bootloader can provide.
> > > >
> > > > > +
> > > > > +/ {
> > > > > +       chosen {
> > > > > +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> > > > > +       };
> > > > > +};
> > > > > +
> > > > > +This random value should be provided by bootloader.
> > > > > +
> > > > >  stdout-path
> > > > >  -----------
> > > > >
> > > > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > > > > index 413d566405d1..ade4261516dd 100644
> > > > > --- a/arch/arm64/kernel/setup.c
> > > > > +++ b/arch/arm64/kernel/setup.c
> > > > > @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
> > > > >         early_fixmap_init();
> > > > >         early_ioremap_init();
> > > > >
> > > > > +       early_init_dt_rng_seed(__fdt_pointer);
> > > > > +
> > > >
> > > > I'm trying to reduce or eliminate all these early_init_dt_* calls.
> > > >
> > > > Why is this arch specific and why can't this be done after
> > > > unflattening? It doesn't look like add_device_randomness() needs
> > > > anything early.
> > > Currently unflattening is called after setup_machine_fdt(), which
> > > called fixmap_remap_fdt() //__fixmap_remap_fdt(dt_phys, &size,
> > > PAGE_KERNEL_RO), and we can't modify DT after that since it's read
> > > only. But we need to clear (eg. write 0 to it) the rng-seed after
> > > reading from DT.
> >
> > Why do you need to clear it? That wasn't necessary for kaslr-seed.
> I think it's for security purpose. If we know the random seed, it's
> more likely we can predict randomness.
> Currently on arm64, kaslr-seed will be wiped out (in
> arch/arm64/kernel/kaslr.c#get_kaslr_seed(), it's set to 0) so we can't
> read from sysfs (eg. /sys/firmware/devicetree/.../kaslr-seed)
> I'm not sure on other arch if it will be wiped out.

The difference is if I have the kaslr seed, I can calculate the kernel
base address.

In your case, you are feeding an RNG which continually has entropy
added to it. I can't see that knowing one piece of the entropy data is
a security hole. It looks more like you've just copied what what done
for kaslr-seed.

> > Why not change the mapping to RW? It would be nice if this worked on
> > more than one arch.

Still wondering on this question. Mapping it R/W would mean rng-seed
could be handled later and completely out of the arch code and so
could the zeroing of the kaslr-seed. Also, we generally assume the FDT
is modifiable for any fixups. This happens on arm32 and powerpc, but I
guess we haven't needed that yet on arm64.

Rob

Re: [PATCH] arm64: add support for rng-seed

[Stephen Boyd]

Quoting Rob Herring (2019-05-08 09:07:11)
> On Wed, May 8, 2019 at 10:06 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > On Wed, May 8, 2019 at 10:04 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Tue, May 7, 2019 at 11:08 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > > >
> > > > On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > +boot-architecture list as there was some discussion about this IIRC.
> > > > >
> > > > > On Mon, May 6, 2019 at 11:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > > > > >
> > > > > > Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> > > > > > that can be passed to kernel called very early to increase device
> > > > > > randomness. Bootloader should provide this entropy and the value is
> > > > > > read from /chosen/rng-seed in DT.
> > > > > >
> > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > > > >
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
> > > > >
> > > > > Actually, this file has been converted to json-schema and lives
> > > > > here[1]. I need to remove this one (or leave it with a reference to
> > > > > the new one).
> > > > >
> > > > > >  arch/arm64/kernel/setup.c                    |  2 ++
> > > > > >  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
> > > > > >  include/linux/of_fdt.h                       |  1 +
> > > > > >  4 files changed, 50 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > > > > > index 45e79172a646..bfd360691650 100644
> > > > > > --- a/Documentation/devicetree/bindings/chosen.txt
> > > > > > +++ b/Documentation/devicetree/bindings/chosen.txt
> > > > > > @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
> > > > > >  the Linux EFI stub (which will populate the property itself, using
> > > > > >  EFI_RNG_PROTOCOL).
> > > > > >
> > > > > > +rng-seed
> > > > > > +-----------
> > > > > > +
> > > > > > +This property served as an entropy to add device randomness. It is parsed
> > > > > > +as a 64 byte value, e.g.
> > > > >
> > > > > Why only 64-bytes?
> > > > We can also not specify size and read what bootloader can provide.
> > > > >
> > > > > > +
> > > > > > +/ {
> > > > > > +       chosen {
> > > > > > +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> > > > > > +       };
> > > > > > +};
> > > > > > +
> > > > > > +This random value should be provided by bootloader.
> > > > > > +
> > > > > >  stdout-path
> > > > > >  -----------
> > > > > >
> > > > > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > > > > > index 413d566405d1..ade4261516dd 100644
> > > > > > --- a/arch/arm64/kernel/setup.c
> > > > > > +++ b/arch/arm64/kernel/setup.c
> > > > > > @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
> > > > > >         early_fixmap_init();
> > > > > >         early_ioremap_init();
> > > > > >
> > > > > > +       early_init_dt_rng_seed(__fdt_pointer);
> > > > > > +
> > > > >
> > > > > I'm trying to reduce or eliminate all these early_init_dt_* calls.
> > > > >
> > > > > Why is this arch specific and why can't this be done after
> > > > > unflattening? It doesn't look like add_device_randomness() needs
> > > > > anything early.
> > > > Currently unflattening is called after setup_machine_fdt(), which
> > > > called fixmap_remap_fdt() //__fixmap_remap_fdt(dt_phys, &size,
> > > > PAGE_KERNEL_RO), and we can't modify DT after that since it's read
> > > > only. But we need to clear (eg. write 0 to it) the rng-seed after
> > > > reading from DT.
> > >
> > > Why do you need to clear it? That wasn't necessary for kaslr-seed.
> > I think it's for security purpose. If we know the random seed, it's
> > more likely we can predict randomness.
> > Currently on arm64, kaslr-seed will be wiped out (in
> > arch/arm64/kernel/kaslr.c#get_kaslr_seed(), it's set to 0) so we can't
> > read from sysfs (eg. /sys/firmware/devicetree/.../kaslr-seed)
> > I'm not sure on other arch if it will be wiped out.
> 
> The difference is if I have the kaslr seed, I can calculate the kernel
> base address.
> 
> In your case, you are feeding an RNG which continually has entropy
> added to it. I can't see that knowing one piece of the entropy data is
> a security hole. It looks more like you've just copied what what done
> for kaslr-seed.
> 
> > > Why not change the mapping to RW? It would be nice if this worked on
> > > more than one arch.
> 
> Still wondering on this question. Mapping it R/W would mean rng-seed
> could be handled later and completely out of the arch code and so
> could the zeroing of the kaslr-seed. Also, we generally assume the FDT
> is modifiable for any fixups. This happens on arm32 and powerpc, but I
> guess we haven't needed that yet on arm64.
> 

Maybe we can make the mapping of the FDT be RW until unflattening and
then provide a weak arch hook to remap the FDT as RO if the architecture
supports it? This way arm64 can mark it RO after any fixes have been
made.

BTW, maybe we should put the 'initial_boot_params' pointer into
__ro_after_init? That way it can't be repointed after init, but it looks
like almost no code uses the flat DT after init anyway besides sysfs
raw read.

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4734223ab702..483e48f860ec 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -38,7 +38,7 @@
  * memory entries in the /memory node. This function may be called
  * any time after initial_boot_param is set.
  */
-void of_fdt_limit_memory(int limit)
+void __init of_fdt_limit_memory(int limit)
 {
 	int memory;
 	int len;
@@ -145,8 +145,8 @@ static bool of_fdt_device_is_available(const void *blob, unsigned long node)
 /**
  * of_fdt_match - Return true if node matches a list of compatible values
  */
-int of_fdt_match(const void *blob, unsigned long node,
-                 const char *const *compat)
+static int __init of_fdt_match(const void *blob, unsigned long node,
+			       const char *const *compat)
 {
 	unsigned int tmp, score = 0;
 
@@ -535,7 +535,7 @@ EXPORT_SYMBOL_GPL(of_fdt_unflatten_tree);
 int __initdata dt_root_addr_cells;
 int __initdata dt_root_size_cells;
 
-void *initial_boot_params;
+void *initial_boot_params __ro_after_init;
 
 #ifdef CONFIG_OF_EARLY_FLATTREE
 
@@ -758,7 +758,7 @@ int __init of_scan_flat_dt_subnodes(unsigned long parent,
  * @return offset of the subnode, or -FDT_ERR_NOTFOUND if there is none
  */
 
-int of_get_flat_dt_subnode_by_name(unsigned long node, const char *uname)
+int __init of_get_flat_dt_subnode_by_name(unsigned long node, const char *uname)
 {
 	return fdt_subnode_offset(initial_boot_params, node, uname);
 }
@@ -804,7 +804,7 @@ int __init of_flat_dt_is_compatible(unsigned long node, const char *compat)
 /**
  * of_flat_dt_match - Return true if node matches a list of compatible values
  */
-int __init of_flat_dt_match(unsigned long node, const char *const *compat)
+static int __init of_flat_dt_match(unsigned long node, const char *const *compat)
 {
 	return of_fdt_match(initial_boot_params, node, compat);
 }
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index a713e5d156d8..97b646e0ff2c 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -30,8 +30,6 @@ extern void *of_fdt_get_property(const void *blob,
 				 int *size);
 extern bool of_fdt_is_big_endian(const void *blob,
 				 unsigned long node);
-extern int of_fdt_match(const void *blob, unsigned long node,
-			const char *const *compat);
 extern void *of_fdt_unflatten_tree(const unsigned long *blob,
 				   struct device_node *dad,
 				   struct device_node **mynodes);
@@ -64,7 +62,6 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
 extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
 				       int *size);
 extern int of_flat_dt_is_compatible(unsigned long node, const char *name);
-extern int of_flat_dt_match(unsigned long node, const char *const *matches);
 extern unsigned long of_get_flat_dt_root(void);
 extern int of_get_flat_dt_size(void);
 extern uint32_t of_get_flat_dt_phandle(unsigned long node);

Re: [PATCH] arm64: add support for rng-seed

[Hsin-Yi Wang]

On Thu, May 9, 2019 at 12:07 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, May 8, 2019 at 10:06 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > On Wed, May 8, 2019 at 10:04 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Tue, May 7, 2019 at 11:08 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > > >
> > > > On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > +boot-architecture list as there was some discussion about this IIRC.
> > > > >
> > > > > On Mon, May 6, 2019 at 11:54 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> > > > > >
> > > > > > Introducing a chosen node, rng-seed, which is an 64 bytes entropy
> > > > > > that can be passed to kernel called very early to increase device
> > > > > > randomness. Bootloader should provide this entropy and the value is
> > > > > > read from /chosen/rng-seed in DT.
> > > > > >
> > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > > > > >
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
> > > > >
> > > > > Actually, this file has been converted to json-schema and lives
> > > > > here[1]. I need to remove this one (or leave it with a reference to
> > > > > the new one).
> > > > >
> > > > > >  arch/arm64/kernel/setup.c                    |  2 ++
> > > > > >  drivers/of/fdt.c                             | 33 ++++++++++++++++++++
> > > > > >  include/linux/of_fdt.h                       |  1 +
> > > > > >  4 files changed, 50 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/chosen.txt b/Documentation/devicetree/bindings/chosen.txt
> > > > > > index 45e79172a646..bfd360691650 100644
> > > > > > --- a/Documentation/devicetree/bindings/chosen.txt
> > > > > > +++ b/Documentation/devicetree/bindings/chosen.txt
> > > > > > @@ -28,6 +28,20 @@ mode) when EFI_RNG_PROTOCOL is supported, it will be overwritten by
> > > > > >  the Linux EFI stub (which will populate the property itself, using
> > > > > >  EFI_RNG_PROTOCOL).
> > > > > >
> > > > > > +rng-seed
> > > > > > +-----------
> > > > > > +
> > > > > > +This property served as an entropy to add device randomness. It is parsed
> > > > > > +as a 64 byte value, e.g.
> > > > >
> > > > > Why only 64-bytes?
> > > > We can also not specify size and read what bootloader can provide.
> > > > >
> > > > > > +
> > > > > > +/ {
> > > > > > +       chosen {
> > > > > > +               rng-seed = <0x31951b3c 0xc9fab3a5 0xffdf1660 ...>
> > > > > > +       };
> > > > > > +};
> > > > > > +
> > > > > > +This random value should be provided by bootloader.
> > > > > > +
> > > > > >  stdout-path
> > > > > >  -----------
> > > > > >
> > > > > > diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> > > > > > index 413d566405d1..ade4261516dd 100644
> > > > > > --- a/arch/arm64/kernel/setup.c
> > > > > > +++ b/arch/arm64/kernel/setup.c
> > > > > > @@ -292,6 +292,8 @@ void __init setup_arch(char **cmdline_p)
> > > > > >         early_fixmap_init();
> > > > > >         early_ioremap_init();
> > > > > >
> > > > > > +       early_init_dt_rng_seed(__fdt_pointer);
> > > > > > +
> > > > >
> > > > > I'm trying to reduce or eliminate all these early_init_dt_* calls.
> > > > >
> > > > > Why is this arch specific and why can't this be done after
> > > > > unflattening? It doesn't look like add_device_randomness() needs
> > > > > anything early.
> > > > Currently unflattening is called after setup_machine_fdt(), which
> > > > called fixmap_remap_fdt() //__fixmap_remap_fdt(dt_phys, &size,
> > > > PAGE_KERNEL_RO), and we can't modify DT after that since it's read
> > > > only. But we need to clear (eg. write 0 to it) the rng-seed after
> > > > reading from DT.
> > >
> > > Why do you need to clear it? That wasn't necessary for kaslr-seed.
> > I think it's for security purpose. If we know the random seed, it's
> > more likely we can predict randomness.
> > Currently on arm64, kaslr-seed will be wiped out (in
> > arch/arm64/kernel/kaslr.c#get_kaslr_seed(), it's set to 0) so we can't
> > read from sysfs (eg. /sys/firmware/devicetree/.../kaslr-seed)
> > I'm not sure on other arch if it will be wiped out.
>
> The difference is if I have the kaslr seed, I can calculate the kernel
> base address.
>
> In your case, you are feeding an RNG which continually has entropy
> added to it. I can't see that knowing one piece of the entropy data is
> a security hole. It looks more like you've just copied what what done
> for kaslr-seed.
+Kees who can probably explain this better.

This early added entropy is also going to be used for stack canary. At
the time it's created there's not be much entropy (before
boot_init_stack_canary(), there's only add_latent_entropy() and
command_line).
On arm64, there is a single canary for all tasks. If RNG is weak or
the seed can be read, it might be easier to figure out the canary.

>
> > > Why not change the mapping to RW? It would be nice if this worked on
> > > more than one arch.
>
> Still wondering on this question. Mapping it R/W would mean rng-seed
> could be handled later and completely out of the arch code and so
> could the zeroing of the kaslr-seed. Also, we generally assume the FDT
> is modifiable for any fixups. This happens on arm32 and powerpc, but I
> guess we haven't needed that yet on arm64.
We can try to map it to RW and map back to RO later if needed on
arm64, like Stephen's suggestion.
>
> Rob

Thanks for the comments and suggestions.

Re: [PATCH] arm64: add support for rng-seed

[Kees Cook]

On Thu, May 9, 2019 at 1:00 AM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> This early added entropy is also going to be used for stack canary. At
> the time it's created there's not be much entropy (before
> boot_init_stack_canary(), there's only add_latent_entropy() and
> command_line).
> On arm64, there is a single canary for all tasks. If RNG is weak or
> the seed can be read, it might be easier to figure out the canary.

With newer compilers[1] there will be a per-task canary on arm64[2],
which will improve this situation, but many architectures lack a
per-task canary, unfortunately. I've also recently rearranged the RNG
initialization[3] which should also help with better entropy mixing.
But each of these are kind of band-aids against not having sufficient
initial entropy, which leaves the canary potentially exposed.

-Kees

[1] https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=359c1bf35e3109d2f3882980b47a5eae46123259
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0a1213fa7432778b71a1c0166bf56660a3aab030
[3] https://git.kernel.org/pub/scm/linux/kernel/git/tytso/random.git/commit/?h=dev&id=d55535232c3dbde9a523a9d10d68670f5fe5dec3

-- 
Kees Cook

Re: [PATCH] arm64: add support for rng-seed

[Rasmus Villemoes]

On 09/05/2019 10.00, Hsin-Yi Wang wrote:
>>>> Why do you need to clear it? That wasn't necessary for kaslr-seed.
>>> I think it's for security purpose. If we know the random seed, it's
>>> more likely we can predict randomness.
>>> Currently on arm64, kaslr-seed will be wiped out (in
>>> arch/arm64/kernel/kaslr.c#get_kaslr_seed(), it's set to 0) so we can't
>>> read from sysfs (eg. /sys/firmware/devicetree/.../kaslr-seed)
>>> I'm not sure on other arch if it will be wiped out.
>>
>> The difference is if I have the kaslr seed, I can calculate the kernel
>> base address.
>>
>> In your case, you are feeding an RNG which continually has entropy
>> added to it. I can't see that knowing one piece of the entropy data is
>> a security hole. It looks more like you've just copied what what done
>> for kaslr-seed.
> +Kees who can probably explain this better.
> 
> This early added entropy is also going to be used for stack canary. At
> the time it's created there's not be much entropy (before
> boot_init_stack_canary(), there's only add_latent_entropy() and
> command_line).

So, why not just have the bootloader add whatever entropy it has via the
commandline, which already gets mixed in? That requires no kernel
changes, and works for all architectures.

If anything, perhaps instead of just adding gobbledygook=abc123, make an
official command line parameter (there was talk about this at some
point), and have the kernel overwrite the value with xxx so it's not
visible in /proc/cmdline.

Rasmus

Re: [PATCH] arm64: add support for rng-seed

[Hsin-Yi Wang]

On Fri, May 10, 2019 at 2:14 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:

> So, why not just have the bootloader add whatever entropy it has via the
> commandline, which already gets mixed in? That requires no kernel
> changes, and works for all architectures.
>
> If anything, perhaps instead of just adding gobbledygook=abc123, make an
> official command line parameter (there was talk about this at some
> point), and have the kernel overwrite the value with xxx so it's not
> visible in /proc/cmdline.
>
> Rasmus

For some arch, besides commandline, we also need to overwrite bootargs
in fdt, otherwise it's still visible by
/sys/firmware/devicetree/base/chosen/bootargs for example.

Originally planned to land v2 as

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index de893c9616a1..96ea5eba9dd5 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>
@@ -1079,6 +1080,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);

@@ -1113,6 +1115,15 @@ 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)
+               return 1;
+
+       /* try to clear seed so it won't be found. */
+        fdt_nop_property(initial_boot_params, node, "rng-seed");
+
+        add_device_randomness(rng_seed, l);
+
        /* break now */
        return 1;
 }

(For arm64 RW/RO issue, it will be done in other patch.)

If we add parameter into commandline, I think we probably also need to
do similar changes here since there are fdt related overwrite.

Re: [PATCH] arm64: add support for rng-seed

[Stephen Boyd]

Quoting Rasmus Villemoes (2019-05-09 23:14:00)
> 
> So, why not just have the bootloader add whatever entropy it has via the
> commandline, which already gets mixed in? That requires no kernel
> changes, and works for all architectures.
> 
> If anything, perhaps instead of just adding gobbledygook=abc123, make an
> official command line parameter (there was talk about this at some
> point), and have the kernel overwrite the value with xxx so it's not
> visible in /proc/cmdline.
> 

Why is using the commandline desired? Just for ease of implementation
and cross-architecture support because we already mix in the
commandline? 

The kernel commandline is limited in size so we would waste around
64-bytes of the buffer to get a random chunk of data from the bootloader
into the kernel instead of allowing more parameters. Or if we wanted a
large chunk of random bytes then we would start running into the length
limit. Given that EFI based systems already have a way to inject more
randomness into the kernel's RNG very early by means of an RNG seed EFI
protocol it looks irrelevant to want to be cross-architecture in this
way because EFI platforms wouldn't use it. 

If DT based systems can all get support for this in the generic DT code
then we're able to make things work on both EFI and DT platforms with a
little extra __init code while keeping things away from the commandline.
That sounds like a win to me because the commandline is limited in size
and meant to pass things like parameters and flags to the kernel, not
raw data like seeds and binary gook.

Re: [PATCH] arm64: add support for rng-seed

[Hsin-Yi Wang]

On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:

> >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
>
> Actually, this file has been converted to json-schema and lives
> here[1]. I need to remove this one (or leave it with a reference to
> the new one).
>

Hi Rob,
I can't find where the new document is. Can you help point it again? Thanks.

Re: [PATCH] arm64: add support for rng-seed

[Rob Herring]

On Thu, May 9, 2019 at 11:27 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Wed, May 8, 2019 at 3:47 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> > >  Documentation/devicetree/bindings/chosen.txt | 14 +++++++++
> >
> > Actually, this file has been converted to json-schema and lives
> > here[1]. I need to remove this one (or leave it with a reference to
> > the new one).
> >
>
> Hi Rob,
> I can't find where the new document is. Can you help point it again? Thanks.

Sorry, forgot to add that:

https://github.com/devicetree-org/dt-schema/blob/master/schemas/chosen.yaml

Rob