| Message ID | 20190612043258.166048-4-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for rng-seed | expand |
On Wed, Jun 12, 2019 at 12:33:02PM +0800, Hsin-Yi Wang wrote: > Adding "rng-seed" to dtb. It's fine to add this property if original > fdt doesn't contain it. Since original seed will be wiped after > read, so use a default size 128 bytes here. Why is 128 bytes the default value? I didn't see an update to Documentation/devicetree/bindings/chosen.txt, so it's not clear to me precisely what we expect. > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > --- > change log v5->v6: > * no change > --- > arch/arm64/kernel/machine_kexec_file.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > index 58871333737a..d40fde72a023 100644 > --- a/arch/arm64/kernel/machine_kexec_file.c > +++ b/arch/arm64/kernel/machine_kexec_file.c > @@ -27,6 +27,8 @@ > #define FDT_PROP_INITRD_END "linux,initrd-end" > #define FDT_PROP_BOOTARGS "bootargs" > #define FDT_PROP_KASLR_SEED "kaslr-seed" > +#define FDT_PROP_RNG_SEED "rng-seed" > +#define RNG_SEED_SIZE 128 > > const struct kexec_file_ops * const kexec_file_loaders[] = { > &kexec_image_ops, > @@ -102,6 +104,23 @@ static int setup_dtb(struct kimage *image, > FDT_PROP_KASLR_SEED); > } > > + /* add rng-seed */ > + if (rng_is_initialized()) { > + void *rng_seed = kmalloc(RNG_SEED_SIZE, GFP_ATOMIC); For 128 bytes, it would be better to use a buffer on the stack. That avoids the possibility of the allocation failing. > + get_random_bytes(rng_seed, RNG_SEED_SIZE); > + > + ret = fdt_setprop(dtb, off, FDT_PROP_RNG_SEED, rng_seed, > + RNG_SEED_SIZE); > + kfree(rng_seed); > + > + if (ret) > + goto out; If the RNG wasn't initialised, we'd carry on with a warning. Why do we follow a different policy here? Thanks, Mark. > + > + } else { > + pr_notice("RNG is not initialised: omitting \"%s\" property\n", > + FDT_PROP_RNG_SEED); > + } > + > out: > if (ret) > return (ret == -FDT_ERR_NOSPACE) ? -ENOMEM : -EINVAL; > @@ -110,7 +129,8 @@ static int setup_dtb(struct kimage *image, > } > > /* > - * More space needed so that we can add initrd, bootargs and kaslr-seed. > + * More space needed so that we can add initrd, bootargs, kaslr-seed, and > + * rng-seed. > */ > #define DTB_EXTRA_SPACE 0x1000 > > -- > 2.20.1 >
On Fri, Jun 28, 2019 at 5:42 PM Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Jun 12, 2019 at 12:33:02PM +0800, Hsin-Yi Wang wrote: > > Adding "rng-seed" to dtb. It's fine to add this property if original > > fdt doesn't contain it. Since original seed will be wiped after > > read, so use a default size 128 bytes here. > > Why is 128 bytes the default value? More than 64 bytes should be enough. > > I didn't see an update to Documentation/devicetree/bindings/chosen.txt, > so it's not clear to me precisely what we expect. > Rob suggested to update in a newer dt-schema documentation at https://github.com/devicetree-org/dt-schema. A pull request has been sent but perhaps it would continue if kernel patches are accepted. > > For 128 bytes, it would be better to use a buffer on the stack. That > avoids the possibility of the allocation failing. > Okay, I'll update this. > > If the RNG wasn't initialised, we'd carry on with a warning. Why do we > follow a different policy here? > For failure case, I think kernel can still be boot since this is not a very fatal case, just same as the seed wasn't provided by bootloader at first boot. So I'll also let fdt_setprop() failed case carry on with warning. Thanks
On Fri, Jun 28, 2019 at 7:47 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote: > > > > If the RNG wasn't initialised, we'd carry on with a warning. Why do we > > follow a different policy here? > > (Sorry, please ignore previous comment) I think this part should be same as kaslr, since they are both adding random seeds: If RNG isn't initialized, we won't be able to set these seeds, and dtb can't do anything else to deal with this, so carry on with warning. If fdt_setprop failed with no space, create_dtb() will try to setup dtb again with more space. Other failures are setting fdt's error, so returns invalid.
diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c index 58871333737a..d40fde72a023 100644 --- a/arch/arm64/kernel/machine_kexec_file.c +++ b/arch/arm64/kernel/machine_kexec_file.c @@ -27,6 +27,8 @@ #define FDT_PROP_INITRD_END "linux,initrd-end" #define FDT_PROP_BOOTARGS "bootargs" #define FDT_PROP_KASLR_SEED "kaslr-seed" +#define FDT_PROP_RNG_SEED "rng-seed" +#define RNG_SEED_SIZE 128 const struct kexec_file_ops * const kexec_file_loaders[] = { &kexec_image_ops, @@ -102,6 +104,23 @@ static int setup_dtb(struct kimage *image, FDT_PROP_KASLR_SEED); } + /* add rng-seed */ + if (rng_is_initialized()) { + void *rng_seed = kmalloc(RNG_SEED_SIZE, GFP_ATOMIC); + get_random_bytes(rng_seed, RNG_SEED_SIZE); + + ret = fdt_setprop(dtb, off, FDT_PROP_RNG_SEED, rng_seed, + RNG_SEED_SIZE); + kfree(rng_seed); + + if (ret) + goto out; + + } else { + pr_notice("RNG is not initialised: omitting \"%s\" property\n", + FDT_PROP_RNG_SEED); + } + out: if (ret) return (ret == -FDT_ERR_NOSPACE) ? -ENOMEM : -EINVAL; @@ -110,7 +129,8 @@ static int setup_dtb(struct kimage *image, } /* - * More space needed so that we can add initrd, bootargs and kaslr-seed. + * More space needed so that we can add initrd, bootargs, kaslr-seed, and + * rng-seed. */ #define DTB_EXTRA_SPACE 0x1000