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

[Ilias Apalodimas]

Hi Jerome,

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

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

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