[RFC PATCH 0/4] Fix page permission on arm64 architectures

Simon Glass sjg at chromium.org
Sun Feb 9 15:28:11 CET 2025 

Hi Ilias,

On Thu, 6 Feb 2025 at 05:40, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 6 Feb 2025 at 14:33, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, 30 Jan 2025 at 00:21, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > U-Boot maps all pages as RWX. Sadly it's not not 1990 anymore and we are
> > > better off mapping binaries with proper permissions.
> > >
> > > This is an attempt to map the U-Boot binary properly, but leave the
> > > area we load binaries unaffected and RWX.
> > >
> > > patch #1 adds printing capabilities for page permissions in meminfo
> > > patch #2 prepares our linker scripts, aligns sections in page boundaries etc
> > > patch #3 is adding a function to change page tables
> > > patch #4 sets the permissions for .rodata, .text and .data
> > >
> > > There's a few problems problem in this RFC ...
> > > - U-Boot has no MMU APIs in place. As a result I just wired up the function
> > >   changing the page permissions in board_r.c but only for arm64. In the long
> > >   term we should add an API that other archs can use if they wish/can
> >
> > But this creates more work for others. It would be better to do the
> > job properly at the start. You don't need to support other archs, but
> > you should create a plausible API and some structs which others can
> > use. See cpu.h for some ideas. Of course you can't predict the future,
> > but others will build on it if you make an effort. Otherwise, I doubt
> > anyone else will (later) clean it up.
>
> I can move that function to cpu_ops.h and have if NULL for other archs
>
> >
> > > - meminfo doesn't align all results properly (Caleb?)
> > > - I am not setting the permissions of EFI runtime services section on purpose.
> > >   Since the OS is allowed to call SetVirtualAddressMap, we need to reset the
> > >   pages to RWX just before ExitBootServices, so the OS can relocate that code.
> > >   In arm64 this isn't a huge problem, because we explicitly disable SVAM if
> > >   if VA_BITS > 39, but other OSes/archs will have a problem.
> > >
> > > # TODO:
> > > - Long term. Add an MMU API and use that instead of exporting arch specific
> > >   functions
> > > - Add an enum in mmu_set_attrs() instead of 1,2,3 arguments
> > > - Set the permissions of EFI runtime services anbd reset them before EBS()
> > > - Fix alignment when printing pages & information
> > > - Add a Kconfig option so the feature can be turned off.
> > >   The reason is that QEMU CI and some boards, fail because they are trying to
> > >   write RO pages. This usually happens due to variables being wrongly defined as
> > >   const, but with the updated page permissions this leads to a crash [0] [1].
> > >   In both cases the reported ESR is 0x9600004f which translates to
> > >   "Abort caused by writing to memory" "Permission fault, level 3.". On top of
> > >   that not setting pages as RO (and only setting .data and .text sections) works
> > >   fine. So until we find and fix the bugs above we can't turn this on
> > >   unconditionally.
> > >
> > > # Open questions
> > > - Is initr_reloc_global_data() the right place to change permissions? Or is there
> > >   a better/safer place to do that?
> > >
> > > # How to test patches
> > > - Aplly them an enable
> > >   CONFIG_CMD_MEMINFO=y
> > >   CONFIG_CMD_MEMINFO_MAP=y
> > >   'meminfo' should print something along the lines of
> > > => meminfo
> > > DRAM:  8 GiB
> > > Walking pagetable at 000000023ffe0000, va_bits: 40. Using 4 levels
> > > [0x23ffe1000]                     |  Table |               |
> > >   [0x23ffe2000]                   |  Table |               |
> > >     [0x000000000 - 0x008000000]   |  Block | RWX | Normal        | Inner-shareable
> > >     [0x008000000 - 0x040000000]   |  Block | RW | Device-nGnRnE | Non-shareable
> > >   [0x040000000 - 0x200000000]     |  Block | RWX | Normal        | Inner-shareable
> > >   [0x23ffea000]                   |  Table |               |
> > >     [0x200000000 - 0x23f600000]   |  Block | RWX | Normal        | Inner-shareable
> > >     [0x23ffeb000]                 |  Table |               |
> > >       [0x23f600000 - 0x23f68b000] |  Pages | RWX | Normal        | Inner-shareable
> > >       [0x23f68b000 - 0x23f74e000] |  Pages | RX | Normal        | Inner-shareable
> > >       [0x23f74e000 - 0x23f793000] |  Pages | RO | Normal        | Inner-shareable
> > >       [0x23f793000 - 0x23f794000] |  Pages | RWX | Normal        | Inner-shareable
> > >       [0x23f794000 - 0x23f79d000] |  Pages | RW | Normal        | Inner-shareable
> > >       [0x23f79d000 - 0x23f800000] |  Pages | RWX | Normal        | Inner-shareable
> > >     [0x23f800000 - 0x240000000]   |  Block | RWX | Normal        | Inner-shareable
> > >   [0x240000000 - 0x4000000000]     |  Block | RWX | Normal        | Inner-shareable
> > >   [0x23ffe3000]                   |  Table |               |
> > >     [0x4010000000 - 0x4020000000]   |  Block | RW | Device-nGnRnE | Non-shareable
> > > [0x23ffe4000]                     |  Table |               |
> > >   [0x8000000000 - 0x10000000000]     |  Block | RW | Device-nGnRnE | Non-shareable
> >
> > No, this is not a memory map.
>
> This is *literally* the memory map.

The memory map currently has a single address and describes what each
region is for. This looks like a detailed list of some other
attributes.

>
> > Please make all that stuff an option.
> > Try to use generic structs to print it, not ARM-specific code.
>
> How? The information in the page tables is arch specific.

enum cpu_mem_permission {
   CPUMP_RO = BIT(0),
   CPUMP_RW = BIT(1),
   etc.
};

struct cpu_mem_attrs {
   uint permission;   /* enum cpu_mem_permission */
   (more things here)
};

Regards,
Simon


>
> Thanks
> /Ilias
> >
> > >
> > > [0] https://lore.kernel.org/u-boot/20250129-rockchip-pinctrl-const-v1-0-450ccdadfa7e@cherry.de/
> > > [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/jobs/1008714
> > >
> > > Ilias Apalodimas (4):
> > >   meminfo: add memory details for armv8
> > >   arm: Prepare linker scripts for memory permissions
> > >   arm64: mmu_change_region_attr() add an option not to break PTEs
> > >   arm64: Change mapping for data/rodata/text
> > >
> > >  Makefile                                | 15 +++++----
> > >  arch/arm/cpu/armv8/cache_v8.c           | 45 +++++++++++++++++++++++--
> > >  arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 10 +++---
> > >  arch/arm/cpu/armv8/u-boot.lds           | 32 +++++++++++-------
> > >  arch/arm/include/asm/armv8/mmu.h        |  2 ++
> > >  arch/arm/include/asm/system.h           |  3 +-
> > >  arch/arm/mach-snapdragon/board.c        |  2 +-
> > >  cmd/meminfo.c                           |  5 +++
> > >  common/board_r.c                        |  7 ++++
> > >  include/asm-generic/sections.h          |  2 ++
> > >  lib/efi_loader/efi_runtime.c            |  2 ++
> > >  11 files changed, 97 insertions(+), 28 deletions(-)
> > >
> > > --
> > > 2.43.0
> > >
> >
> > Regards,
> > Simon

More information about the U-Boot mailing list