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

[Anshuman Khandual]

On 3/9/21 6:06 PM, Ard Biesheuvel wrote:
> 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?

Yes, looks good. But probably with a comment explaining how this clear
separation in the kernel page table enables the use of hierarchical
permissions structure.

> 
>>>
>>> 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.

Right, it is definitely safe because the values are going to be ignored
otherwise. But would not it be better to have a small comment explaining
this possible interaction with HPDS ?

> 
>>> 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.

Alright, no problem.