| Message ID | 20250130072100.27297-1-ilias.apalodimas@linaro.org |
|---|---|
| Headers | show |
| Series | Fix page permission on arm64 architectures | expand |
Hi Ilias, On 30/01/25 12:50 pm, Ilias Apalodimas 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 > - 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 > > [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(-) This patch series is increasing u-boot size by 13kB which is breaking our mini u-boot configurations. Without patch: text data bss dec hex filename 0 115632 0 115632 1c3b0 u-boot.elf With patch: text data bss dec hex filename 0 128688 0 128688 1f6b0 u-boot.elf Regards, Love Kumar > > -- > 2.43.0 >
Hi Love, On Tue, 4 Feb 2025 at 13:53, Love Kumar <love.kumar@amd.com> wrote: > > Hi Ilias, > > > On 30/01/25 12:50 pm, Ilias Apalodimas 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 > > - 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 > > > > [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(-) > > This patch series is increasing u-boot size by 13kB which is breaking > our mini u-boot configurations. > There's not much I can do about it, since sections need to be page-aligned to be able to set permissions. On v1 I'll have the option under a Kconfig, so you need to keep it disabled. /Ilias > Without patch: > text data bss dec hex filename > 0 115632 0 115632 1c3b0 u-boot.elf > > With patch: > text data bss dec hex filename > 0 128688 0 128688 1f6b0 u-boot.elf > > Regards, > Love Kumar > > > > > -- > > 2.43.0 > >
Hi Ilias, On 04/02/25 5:42 pm, Ilias Apalodimas wrote: > Hi Love, > > On Tue, 4 Feb 2025 at 13:53, Love Kumar <love.kumar@amd.com> wrote: >> >> Hi Ilias, >> >> >> On 30/01/25 12:50 pm, Ilias Apalodimas 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 >>> - 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 >>> >>> [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(-) >> >> This patch series is increasing u-boot size by 13kB which is breaking >> our mini u-boot configurations. >> > > There's not much I can do about it, since sections need to be > page-aligned to be able to set permissions. > On v1 I'll have the option under a Kconfig, so you need to keep it disabled. Thank you. That will work for us to keep it disabled for mini. Regards, Love Kumar > > /Ilias > >> Without patch: >> text data bss dec hex filename >> 0 115632 0 115632 1c3b0 u-boot.elf >> >> With patch: >> text data bss dec hex filename >> 0 128688 0 128688 1f6b0 u-boot.elf >> >> Regards, >> Love Kumar >> >>> >>> -- >>> 2.43.0 >>>
Hi Ilias, On Thu, 30 Jan 2025 at 00:21, Ilias Apalodimas <ilias.apalodimas@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. > - 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. Please make all that stuff an option. Try to use generic structs to print it, not ARM-specific code. > > [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
Hi Simon, On Thu, 6 Feb 2025 at 14:33, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Thu, 30 Jan 2025 at 00:21, Ilias Apalodimas > <ilias.apalodimas@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. > 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. 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
Hi Ilias, On Thu, 6 Feb 2025 at 05:40, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Thu, 6 Feb 2025 at 14:33, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Thu, 30 Jan 2025 at 00:21, Ilias Apalodimas > > <ilias.apalodimas@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
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 - 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 [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