On Mon, May 20, 2024 at 1:29 AM Michal Simek <michal.si...@amd.com> wrote: > > Hi Tim, > > On 5/16/24 17:58, Tim Harvey wrote: > > On Wed, May 15, 2024 at 1:50 PM Tim Harvey <thar...@gateworks.com> 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. > >> > >> If we have DM_RNG enabled populate this value automatically when > >> fdt_chosen is called unless ARMV8_SEC_FIRMWARE_SUPPORT is enabled as > >> it's implementation uses a different source of entropy. > >> > >> As this fdt node is added elsewhere create a library function and > >> use it to deduplicate code. > >> > >> Note that the kalsrseed command (CMD_KASLRSEED) is likely pointless now > >> but left in place in case boot scripts exist that rely on this command > >> existing and returning success. An informational message is printed to > >> alert users of this command that it is likely no longer needed. > >> > >> Signed-off-by: Tim Harvey <thar...@gateworks.com> > >> --- > >> 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 > >> --- > >> board/xilinx/common/board.c | 35 ----------------------------- > >> boot/fdt_support.c | 10 +++++++++ > >> boot/pxe_utils.c | 35 +++-------------------------- > >> cmd/kaslrseed.c | 45 ++++++------------------------------- > >> include/kaslrseed.h | 17 ++++++++++++++ > >> lib/Makefile | 1 + > >> lib/kaslrseed.c | 34 ++++++++++++++++++++++++++++ > >> 7 files changed, 72 insertions(+), 105 deletions(-) > >> create mode 100644 include/kaslrseed.h > >> create mode 100644 lib/kaslrseed.c > >> > >> diff --git a/board/xilinx/common/board.c b/board/xilinx/common/board.c > >> index 30a81376ac41..f741e8957818 100644 > >> --- a/board/xilinx/common/board.c > >> +++ b/board/xilinx/common/board.c > >> @@ -713,41 +713,6 @@ int ft_board_setup(void *blob, struct bd_info *bd) > >> if (IS_ENABLED(CONFIG_FDT_FIXUP_PARTITIONS) && > >> IS_ENABLED(CONFIG_NAND_ZYNQ)) > >> fdt_fixup_mtdparts(blob, nodes, ARRAY_SIZE(nodes)); > >> > >> - if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) { > >> - debug("No RNG device\n"); > >> - return 0; > >> - } > >> - > >> - if (dm_rng_read(dev, buf, n)) { > >> - debug("Reading RNG failed\n"); > >> - return 0; > >> - } > >> - > >> - if (!blob) { > >> - debug("No FDT memory address configured. Please > >> configure\n" > >> - "the FDT address via \"fdt addr <address>\" > >> command.\n" > >> - "Aborting!\n"); > >> - return 0; > >> - } > >> - > >> - ret = fdt_check_header(blob); > >> - if (ret < 0) { > >> - debug("fdt_chosen: %s\n", fdt_strerror(ret)); > >> - return ret; > >> - } > >> - > >> - nodeoffset = fdt_find_or_add_subnode(blob, 0, "chosen"); > >> - if (nodeoffset < 0) { > >> - debug("Reading chosen node failed\n"); > >> - return nodeoffset; > >> - } > >> - > >> - ret = fdt_setprop(blob, nodeoffset, "kaslr-seed", buf, > >> sizeof(buf)); > >> - if (ret < 0) { > >> - debug("Unable to set kaslr-seed on chosen node: %s\n", > >> fdt_strerror(ret)); > >> - return ret; > >> - } > >> - > >> return 0; > >> } > >> #endif > >> diff --git a/boot/fdt_support.c b/boot/fdt_support.c > >> index 874ca4d6f5af..3455d60d69dc 100644 > >> --- a/boot/fdt_support.c > >> +++ b/boot/fdt_support.c > >> @@ -8,6 +8,7 @@ > >> > >> #include <abuf.h> > >> #include <env.h> > >> +#include <kaslrseed.h> > >> #include <log.h> > >> #include <mapmem.h> > >> #include <net.h> > >> @@ -300,6 +301,15 @@ int fdt_chosen(void *fdt) > >> if (nodeoffset < 0) > >> return nodeoffset; > >> > >> + if (IS_ENABLED(CONFIG_DM_RNG) && > >> !IS_ENABLED(ARMV8_SEC_FIRMWARE_SUPPORT)) { > >> + err = fdt_kaslrseed(fdt); > >> + if (err) { > >> + printf("WARNING: could not set kaslr-seed %s.\n", > >> + fdt_strerror(err)); > >> + return err; > >> + } > >> + } > >> + > >> if (IS_ENABLED(CONFIG_BOARD_RNG_SEED) && !board_rng_seed(&buf)) { > >> err = fdt_setprop(fdt, nodeoffset, "rng-seed", > >> abuf_data(&buf), abuf_size(&buf)); > >> diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c > >> index 4b22bb6f525a..8d70233fc08d 100644 > >> --- a/boot/pxe_utils.c > >> +++ b/boot/pxe_utils.c > >> @@ -8,6 +8,7 @@ > >> #include <dm.h> > >> #include <env.h> > >> #include <image.h> > >> +#include <kaslrseed.h> > >> #include <log.h> > >> #include <malloc.h> > >> #include <mapmem.h> > >> @@ -323,10 +324,6 @@ static void label_boot_kaslrseed(void) > >> #if CONFIG_IS_ENABLED(DM_RNG) > >> ulong fdt_addr; > >> struct fdt_header *working_fdt; > >> - size_t n = 0x8; > >> - struct udevice *dev; > >> - u64 *buf; > >> - int nodeoffset; > >> int err; > >> > >> /* Get the main fdt and map it */ > >> @@ -342,35 +339,9 @@ static void label_boot_kaslrseed(void) > >> if (err <= 0) > >> return; > >> > >> - if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) { > >> - printf("No RNG device\n"); > >> - return; > >> - } > >> - > >> - nodeoffset = fdt_find_or_add_subnode(working_fdt, 0, "chosen"); > >> - if (nodeoffset < 0) { > >> - printf("Reading chosen node failed\n"); > >> - return; > >> - } > >> - > >> - buf = malloc(n); > >> - if (!buf) { > >> - printf("Out of memory\n"); > >> - return; > >> - } > >> - > >> - if (dm_rng_read(dev, buf, n)) { > >> - printf("Reading RNG failed\n"); > >> - goto err; > >> - } > >> - > >> - err = fdt_setprop(working_fdt, nodeoffset, "kaslr-seed", buf, > >> sizeof(buf)); > >> - if (err < 0) { > >> + err = fdt_kaslrseed(working_fdt); > >> + if (err < 0) > >> printf("Unable to set kaslr-seed on chosen node: %s\n", > >> fdt_strerror(err)); > >> - goto err; > >> - } > >> -err: > >> - free(buf); > >> #endif > >> return; > >> } > >> diff --git a/cmd/kaslrseed.c b/cmd/kaslrseed.c > >> index e0d3c7fe7489..ea5c07d729cf 100644 > >> --- a/cmd/kaslrseed.c > >> +++ b/cmd/kaslrseed.c > >> @@ -9,33 +9,16 @@ > >> #include <command.h> > >> #include <dm.h> > >> #include <hexdump.h> > >> +#include <kaslrseed.h> > >> #include <malloc.h> > >> #include <rng.h> > >> #include <fdt_support.h> > >> > >> static int do_kaslr_seed(struct cmd_tbl *cmdtp, int flag, int argc, char > >> *const argv[]) > >> { > >> - size_t n = 0x8; > >> - struct udevice *dev; > >> - u64 *buf; > >> - int nodeoffset; > >> - int ret = CMD_RET_SUCCESS; > >> + int err; > >> > >> - if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) { > >> - printf("No RNG device\n"); > >> - return CMD_RET_FAILURE; > >> - } > >> - > >> - buf = malloc(n); > >> - if (!buf) { > >> - printf("Out of memory\n"); > >> - return CMD_RET_FAILURE; > >> - } > >> - > >> - if (dm_rng_read(dev, buf, n)) { > >> - printf("Reading RNG failed\n"); > >> - return CMD_RET_FAILURE; > >> - } > >> + printf("Notice: a /chosen/kaslr-seed is automatically added to the > >> device-tree when booted via booti/bootm/bootz therefore using this command > >> is likely no longer needed"); > >> > >> if (!working_fdt) { > >> printf("No FDT memory address configured. Please > >> configure\n" > >> @@ -44,27 +27,13 @@ static int do_kaslr_seed(struct cmd_tbl *cmdtp, int > >> flag, int argc, char *const > >> return CMD_RET_FAILURE; > >> } > >> > >> - ret = fdt_check_header(working_fdt); > >> - if (ret < 0) { > >> - printf("fdt_chosen: %s\n", fdt_strerror(ret)); > >> + err = fdt_kaslrseed(working_fdt); > >> + if (err < 0) { > >> + printf("Unable to set kaslr-seed on chosen node: %s\n", > >> fdt_strerror(err)); > >> return CMD_RET_FAILURE; > >> } > >> > >> - nodeoffset = fdt_find_or_add_subnode(working_fdt, 0, "chosen"); > >> - if (nodeoffset < 0) { > >> - printf("Reading chosen node failed\n"); > >> - return CMD_RET_FAILURE; > >> - } > >> - > >> - ret = fdt_setprop(working_fdt, nodeoffset, "kaslr-seed", buf, > >> sizeof(buf)); > >> - if (ret < 0) { > >> - printf("Unable to set kaslr-seed on chosen node: %s\n", > >> fdt_strerror(ret)); > >> - return CMD_RET_FAILURE; > >> - } > >> - > >> - free(buf); > >> - > >> - return ret; > >> + return CMD_RET_SUCCESS; > >> } > >> > >> U_BOOT_LONGHELP(kaslrseed, > >> diff --git a/include/kaslrseed.h b/include/kaslrseed.h > >> new file mode 100644 > >> index 000000000000..3fe82f418acf > >> --- /dev/null > >> +++ b/include/kaslrseed.h > >> @@ -0,0 +1,17 @@ > >> +/* SPDX-License-Identifier: GPL-2.0+ */ > >> +/* > >> + * Copyright 2024 Tim Harvey <thar...@gateworks.com> > >> + */ > >> + > >> +#if !defined _KASLRSEED_H_ > >> +#define _KASLRSEED_H_ > >> + > >> +/** > >> + * fdt_kaslrseed() - create a 'kaslr-seed' node in chosen > >> + * > >> + * @blob: fdt blob > >> + * Return: 0 if OK, -ve on error > >> + */ > >> +int fdt_kaslrseed(void *blob); > >> + > >> +#endif /* _KASLRSEED_H_ */ > >> diff --git a/lib/Makefile b/lib/Makefile > >> index 2a76acf100d0..20a0242055fa 100644 > >> --- a/lib/Makefile > >> +++ b/lib/Makefile > >> @@ -149,6 +149,7 @@ obj-y += date.o > >> obj-y += rtc-lib.o > >> obj-$(CONFIG_LIB_ELF) += elf.o > >> > >> +obj-$(CONFIG_DM_RNG) += kaslrseed.o > >> obj-$(CONFIG_$(SPL_TPL_)SEMIHOSTING) += semihosting.o > >> > >> # > >> diff --git a/lib/kaslrseed.c b/lib/kaslrseed.c > >> new file mode 100644 > >> index 000000000000..ad06bee2b88d > >> --- /dev/null > >> +++ b/lib/kaslrseed.c > >> @@ -0,0 +1,34 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * Copyright 2024 Tim Harvey <thar...@gateworks.com> > >> + */ > >> +#include <dm.h> > >> +#include <rng.h> > >> +#include <fdt_support.h> > >> + > >> +int fdt_kaslrseed(void *fdt) > >> +{ > >> + struct udevice *dev; > >> + int nodeoffset; > >> + u64 data; > >> + int err; > >> + > >> + err = fdt_check_header(fdt); > >> + if (err < 0) > >> + return err; > >> + > >> + /* find or create "/chosen" node. */ > >> + nodeoffset = fdt_find_or_add_subnode(fdt, 0, "chosen"); > >> + if (nodeoffset < 0) > >> + return nodeoffset; > >> + > >> + err = uclass_get_device(UCLASS_RNG, 0, &dev); > >> + if (!err) > >> + err = dm_rng_read(dev, &data, sizeof(data)); > >> + if (!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)); > > I have used debug messages to better track if error happens. I think it would > be > good if you can extend your code with it. > Because if this fails people have to start tracking if issue is that there is > RNG device or reading fails or issue with setting up property.
Hi Michal, I'm assuming there is no valid case to have DM_RNG enabled and no UCLASS_RNG driver which is why I'm printing an error if it ultimately fails (vs a silent debug statement). If that is true then I should probably printf errors for all the cases above vs debugs wouldn't you think? > > In v1 it was said that you won't update it if the kaslr-seed property exists. > I can't see this in the code. Or do I read it incorrectly? > That was a question posed in the discussion but I didn't get an answer if that was what I should do. None of the three places I'm deduplicating the functionality from check to see if it already exists... they would overwrite an existing one. Is there a valid case where you would not want to overwrite it? > > >> + > >> + return err; > >> +} > >> -- > >> 2.25.1 > >> > > > > I realized I forgot a few CC's here. Adding: > > > > Michal Simek <michal.si...@amd.com> (board/xilinx/common/board.c and > > configs/xilinx* which have CMD_KASLRSEED) > > Andy Yan <andy....@rock-chips.com> (configs/evb-rk3308_defconfig and > > configs/roc-cc-rk3308_defconfig which have CMD_KASLRSEED) > > Akash Gajjar <gajjar04ak...@gmail.com> > > (configs/rock-pi-s-rk3308_defconfig which has CMD_KASLRSEED) > > Ilias Apalodimas <ilias.apalodi...@linaro.org> (MEASURE_DEVICETREE, > > MEASURED_BOOT) > > > > Michal, I see that board/xilinx/common/board.c is adding > > chosen/kaslr-seed in ft_board_setup as well as some of the > > configs/xilinx boards enabling CMD_KASLRSEED - can we remove > > CMD_ASLRSEED from those boards? (ie no bootscripts require it?) > > > Based on my note above that you shouldn't change kaslr-seed if exists. I can add that if you think it's the correct behavior, but that is not what is done in the current code. > > What would also make sense is to extend kaslrseed command to pass one more > parameter to pass which RNG instance should generate data on system with more > RNGs. Then pretty much with command you can choose different RNG because > current > code hardcoded it to instance 0. Do you mean extend the command or the fdt_kaslrseed function? I'm trying to obsolete the command. I can add an index to fdt_kaslrseed but would it be more useful to add a udevice* that is used unless null? I can't see where someone wanting to call this function would know the index of a specific UCLASS_RNG device and not have the device * handy instead. It sounds like with the question of not overwriting an existing value you are anticipating board specific ft_board_setup functions that may want to behave differently and choose a different rng source? If so, ft_board_setup is called 'after' fdt_chosen so maybe fdt_kaslrseed also needs a boolean to tell it to overwrite existing? Are you open to removing CMD_KASLRSEED from the configs/xilinx* which have it? If I can get everyone's approval for the few boards that include CMD_KASLRSEED I would like to remove the command if we are doing it automatically. (adding Chris Morgan <macromor...@hotmail.com> to cc who authored that cmd) > > Do you have correct dependency in generic code when MEASURED_BOOT is enabled > as > was mentioned in v1? If yes it should be mentioned in commit message. > Likely something is needed but I'm not sure if it's MEASURED_BOOT or MEASURE_DEVICETREE - That's a question I posed to Ilias. Best Regards, Tim