Re: [PATCH v4 1/5] fdt: common API to populate kaslr seed

[Simon Glass <sjg@chromium.org>]

Hi Sean,

On Fri, 3 Nov 2023 at 12:39, <seanedmond@linux.microsoft.com> wrote:
>
> From: Dhananjay Phadke <dphadke@linux.microsoft.com>
>
> fdt_fixup_kaslr_seed() will update given ofnode with random seed value.
> Source for random seed can be TPM or RNG driver in u-boot or sec
> firmware (ARM).
>
> Signed-off-by: Dhananjay Phadke <dphadke@linux.microsoft.com>
> Signed-off-by: Sean Edmond <senaedmond@microsoft.com>
> ---
>  arch/arm/cpu/armv8/sec_firmware.c | 39 +++++++++++--------------------
>  boot/fdt_support.c                | 19 +++++++++++++++
>  drivers/core/ofnode.c             | 17 ++++++++++++++
>  include/dm/ofnode.h               | 12 ++++++++++
>  include/fdt_support.h             |  9 +++++++
>  5 files changed, 71 insertions(+), 25 deletions(-)
>
> diff --git a/arch/arm/cpu/armv8/sec_firmware.c b/arch/arm/cpu/armv8/sec_firmware.c
> index c0e8726346f..5f04cd8aecd 100644
> --- a/arch/arm/cpu/armv8/sec_firmware.c
> +++ b/arch/arm/cpu/armv8/sec_firmware.c
> @@ -411,46 +411,35 @@ int sec_firmware_init(const void *sec_firmware_img,
>  /*
>   * fdt_fix_kaslr - Add kalsr-seed node in Device tree
>   * @fdt:               Device tree
> - * @eret:              0 in case of error, 1 for success
> + * @eret:              0 for success
>   */
>  int fdt_fixup_kaslr(void *fdt)
>  {
> -       int nodeoffset;
> -       int err, ret = 0;
> -       u8 rand[8];
> +       int ret = 0;
>
>  #if defined(CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT)
> +       u8 rand[8];
> +       ofnode root;
> +
>         /* Check if random seed generation is  supported */
>         if (sec_firmware_support_hwrng() == false) {
>                 printf("WARNING: SEC firmware not running, no kaslr-seed\n");
> -               return 0;
> +               return -EOPNOTSUPP;
>         }
>
> -       err = sec_firmware_get_random(rand, 8);
> -       if (err < 0) {
> +       ret = sec_firmware_get_random(rand, 8);
> +       if (ret < 0) {
>                 printf("WARNING: No random number to set kaslr-seed\n");
> -               return 0;
> +               return ret;
>         }
>
> -       err = fdt_check_header(fdt);
> -       if (err < 0) {
> -               printf("fdt_chosen: %s\n", fdt_strerror(err));
> -               return 0;
> +       ret = root_ofnode_from_fdt(fdt, &root);
> +       if (ret < 0) {
> +               printf("WARNING: Unable to get root ofnode\n");
> +               return ret;
>         }
>
> -       /* find or create "/chosen" node. */
> -       nodeoffset = fdt_find_or_add_subnode(fdt, 0, "chosen");
> -       if (nodeoffset < 0)
> -               return 0;
> -
> -       err = fdt_setprop(fdt, nodeoffset, "kaslr-seed", rand,
> -                                 sizeof(rand));
> -       if (err < 0) {
> -               printf("WARNING: can't set kaslr-seed %s.\n",
> -                      fdt_strerror(err));
> -               return 0;
> -       }
> -       ret = 1;
> +       ret = fdt_fixup_kaslr_seed(root, rand, sizeof(rand));
>  #endif
>
>         return ret;
> diff --git a/boot/fdt_support.c b/boot/fdt_support.c
> index 5e49078f8c3..52be4375b46 100644
> --- a/boot/fdt_support.c
> +++ b/boot/fdt_support.c
> @@ -631,6 +631,25 @@ void fdt_fixup_ethernet(void *fdt)
>         }
>  }
>
> +int fdt_fixup_kaslr_seed(ofnode node, const u8 *seed, int len)
> +{
> +       ofnode chosen;
> +       int ret;
> +
> +       /* find or create "/chosen" node. */
> +       ret = ofnode_add_subnode(node, "chosen", &chosen);
> +       if (ret && ret != -EEXIST)
> +               return -ENOENT;
> +
> +       ret = ofnode_write_prop(chosen, "kaslr-seed", seed, len, true);
> +       if (ret) {
> +               printf("WARNING: can't set kaslr-seed\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  int fdt_record_loadable(void *blob, u32 index, const char *name,
>                         uintptr_t load_addr, u32 size, uintptr_t entry_point,
>                         const char *type, const char *os, const char *arch)
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index 29a42945102..55291f0202b 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -966,6 +966,23 @@ ofnode oftree_path(oftree tree, const char *path)
>         }
>  }
>
> +int root_ofnode_from_fdt(void *fdt, ofnode *root_node)
> +{
> +       oftree tree;
> +       /* If OFNODE_MULTI_TREE is not set, and if fdt is not the control FDT,
> +        *  oftree_from_fdt() will return NULL
> +        */
> +       tree = oftree_from_fdt(fdt);
> +
> +       if (!oftree_valid(tree)) {
> +               printf("Cannot create oftree\n");
> +               return -EINVAL;
> +       }
> +       *root_node = oftree_root(tree);
> +
> +       return 0;
> +}
> +
>  const void *ofnode_read_chosen_prop(const char *propname, int *sizep)
>  {
>         ofnode chosen_node;
> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> index 19e97a90327..5759cac5b30 100644
> --- a/include/dm/ofnode.h
> +++ b/include/dm/ofnode.h
> @@ -936,6 +936,18 @@ ofnode oftree_path(oftree tree, const char *path);
>   */
>  ofnode oftree_root(oftree tree);
>
> +/**
> + * root_ofnode_from_fdt() - Gets the root ofnode given an FDT blob.
> + *                          Note, this will fail if OFNODE_MULTI_TREE
> + *                          is not set.
> + *
> + * @fdt: Device tree to use
> + * @root_node : Root ofnode
> + *
> + * Return: 0 if OK, -ve on error
> + */
> +int root_ofnode_from_fdt(void *fdt, ofnode *root_node);
> +
>  /**
>   * ofnode_read_chosen_prop() - get the value of a chosen property
>   *
> diff --git a/include/fdt_support.h b/include/fdt_support.h
> index 2cd83668982..d967118bedf 100644
> --- a/include/fdt_support.h
> +++ b/include/fdt_support.h
> @@ -11,6 +11,7 @@
>         !defined(USE_HOSTCC)
>
>  #include <asm/u-boot.h>
> +#include <dm/ofnode.h>

I believe you only need dm/ofnode-decl.h here. That avoids pulling in
ofnode.h which ends up with of.h which includes asm/global_data.h
which causes a warning for qemu_arm64 for me.

Perhaps that last include could be dropped? But in any case, we should
use ofnode-decl.h in preference to ofnode.h if possible

>  #include <linux/libfdt.h>
>  #include <abuf.h>
>
> @@ -121,6 +122,14 @@ static inline int fdt_fixup_memory_banks(void *blob, u64 start[], u64 size[],
>  #endif
>
>  void fdt_fixup_ethernet(void *fdt);
> +
> +/*
> + * fdt_fixup_kaslr_seed - Add kaslr-seed node in Device tree
> + * @node:              ofnode
> + * @eret:              0 for success
> + */
> +int fdt_fixup_kaslr_seed(ofnode node, const u8 *seed, int len);
> +
>  int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
>                          const void *val, int len, int create);
>  void fdt_fixup_qe_firmware(void *fdt);
> --
> 2.42.0
>

Regards,
Simon