Re: [PATCH v2 21/21] riscv: add ELF segment-based memory protection with MMU

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |