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

[Heinrich Schuchardt]

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 <tharvey at 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.