On 2/5/25 08:16, Ilias Apalodimas wrote:
> Now that we have everything in place switch the page permissions for
> .rodata, .text and .data just after we relocate everything in top of the
> RAM.
>
> Unfortunately we can't enable this by default, since we have examples of
> U-Boot crashing due to invalid access. This usually happens because code
> defines const variables that it later writes. So hide it behind a Kconfig
> option until we sort it out.
>
> It's worth noting that EFI runtime services are not covered by this
> patch on purpose. Since the OS can call SetVirtualAddressMap which can
> relocate runtime services, we need to set them to RX initially but remap
> them as RWX right before ExitBootServices.
>
> Link:
> https://lore.kernel.org/u-boot/20250129-rockchip-pinctrl-const-v1-0-450ccdadf...@cherry.de/
> Link:
> https://lore.kernel.org/u-boot/20250130133646.2177194-1-andre.przyw...@arm.com/
> Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> ---
> common/Kconfig | 13 +++++++++++++
> common/board_r.c | 20 ++++++++++++++++++++
> 2 files changed, 33 insertions(+)
>
> diff --git a/common/Kconfig b/common/Kconfig
> index 7685914fa6fd..dbae7e062b0a 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -914,6 +914,19 @@ config STACKPROTECTOR
> Enable stack smash detection through compiler's stack-protector
> canary logic
>
> +config MMU_PGPROT
> + bool "Enable RO, RW and RX mappings"
> + help
> + U-Boot maps all pages as RWX. If selected pages will
> + be marked as RO(.rodata), RX(.text), RW(.data) right after
Space before (
> + we relocate. Since code sections needs to be page aligned
> + the final binary size will increase.
> + The mapping can be dumped using the 'meminfo' command.
OK
> + We should make this default 'y' in the future, but currently
> + we have code defining const variables that are later written.
> + Enabling this will crash U-Boot if that code path runs, so keep
> + it off by default until we fix invalid accesses.
Not sure this needs to be documented in the Kconfig help. Perhaps just
keep a patch ready and send it early in the next release cycle for people
to test and debug?
> +
> config SPL_STACKPROTECTOR
> bool "Stack Protector buffer overflow detection for SPL"
> depends on STACKPROTECTOR && SPL
> diff --git a/common/board_r.c b/common/board_r.c
> index 179259b00de8..c1e9aa46e3fa 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -170,7 +170,27 @@ static int initr_reloc_global_data(void)
> efi_save_gd();
>
> efi_runtime_relocate(gd->relocaddr, NULL);
> +
> #endif
> + /*
> + * We are done with all relocations change the permissions of the binary
> + * NOTE: __start_rodata etc are defined in arm64 linker scripts and
> + * sections.h. If you want to add support for your platform you need to
> + * add the symbols on your linker script, otherwise they will point to
> + * random addresses.
> + *
> + */
> + if (IS_ENABLED(CONFIG_MMU_PGPROT)) {
> + pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_rodata),
> + (phys_addr_t)(uintptr_t)(__end_rodata -
> __start_rodata),
> + MMU_ATTR_RO);
> + pgprot_set_attrs((phys_addr_t)(uintptr_t)(__start_data),
> + (phys_addr_t)(uintptr_t)(__end_data -
> __start_data),
> + MMU_ATTR_RW);
> + pgprot_set_attrs((phys_addr_t)(uintptr_t)(__text_start),
> + (phys_addr_t)(uintptr_t)(__text_end -
> __text_start),
> + MMU_ATTR_RX);
> + }
>
> return 0;
> }
Reviewed-by: Jerome Forissier <jerome.foriss...@linaro.org>
Thanks,
--
Jerome
