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

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@linaro.org>
---
 common/Kconfig   | 13 +++++++++++++
 common/board_r.c | 20 ++++++++++++++++++++
 2 files changed, 33 insertions(+)

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@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@linaro.org>

Thanks,

Hi Jerome,

On Wed, 5 Feb 2025 at 11:57, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
>
>
> 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@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 (

Sure

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

I'd like people to see it and try to debug when they see random
crashes. I think it's easier if we document that here until things are
fixed.
OTOH i don't really mind whatever people think it's best

>
> > +
> >  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@linaro.org>

Thanks!
/Ilias
>
> Thanks,
> --
> Jerome

On Wed, Feb 05, 2025 at 12:31:33PM +0200, Ilias Apalodimas wrote:
> Hi Jerome,
> 
> On Wed, 5 Feb 2025 at 11:57, Jerome Forissier
> <jerome.forissier@linaro.org> wrote:
> >
> >
> >
> > 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@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 (
> 
> Sure
> 
> >
> > > +       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?
> 
> I'd like people to see it and try to debug when they see random
> crashes. I think it's easier if we document that here until things are
> fixed.
> OTOH i don't really mind whatever people think it's best

We should change the tone / emphasis I think. Something like:

Enabling this feature can expose bugs in U-Boot where we have code that
violates read-only permissions for example. Use this feature with
caution.

And then we should encourage our more active SoC maintainers to default
this option to being enabled after testing out a few platforms as long
term it should be on by default but initial testing has shown that
assuming most platforms are OK isn't a good idea.