Re: [PATCH 2/2] arm64: mm: use XN table mapping attributes for the linear region

[Mark Rutland <mark.rutland@arm.com>]

On Thu, Mar 04, 2021 at 06:11:45PM +0100, Ard Biesheuvel wrote:
> The way the arm64 kernel virtual address space is constructed guarantees
> that swapper PGD entries are never shared between the linear region on
> the one hand, and the vmalloc region on the other, which is where all
> kernel text, module text and BPF text mappings reside.
> 
> This means that mappings in the linear region (which never require
> executable permissions) never share any table entries at any level with
> mappings that do require executable permissions, and so we can set the
> table-level PXN/UXN attributes for all table entries that are created
> while setting up mappings in the linear region. Since swapper's PGD
> level page table is mapped r/o itself, this adds another layer of
> robustness to the way the kernel manages its own page tables.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/include/asm/pgtable-hwdef.h |  3 +++
>  arch/arm64/mm/mmu.c                    | 27 +++++++++++++++-----
>  2 files changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
> index e64e77a345b2..b6f50abd63d7 100644
> --- a/arch/arm64/include/asm/pgtable-hwdef.h
> +++ b/arch/arm64/include/asm/pgtable-hwdef.h
> @@ -101,6 +101,7 @@
>  #define P4D_TYPE_MASK		(_AT(p4dval_t, 3) << 0)
>  #define P4D_TYPE_SECT		(_AT(p4dval_t, 1) << 0)
>  #define P4D_SECT_RDONLY		(_AT(p4dval_t, 1) << 7)		/* AP[2] */
> +#define P4D_TABLE_XN		(_AT(p4dval_t, 3) << 59)	/* UXN:PXN */

I reckon it's worth having separate P?D_TABLE_UXN and P?D_TABLE_PXN
definitions, so that it's easier/possible to:

* Set UXNTable for *all* kernel mappings

* Set PXNTable for *all* user mappings

... though we don't need to do either immediately.

Otherwise, this looks good to me, and with or without that split:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

>  
>  /*
>   * Level 1 descriptor (PUD).
> @@ -110,6 +111,7 @@
>  #define PUD_TYPE_MASK		(_AT(pudval_t, 3) << 0)
>  #define PUD_TYPE_SECT		(_AT(pudval_t, 1) << 0)
>  #define PUD_SECT_RDONLY		(_AT(pudval_t, 1) << 7)		/* AP[2] */
> +#define PUD_TABLE_XN		(_AT(pudval_t, 3) << 59)	/* UXN:PXN */
>  
>  /*
>   * Level 2 descriptor (PMD).
> @@ -131,6 +133,7 @@
>  #define PMD_SECT_CONT		(_AT(pmdval_t, 1) << 52)
>  #define PMD_SECT_PXN		(_AT(pmdval_t, 1) << 53)
>  #define PMD_SECT_UXN		(_AT(pmdval_t, 1) << 54)
> +#define PMD_TABLE_XN		(_AT(pmdval_t, 3) << 59)	/* UXN:PXN */
>  
>  /*
>   * AttrIndx[2:0] encoding (mapping attributes defined in the MAIR* registers).
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 029091474042..ff5cbca403be 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -39,6 +39,7 @@
>  
>  #define NO_BLOCK_MAPPINGS	BIT(0)
>  #define NO_CONT_MAPPINGS	BIT(1)
> +#define NO_EXEC_MAPPINGS	BIT(2)
>  
>  u64 idmap_t0sz = TCR_T0SZ(VA_BITS);
>  u64 idmap_ptrs_per_pgd = PTRS_PER_PGD;
> @@ -185,10 +186,14 @@ static void alloc_init_cont_pte(pmd_t *pmdp, unsigned long addr,
>  
>  	BUG_ON(pmd_sect(pmd));
>  	if (pmd_none(pmd)) {
> +		pmdval_t pmdval = PMD_TYPE_TABLE;
>  		phys_addr_t pte_phys;
> +
> +		if (flags & NO_EXEC_MAPPINGS)
> +			pmdval |= PMD_TABLE_XN;
>  		BUG_ON(!pgtable_alloc);
>  		pte_phys = pgtable_alloc(PAGE_SHIFT);
> -		__pmd_populate(pmdp, pte_phys, PMD_TYPE_TABLE);
> +		__pmd_populate(pmdp, pte_phys, pmdval);
>  		pmd = READ_ONCE(*pmdp);
>  	}
>  	BUG_ON(pmd_bad(pmd));
> @@ -259,10 +264,14 @@ static void alloc_init_cont_pmd(pud_t *pudp, unsigned long addr,
>  	 */
>  	BUG_ON(pud_sect(pud));
>  	if (pud_none(pud)) {
> +		pudval_t pudval = PUD_TYPE_TABLE;
>  		phys_addr_t pmd_phys;
> +
> +		if (flags & NO_EXEC_MAPPINGS)
> +			pudval |= PUD_TABLE_XN;
>  		BUG_ON(!pgtable_alloc);
>  		pmd_phys = pgtable_alloc(PMD_SHIFT);
> -		__pud_populate(pudp, pmd_phys, PUD_TYPE_TABLE);
> +		__pud_populate(pudp, pmd_phys, pudval);
>  		pud = READ_ONCE(*pudp);
>  	}
>  	BUG_ON(pud_bad(pud));
> @@ -306,10 +315,14 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end,
>  	p4d_t p4d = READ_ONCE(*p4dp);
>  
>  	if (p4d_none(p4d)) {
> +		p4dval_t p4dval = P4D_TYPE_TABLE;
>  		phys_addr_t pud_phys;
> +
> +		if (flags & NO_EXEC_MAPPINGS)
> +			p4dval |= P4D_TABLE_XN;
>  		BUG_ON(!pgtable_alloc);
>  		pud_phys = pgtable_alloc(PUD_SHIFT);
> -		__p4d_populate(p4dp, pud_phys, P4D_TYPE_TABLE);
> +		__p4d_populate(p4dp, pud_phys, p4dval);
>  		p4d = READ_ONCE(*p4dp);
>  	}
>  	BUG_ON(p4d_bad(p4d));
> @@ -489,11 +502,11 @@ static void __init map_mem(pgd_t *pgdp)
>  	phys_addr_t kernel_start = __pa_symbol(_stext);
>  	phys_addr_t kernel_end = __pa_symbol(__init_begin);
>  	phys_addr_t start, end;
> -	int flags = 0;
> +	int flags = NO_EXEC_MAPPINGS;
>  	u64 i;
>  
>  	if (rodata_full || crash_mem_map || debug_pagealloc_enabled())
> -		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> +		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>  
>  	/*
>  	 * Take care not to create a writable alias for the
> @@ -1462,7 +1475,7 @@ struct range arch_get_mappable_range(void)
>  int arch_add_memory(int nid, u64 start, u64 size,
>  		    struct mhp_params *params)
>  {
> -	int ret, flags = 0;
> +	int ret, flags = NO_EXEC_MAPPINGS;
>  
>  	VM_BUG_ON(!mhp_range_allowed(start, size, true));
>  
> @@ -1472,7 +1485,7 @@ int arch_add_memory(int nid, u64 start, u64 size,
>  	 */
>  	if (rodata_full || debug_pagealloc_enabled() ||
>  	    IS_ENABLED(CONFIG_KFENCE))
> -		flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
> +		flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>  
>  	__create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
>  			     size, params->pgprot, __pgd_pgtable_alloc,
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel