[PATCH v7 1/4] Add fdt_kaslrseed function to add kaslr-seed to chosen node

[Tim Harvey]

On Thu, Jun 20, 2024 at 6:58 AM Caleb Connolly
<caleb.connolly at 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 <tharvey at gateworks.com>
> > Reviewed-by: Simon Glass <sjg at chromium.org>
> > Cc: Michal Simek <michal.simek at amd.com>
> > Cc: Andy Yan <andy.yan at rock-chips.com>
> > Cc: Akash Gajjar <gajjar04akash at gmail.com>
> > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > Cc: Patrice Chotard <patrice.chotard at foss.st.com>
> > Cc: Devarsh Thakkar <devarsht at ti.com>
> > Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> > Cc: Hugo Villeneuve <hvilleneuve at dimonoff.com>
> > Cc: Marek Vasut <marex at denx.de>
> > Cc: Tom Rini <trini at konsulko.com>
> > Cc: Chris Morgan <macromorgan at 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)