Re: [PATCH] ArmPkg/ArmMmuLib AARCH64: avoid EL0 accessible mappings

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Sep 22, 2021 at 18:19:54 +0200, Ard Biesheuvel wrote:
> We never run any code at EL0, and so it would seem that any access
> permissions set for EL0 (via the AP[1] attribute in the page tables) are
> irrelevant. We currently set EL0 and EL1 permissions to the same value
> arbitrarily.
> 
> However, this causes problems on hardware like the Apple M1 running the
> hypervisor framework, which enters EL1 with SCTLR_EL1.SPAN enabled,
> which causes the Privileged Access Never (PAN) feature to be enabled on
> any exception taken to EL1, including the IRQ exceptions that handle our
> timer interrupt. When PAN is enabled, EL1 has no access to any mappings
> that are also accessible to EL0, causing the firmware to crash if it
> attempts to access such a mapping.
> 
> Even though it is debatable whether or not SCTLR_EL1.SPAN should be
> disabled at entry or whether the firmware should put all UNKNOWN bits in
> all system registers in a consistent state (which it should), using EL0
> permissions serves no purpose whatsoever so let's fix that regardless.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Acked-by: Leif Lindholm <leif@nuviainc.com>

Do we want to mirror this for (ARMv8) AArch32?

/
    Leif

> ---
>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              | 2 +-
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index 838803aa9b44..56ce84f37e8a 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -283,7 +283,7 @@ EfiAttributeToArmAttribute (
>  
>    // Determine protection attributes
>    if ((EfiAttributes & EFI_MEMORY_RO) != 0) {
> -    ArmAttributes |= TT_AP_RO_RO;
> +    ArmAttributes |= TT_AP_NO_RO;
>    }
>  
>    // Process eXecute Never attribute
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 8c736d25bb80..512801c88638 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -356,7 +356,7 @@ GcdAttributeToPageAttribute (
>    }
>  
>    if ((GcdAttributes & EFI_MEMORY_RO) != 0) {
> -    PageAttributes |= TT_AP_RO_RO;
> +    PageAttributes |= TT_AP_NO_RO;
>    }
>  
>    return PageAttributes | TT_AF;
> @@ -449,7 +449,7 @@ ArmSetMemoryRegionReadOnly (
>    return SetMemoryRegionAttribute (
>             BaseAddress,
>             Length,
> -           TT_AP_RO_RO,
> +           TT_AP_NO_RO,
>             ~TT_ADDRESS_MASK_BLOCK_ENTRY);
>  }
>  
> @@ -462,7 +462,7 @@ ArmClearMemoryRegionReadOnly (
>    return SetMemoryRegionAttribute (
>             BaseAddress,
>             Length,
> -           TT_AP_RW_RW,
> +           TT_AP_NO_RW,
>             ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK));
>  }
>  
> -- 
> 2.30.2
>