[PATCH v1 6/6] arm64: Enable RW, RX and RO mappings for the relocated binary

[Jerome Forissier]

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-450ccdadfa7e@cherry.de/
> Link: https://lore.kernel.org/u-boot/20250130133646.2177194-1-andre.przywara@arm.com/
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at 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.forissier at linaro.org>

Thanks,
-- 
Jerome