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

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

[Tim Harvey]

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 poulate this value automatically when
fdt_chosen is called.

Signed-off-by: Tim Harvey <tharvey@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);
+		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;
+		}
+	}
+
 	if (IS_ENABLED(CONFIG_BOARD_RNG_SEED) && !board_rng_seed(&buf)) {
 		err = fdt_setprop(fdt, nodeoffset, "rng-seed",
 				  abuf_data(&buf), abuf_size(&buf));
-- 
2.25.1

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

[Marek Vasut]

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.

> If we have DM_RNG enabled poulate this value automatically when
> fdt_chosen is called.
> 
> Signed-off-by: Tim Harvey <tharvey@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.

Re: [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@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.

Re: [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@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

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

[Marek Vasut]

On 5/15/24 9:57 PM, Tim Harvey wrote:

[...]

>> 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

These four i.MX8M I can test (and you can drop the kaslrseed command 
from them, no worries).

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

That sounds good to me.

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

[Tim Harvey]

On Tue, May 14, 2024 at 5:50 PM Marek Vasut <marex@denx.de> 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.
>
> > If we have DM_RNG enabled poulate this value automatically when
> > fdt_chosen is called.

Hi Marek,

Just noticed a typo in the commit log - I'll s/poulate/populate/ in v2

> >
> > Signed-off-by: Tim Harvey <tharvey@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]; ?
>

Sure... that makes sense - u64 data (just 1 64bit value)

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

Yes I started off making a function to do this but then I noticed we
had an fdt_chosen function and it fit there nicer as I didn't have to
find/create the chosen node. I also didn't know of a great place to
put it.

I will create a lib/kaslrseed.c with function for v2.

>
> > +             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.

Yes, I will avoid the malloc to fix that.

Thanks,

Tim

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

[Marek Vasut]

On 5/15/24 6:29 PM, Tim Harvey wrote:
> On Tue, May 14, 2024 at 5:50 PM Marek Vasut <marex@denx.de> 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.
>>
>>> If we have DM_RNG enabled poulate this value automatically when
>>> fdt_chosen is called.
> 
> Hi Marek,
> 
> Just noticed a typo in the commit log - I'll s/poulate/populate/ in v2
> 
>>>
>>> Signed-off-by: Tim Harvey <tharvey@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]; ?
>>
> 
> Sure... that makes sense - u64 data (just 1 64bit value)

Oh, right. Thanks for fixing it all up and keeping an eye on all the bugs !