On Tue, May 14, 2024 at 10:17 PM Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > 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. >
Hi Heinrich, Yes, Marek pointed out the same thing about de-duplicating code. I can add a lib/kaslrseed.c with a fdt_kaslrseed function to deduplicate the usage. > 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. ok, I will not address arch/arm/cpu/armv8/sec_firmware.c and will protect my implementation with 'if (IS_ENABLED(CONFIG_DM_RNG) && !IS_ENABLED(ARMV8_SEC_FIRMWARE_SUPPORT))' > > For Xilinx boards your patch obsoletes part of the code in > ft_board_setup() of board/xilinx/common/board.c. CCing Michal as maintainer. > yes, I can remove that code to deduplicate as they are using dm_rng_read() thus users of that must have CONFIG_DM_RNG enabled. > The kaslrseed command similarly becomes obsolete with your patch and > should be removed. 'git grep -n CMD_KASLR' indicates which defconfigs > would be impacted. > There are several users of this command currently: $ git grep CMD_KASLR configs/ configs/evb-rk3308_defconfig:CONFIG_CMD_KASLRSEED=y configs/imx8mm_data_modul_edm_sbc_defconfig:CONFIG_CMD_KASLRSEED=y configs/imx8mp_data_modul_edm_sbc_defconfig:CONFIG_CMD_KASLRSEED=y configs/imx8mp_dhcom_pdk2_defconfig:CONFIG_CMD_KASLRSEED=y configs/imx8mp_dhcom_pdk3_defconfig:CONFIG_CMD_KASLRSEED=y configs/roc-cc-rk3308_defconfig:CONFIG_CMD_KASLRSEED=y configs/rock-pi-s-rk3308_defconfig:CONFIG_CMD_KASLRSEED=y configs/xilinx_versal_net_virt_defconfig:CONFIG_CMD_KASLRSEED=y configs/xilinx_versal_virt_defconfig:CONFIG_CMD_KASLRSEED=y configs/xilinx_zynqmp_kria_defconfig:CONFIG_CMD_KASLRSEED=y configs/xilinx_zynqmp_virt_defconfig:CONFIG_CMD_KASLRSEED=y While I can deduplicate code by calling a shared function in that command I don't feel like I should remove that command until the maintainers of the boards above agree on removing it from their defconfigs as they could have bootscripts that fail with the command missing. I could add a warning print in the kaslrseed command indicating that the use of this command is no longer needed. > label_boot_kaslrseed() needs review too. > yes, this can be deduplicated > 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. > board/raspberrypi/rpi/rpi.c:ft_board_setup copies /chosen/kaslr-seed from an fdt apparently passed in from a lower level firmware stage. should I check to see if /chosen/kaslr-seed exists and never overwrite it or just let this get overwritten? > > > >> If we have DM_RNG enabled poulate this value automatically when > > nits > > %s/poulate/populate/ > yes, I will fix that as well. Thanks for the review! Best Regards, Tim