| Message ID | 20240515002248.2920155-1-tharvey@gateworks.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | fdt: add kaslr-seed if DM_RNG is enabled | expand |
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.
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.
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
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 !
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
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.
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));
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(+)