Hi Jerome,
On Wed, 5 Feb 2025 at 11:57, Jerome Forissier
<jerome.foriss...@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-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 (
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.foriss...@linaro.org>
Thanks!
/Ilias
>
> Thanks,
> --
> Jerome
