[PATCH v2 2/3] arm64: mm: use XN table mapping attributes for the linear region
Ard Biesheuvel
ardb at kernel.org
Tue Mar 9 12:36:14 GMT 2021
On Tue, 9 Mar 2021 at 06:08, Anshuman Khandual
<anshuman.khandual at arm.com> wrote:
>
>
>
> On 3/8/21 11:45 PM, 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.
>
> While here, wondering if it would be better to validate this assumption
> via a BUILD_BUG_ON() or something similar ?
>
Sure. Something like
BUILD_BUG_ON(pgd_index(_PAGE_END(VA_BITS_MIN) - 1) ==
pgd_index(_PAGE_END(VA_BITS_MIN)));
perhaps?
> >
> > 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 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. While at it, set the UXN
> > attribute as well for all kernel mappings created at boot.
> >
> What happens when FEAT_HPDS is implemented and also being enabled ? Would
> there be any adverse affect here or at least break the assumption that the
> linear mapping page table entries are safer than before ? Hence, it might
> be still better to check FEAT_HPDS feature enablement here, even it it is
> not being used right now.
I would assume that enabling HPDS is only done to free up those bits
for some other purpose, and until that time, I don't think we need to
worry about this, given that the values are just going to be ignored
otherwise.
> > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > Acked-by: Mark Rutland <mark.rutland at arm.com>
> > ---
> > arch/arm64/include/asm/pgtable-hwdef.h | 6 +++++
> > arch/arm64/mm/mmu.c | 27 +++++++++++++++-----
> > 2 files changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
> > index e64e77a345b2..b82575a33f8b 100644
> > --- a/arch/arm64/include/asm/pgtable-hwdef.h
> > +++ b/arch/arm64/include/asm/pgtable-hwdef.h
> > @@ -101,6 +101,8 @@
> > #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_PXN (_AT(p4dval_t, 1) << 59)
> > +#define P4D_TABLE_UXN (_AT(p4dval_t, 1) << 60)
> >
> > /*
> > * Level 1 descriptor (PUD).
> > @@ -110,6 +112,8 @@
> > #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_PXN (_AT(pudval_t, 1) << 59)
> > +#define PUD_TABLE_UXN (_AT(pudval_t, 1) << 60)
> >
> > /*
> > * Level 2 descriptor (PMD).
> > @@ -131,6 +135,8 @@
> > #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_PXN (_AT(pmdval_t, 1) << 59)
> > +#define PMD_TABLE_UXN (_AT(pmdval_t, 1) << 60)
> >
> > /*
> > * 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..9de59fce0450 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)
>
> NO_EXEC_MAPPINGS flag basically selects PXX_TABLE_PXN because PXX_TABLE_UXN
> is now a default, which is already included as a protection flag. Should not
> this be renamed as NO_KERNEL_EXEC_MAPPINGS matching what PXX_TABLE_PXN is
> going to achieve.
>
I don't think renaming this flag is going to make things more clear tbh.
> >
> > 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 | PMD_TABLE_UXN;
> > phys_addr_t pte_phys;
> > +
> > + if (flags & NO_EXEC_MAPPINGS)
> > + pmdval |= PMD_TABLE_PXN;
> > 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 | PUD_TABLE_UXN;
> > phys_addr_t pmd_phys;
> > +
> > + if (flags & NO_EXEC_MAPPINGS)
> > + pudval |= PUD_TABLE_PXN;
> > 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 | P4D_TABLE_UXN;
> > phys_addr_t pud_phys;
> > +
> > + if (flags & NO_EXEC_MAPPINGS)
> > + p4dval |= P4D_TABLE_PXN;
> > 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,
> >
More information about the linux-arm-kernel
mailing list