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

[Tom Rini]

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

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250205/e4d1dd4f/attachment.sig>