[PATCH] fdt: add kaslr-seed if DM_RNG is enabled

[Tim Harvey]

On Tue, May 14, 2024 at 10:17 PM Heinrich Schuchardt <xypron.glpk at 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