On Thu, Jun 20, 2024 at 6:58 AM Caleb Connolly
<caleb.conno...@linaro.org> wrote:
>
> Hi Tim,
>
> On 18/06/2024 23:06, Tim Harvey wrote:
> > If RANDOMIZE_BASE is enabled in the Linux kernel instructing it to
> > randomize the virtual address at which the kernel image is loaded, it
> > expects entropy to be provided by the bootloader by populating
> > /chosen/kaslr-seed with a 64-bit value from source of entropy at boot.
> >
> > Add a fdt_kaslrseed function to accommodate this allowing an existing
> > node to be overwritten if present. For now use the first rng device
> > but it would be good to enhance this in the future to allow some sort
> > of selection or policy in choosing the rng device used.
> >
> > Signed-off-by: Tim Harvey <thar...@gateworks.com>
> > Reviewed-by: Simon Glass <s...@chromium.org>
> > Cc: Michal Simek <michal.si...@amd.com>
> > Cc: Andy Yan <andy....@rock-chips.com>
> > Cc: Akash Gajjar <gajjar04ak...@gmail.com>
> > Cc: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > Cc: Simon Glass <s...@chromium.org>
> > Cc: Patrick Delaunay <patrick.delau...@foss.st.com>
> > Cc: Patrice Chotard <patrice.chot...@foss.st.com>
> > Cc: Devarsh Thakkar <devar...@ti.com>
> > Cc: Heinrich Schuchardt <xypron.g...@gmx.de>
> > Cc: Hugo Villeneuve <hvillene...@dimonoff.com>
> > Cc: Marek Vasut <ma...@denx.de>
> > Cc: Tom Rini <tr...@konsulko.com>
> > Cc: Chris Morgan <macromor...@hotmail.com>
> > ---
> > v6:
> > - collected tags
> > v5:
> > - move function to boot/fdt_support.c
> > - remove ability to select rng index and note in the commit log
> > something like this as a future enhancement.
> > - fixed typo in commit message s/it's/its/
> > - use cmd_process_error per Michal's suggestion
> > v4:
> > - add missing /n to notice in kaslrseed cmd
> > - combine ints in declaration
> > - remove unused vars from board/xilinx/common/board.c ft_board_setup
> > v3:
> > - skip if CONFIG_MEASURED_BOOT
> > - fix skip for CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT
> > - pass in rng index and bool to specify overwrite
> > - remove duplicate error strings printed outside of fdt_kaslrseed
> > - added note to commit log about how EFI STUB weeds out kalsr-seed
> > v2:
> > - fix typo in commit msg
> > - use stack for seed to avoid unecessary malloc/free
> > - move to a library function and deduplicate code by using it
> > elsewhere
> > ---
> > boot/fdt_support.c | 44 +++++++++++++++++++++++++++++++++++++++++++
> > include/fdt_support.h | 10 ++++++++++
> > 2 files changed, 54 insertions(+)
> >
> > diff --git a/boot/fdt_support.c b/boot/fdt_support.c
> > index 2bd80a9dfb18..b1b2679dea0c 100644
> > --- a/boot/fdt_support.c
> > +++ b/boot/fdt_support.c
> > @@ -7,12 +7,15 @@
> > */
> >
> > #include <common.h>
> > +#include <dm.h>
> > #include <abuf.h>
> > #include <env.h>
> > #include <log.h>
> > #include <mapmem.h>
> > #include <net.h>
> > +#include <rng.h>
> > #include <stdio_dev.h>
> > +#include <dm/device_compat.h>
> > #include <dm/ofnode.h>
> > #include <linux/ctype.h>
> > #include <linux/types.h>
> > @@ -274,6 +277,47 @@ int fdt_initrd(void *fdt, ulong initrd_start, ulong
> > initrd_end)
> > return 0;
> > }
> >
> > +int fdt_kaslrseed(void *fdt, bool overwrite)
> > +{
> > + int len, err, nodeoffset;
> > + struct udevice *dev;
> > + const u64 *orig;
> > + u64 data = 0;
> > +
> > + err = fdt_check_header(fdt);
> > + if (err < 0)
> > + return err;
>
> All the paths that call fdt_kaslrseed() call fdt_check_header()
> beforehand, I think it's safe to drop this.
Hi Caleb,
The do_kaslr_seed path does not check for this and instead passes
working_fdt - perhaps your point is still valid if working_fdt is
guaranteed to always be a valid fdt?
> > +
> > + /* find or create "/chosen" node. */
> > + nodeoffset = fdt_find_or_add_subnode(fdt, 0, "chosen");
> > + if (nodeoffset < 0)
> > + return nodeoffset;
>
> Arguably, this shouldn't be the responsibility of this function, maybe
> it would be better to error our here?
I'm not sure what your point is here... it is erroring out.
Best Regards,
Tim
> > +
> > + /* return without error if we are not overwriting and existing
> > non-zero node */
> > + orig = fdt_getprop(fdt, nodeoffset, "kaslr-seed", &len);
> > + if (orig && len == sizeof(*orig))
> > + data = fdt64_to_cpu(*orig);
> > + if (data && !overwrite) {
> > + debug("not overwriting existing kaslr-seed\n");
> > + return 0;
> > + }
> > + err = uclass_get_device(UCLASS_RNG, 0, &dev);
> > + if (err) {
> > + printf("No RNG device\n");
> > + return err;
> > + }
> > + err = dm_rng_read(dev, &data, sizeof(data));
> > + if (err) {
> > + dev_err(dev, "dm_rng_read failed: %d\n", err);
> > + return err;
> > + }
> > + err = fdt_setprop(fdt, nodeoffset, "kaslr-seed", &data, sizeof(data));
> > + if (err < 0)
> > + printf("WARNING: could not set kaslr-seed %s.\n",
> > fdt_strerror(err));
> > +
> > + return err;
> > +}
> > +
> > /**
> > * board_fdt_chosen_bootargs - boards may override this function to use
> > * alternative kernel command line arguments
> > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > index 4b71b8948d99..741e2360c224 100644
> > --- a/include/fdt_support.h
> > +++ b/include/fdt_support.h
> > @@ -463,4 +463,14 @@ void fdt_fixup_board_enet(void *blob);
> > #ifdef CONFIG_CMD_PSTORE
> > void fdt_fixup_pstore(void *blob);
> > #endif
> > +
> > +/**
> > + * fdt_kaslrseed() - create a 'kaslr-seed' node in chosen
> > + *
> > + * @blob: fdt blob
> > + * @overwrite: do not overwrite existing non-zero node unless true
> > + * Return: 0 if OK, -ve on error
> > + */
> > +int fdt_kaslrseed(void *blob, bool overwrite);
> > +
> > #endif /* ifndef __FDT_SUPPORT_H */
>
> --
> // Caleb (they/them)
