Hi,
On 1/6/26 1:53 PM, Sascha Hauer wrote:
> Enable hardware-enforced W^X (Write XOR Execute) memory protection through
> ELF segment-based permissions using the RISC-V MMU.
>
> This implementation provides memory protection for RISC-V S-mode using
> Sv39 (RV64) or Sv32 (RV32) page tables.
>
> Linker Script Changes:
> - Add PHDRS directives to pbl.lds.S and barebox.lds.S
> - Create three separate PT_LOAD segments with proper permissions:
> * text segment (FLAGS(5) = PF_R|PF_X): code sections
> * rodata segment (FLAGS(4) = PF_R): read-only data
> * data segment (FLAGS(6) = PF_R|PF_W): data and BSS
> - Add 4K alignment between segments for page-granular protection
That's a separate patch from this.
> S-mode MMU Implementation (mmu.c):
> - Implement page table walking for Sv39/Sv32
> - pbl_remap_range(): remap segments with ELF-derived permissions
> - mmu_early_enable(): create identity mapping and enable SATP CSR
> - Map ELF flags to PTE bits:
> * MAP_CODE → PTE_R | PTE_X (read + execute)
> * MAP_CACHED_RO → PTE_R (read only)
> * MAP_CACHED → PTE_R | PTE_W (read + write)
>
> Integration:
> - Update uncompress.c to call mmu_early_enable() before decompression
> (enables caching for faster decompression)
> - Call pbl_mmu_setup_from_elf() after ELF relocation to apply final
> segment-based permissions
> - Uses portable pbl/mmu.c infrastructure to parse PT_LOAD segments
>
> Configuration:
> - Add CONFIG_MMU option (default y for RISCV_S_MODE)
> - Update asm/mmu.h with ARCH_HAS_REMAP and function declarations
>
> Security Benefits:
> - Text sections are read-only and executable (cannot be modified)
> - Read-only data sections are read-only and non-executable
> - Data sections are read-write and non-executable (cannot be executed)
> - Hardware-enforced W^X prevents code injection attacks
>
> This matches the ARM implementation philosophy and provides genuine
> security improvements on RISC-V S-mode platforms.
Mhm, I think this commit message needs to be toned down a bit..
> +#ifdef CONFIG_MMU
As with feedback on V1, why no IS_ENABLED() or stub?
> +#ifdef CONFIG_MMU
Likewise.
> +#ifdef __PBL__
See comments on v1.
> + case MAP_CACHED:
> + case MAP_UNCACHED:
Add a TODO here or some other comment that uncached memory is not support.
> +/* CSR access */
> +#define csr_read(csr) \
We already define these. Refer to feedback on v1.
> +#if defined(CONFIG_MMU) || defined(CONFIG_RISCV_PMP)
Not defined symbol (CONFIG_RISCV_PMP)
> index
> 32d9a7aca3b9a61d542bf3e21e27f1ac51f43ee2..ecd44d4ac756009cd44d7dedbda5a13f1ca3f93d
> 100644
> --- a/include/mmu.h
> +++ b/include/mmu.h
> @@ -20,6 +20,9 @@
> #define MAP_TYPE_MASK 0xFFFF
> #define MAP_ARCH(x) ((u16)~(x))
>
> +#include <asm/mmu.h>
> +#include <asm/io.h>
> +
> /*
> * Depending on the architecture the default mapping can be
> * cached or uncached. Without ARCH_HAS_REMAP being set this
> @@ -27,9 +30,6 @@
> */
> #define MAP_DEFAULT MAP_ARCH_DEFAULT
>
> -#include <asm/mmu.h>
> -#include <asm/io.h>
Why is this moved around?
> -
> static inline bool maptype_is_compatible(maptype_t active, maptype_t check)
> {
> active &= MAP_TYPE_MASK;
> @@ -47,7 +47,7 @@ static inline bool maptype_is_compatible(maptype_t active,
> maptype_t check)
> static inline int arch_remap_range(void *virt_addr, phys_addr_t phys_addr,
> size_t size, maptype_t map_type)
> {
> - if (maptype_is_compatible(map_type, MAP_ARCH_DEFAULT) &&
> + if (maptype_is_compatible(map_type, MAP_DEFAULT) &&
Unneeded change?
> phys_addr == virt_to_phys(virt_addr))
> return 0;
>
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
