On 5/15/24 02:50, Marek Vasut wrote:
On 5/15/24 2:22 AM, 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.
Thanks for working on this one, this is really nice.
The general direction of always supplying a seed for KASLR is right. But
there are some items to observe:
We already have multiple places where /chosen/kaslr-seed is set, e.g.
arch/arm/cpu/armv8/sec_firmware.c and board/xilinx/common/board.c.
Some boards are using the kaslrseed command to initialize
/chosen/kaslr-seed from DM_RNG.
It does not make sense to set it multiple times from different sources
of randomness. I am missing the necessary clean-up in this patch.
For CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT=y the right way forward could be
moving sec_firmware_get_random() into the driver model. Tom is the
maintainer for this code.
For Xilinx boards your patch obsoletes part of the code in
ft_board_setup() of board/xilinx/common/board.c. CCing Michal as maintainer.
The kaslrseed command similarly becomes obsolete with your patch and
should be removed. 'git grep -n CMD_KASLR' indicates which defconfigs
would be impacted.
label_boot_kaslrseed() needs review too.
kaslr-seed is not compatible with measured boot if the device-tree is
measured. When booting via EFI in efi_try_purge_kaslr_seed() we can
safely remove the value because it is not used anyway; we provide the
EFI_RNG_PROTOCOL instead. We also support measured boot via the legacy
Linux entry point. See patch dec166d6b2c2 ("bootm: Support boot
measurement"). We should not populate kaslr-seed if
CONFIG_MEASURE_DEVICETREE=y. CCing Eddie and Ilias as they have been
working on measured boot.
If we have DM_RNG enabled poulate this value automatically when
nits
%s/poulate/populate/
Best regards
Heinrich
fdt_chosen is called.
Signed-off-by: Tim Harvey <thar...@gateworks.com>
---
boot/fdt_support.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/boot/fdt_support.c b/boot/fdt_support.c
index 874ca4d6f5af..cd3069baf450 100644
--- a/boot/fdt_support.c
+++ b/boot/fdt_support.c
@@ -7,10 +7,12 @@
*/
#include <abuf.h>
+#include <dm.h>
#include <env.h>
#include <log.h>
#include <mapmem.h>
#include <net.h>
+#include <rng.h>
#include <stdio_dev.h>
#include <dm/ofnode.h>
#include <linux/ctype.h>
@@ -300,6 +302,27 @@ int fdt_chosen(void *fdt)
if (nodeoffset < 0)
return nodeoffset;
+ if (IS_ENABLED(CONFIG_DM_RNG)) {
+ struct udevice *dev;
+ size_t len = 0x8;
+ u64 *data;
+
+ data = malloc(len);
Can you allocate this 8 byte array on stack , i.e. u64 data[2]; ?
cmd/kaslrseed.c could use similar clean up (and
lib/efi_loader/efi_dt_fixup.c and boot/pxe_utils.c ... uhhh). Maybe you
can deduplicate this functionality into common code shared by all those
duplicates before the duplication gets out of control ?
lib/kaslrseed.c looks like a good place to put the common stuff.
+ if (!data)
+ return -ENOMEM;
+
+ err = uclass_get_device(UCLASS_RNG, 0, &dev);
+ if (!err)
+ err = dm_rng_read(dev, data, len);
+ if (!err)
+ err = fdt_setprop(fdt, nodeoffset, "kaslr-seed", data, len);
+ if (err < 0) {
+ printf("WARNING: could not set kaslr-seed %s.\n",
+ fdt_strerror(err));
+ return err;
+ }
You're missing free() here, but it shouldn't be needed if you allocate
the array on stack, which is better/simpler.
