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

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

[Ard Biesheuvel]

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

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

[Leif Lindholm]

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
> 

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

[Alexander Graf]

On 22.09.21 18:19, 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>


I can confirm that this unbreaks HVF guests running on M1 with
SCTLR_EL1.SPAN=0 as reset state.


Tested-by: Alexander Graf <agraf@csgraf.de>

Alex