[edk2] ArmPkg/AArch64Mmu: disable MMU during page table manipulations

Message ID 1460383035-377-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel April 11, 2016, 1:57 p.m.
On ARM, manipulating live page tables is cumbersome since the architecture
mandates the use of break-before-make, i.e., replacing a block entry with
a table entry requires an intermediate step via an invalid entry, or TLB
conflicts may occur.

Since it is not generally feasible to decide in the page table manipulation
routines whether such an invalid entry will result in those routines
themselves to become unavailable, and since UEFI uses an identity mapping
anyway, it is far simpler to just disable the MMU, perform the page table
manipulations, flush the TLBs and re-enable the MMU.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 ++++++++++++++++++++
 1 file changed, 39 insertions(+)

-- 
2.5.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Mark Rutland April 11, 2016, 2:10 p.m. | #1
Hi Ard,

On Mon, Apr 11, 2016 at 03:57:15PM +0200, Ard Biesheuvel wrote:
> On ARM, manipulating live page tables is cumbersome since the architecture

> mandates the use of break-before-make, i.e., replacing a block entry with

> a table entry requires an intermediate step via an invalid entry, or TLB

> conflicts may occur.

> 

> Since it is not generally feasible to decide in the page table manipulation

> routines whether such an invalid entry will result in those routines

> themselves to become unavailable, and since UEFI uses an identity mapping

> anyway, it is far simpler to just disable the MMU, perform the page table

> manipulations, flush the TLBs and re-enable the MMU.


Perhaps I am missing something, but I suspect that this isn't so simple.
More on that below.

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 ++++++++++++++++++++

>  1 file changed, 39 insertions(+)

> 

> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

> index b7d23c6f3286..2f8f05df8aec 100644

> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

> @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress (

>        }

>      } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {

>        // If we are not at the last level then we need to split this BlockEntry

> +      // Since splitting live block entries may cause TLB conflicts, we need to

> +      // enter this function with the MMU disabled and flush the TLBs before

> +      // re-enabling it. This is the responsibility of the caller.

> +      ASSERT (!ArmMmuEnabled ());

> +

>        if (IndexLevel != PageLevel) {

>          // Retrieve the attributes from the block entry

>          Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;

> @@ -453,6 +458,8 @@ SetMemoryAttributes (

>    RETURN_STATUS                Status;

>    ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;

>    UINT64                      *TranslationTable;

> +  UINTN                        SavedInterruptState;

> +  BOOLEAN                      SavedMmuState;

>  

>    MemoryRegion.PhysicalBase = BaseAddress;

>    MemoryRegion.VirtualBase = BaseAddress;

> @@ -461,6 +468,15 @@ SetMemoryAttributes (

>  

>    TranslationTable = ArmGetTTBR0BaseAddress ();

>  

> +  SavedMmuState = ArmMmuEnabled ();

> +  if (SavedMmuState) {

> +    SavedInterruptState = ArmGetInterruptState ();

> +    if (SavedInterruptState) {

> +      ArmDisableInterrupts();

> +    }

> +    ArmDisableMmu();

> +  }


Unless ArmDisableMmu() performs a Clean+Invalidate of all memory in use
by EDK2 (after the MMU is disabled), this is not safe.

For example, the latest version of your stack (which may be dirty in
cache) is not guaranteed to be visible after the MMU is disabled. If any
of that is dirty it may be naturally evicted at any time,
masking/corrupting data written with the MMU off. Any implicit memory
accesses generated by the compiler may go wrong.

A similar problem applies to anything previously mapped with cacheable
attributes.

> +

>    Status = FillTranslationTable (TranslationTable, &MemoryRegion);

>    if (RETURN_ERROR (Status)) {

>      return Status;

> @@ -468,6 +484,12 @@ SetMemoryAttributes (

>  

>    // Invalidate all TLB entries so changes are synced

>    ArmInvalidateTlb ();

> +  if (SavedMmuState) {

> +    ArmEnableMmu();

> +    if (SavedInterruptState) {

> +      ArmEnableInterrupts ();

> +    }

> +  }


Likewise, now everything written with the MMU off is not guaranteed to
be visible to subsequent cacheable accesses, due to stale lines
allocated before the MMU was disabled. That includes the modifications
to the page tables, which may become eventually become visible to
cacheable accesses in an unpredictable fashion (e.g. you might still end
up with the TLBs filling with conflicting entries).

>    return RETURN_SUCCESS;

>  }

> @@ -483,9 +505,20 @@ SetMemoryRegionAttribute (

>  {

>    RETURN_STATUS                Status;

>    UINT64                       *RootTable;

> +  UINTN                        SavedInterruptState;

> +  BOOLEAN                      SavedMmuState;

>  

>    RootTable = ArmGetTTBR0BaseAddress ();

>  

> +  SavedMmuState = ArmMmuEnabled ();

> +  if (SavedMmuState) {

> +    SavedInterruptState = ArmGetInterruptState ();

> +    if (SavedInterruptState) {

> +      ArmDisableInterrupts();

> +    }

> +    ArmDisableMmu();

> +  }

> +

>    Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask);

>    if (RETURN_ERROR (Status)) {

>      return Status;

> @@ -493,6 +526,12 @@ SetMemoryRegionAttribute (

>  

>    // Invalidate all TLB entries so changes are synced

>    ArmInvalidateTlb ();

> +  if (SavedMmuState) {

> +    ArmEnableMmu();

> +    if (SavedInterruptState) {

> +      ArmEnableInterrupts ();

> +    }

> +  }


The same problems apply to both of these.

Thanks,
Mark.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel April 11, 2016, 2:24 p.m. | #2
On 11 April 2016 at 16:10, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,

>

> On Mon, Apr 11, 2016 at 03:57:15PM +0200, Ard Biesheuvel wrote:

>> On ARM, manipulating live page tables is cumbersome since the architecture

>> mandates the use of break-before-make, i.e., replacing a block entry with

>> a table entry requires an intermediate step via an invalid entry, or TLB

>> conflicts may occur.

>>

>> Since it is not generally feasible to decide in the page table manipulation

>> routines whether such an invalid entry will result in those routines

>> themselves to become unavailable, and since UEFI uses an identity mapping

>> anyway, it is far simpler to just disable the MMU, perform the page table

>> manipulations, flush the TLBs and re-enable the MMU.

>

> Perhaps I am missing something, but I suspect that this isn't so simple.


I am afraid it was I who was missing something. Not having a good day ... :-)

> More on that below.

>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c | 39 ++++++++++++++++++++

>>  1 file changed, 39 insertions(+)

>>

>> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

>> index b7d23c6f3286..2f8f05df8aec 100644

>> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

>> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c

>> @@ -286,6 +286,11 @@ GetBlockEntryListFromAddress (

>>        }

>>      } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {

>>        // If we are not at the last level then we need to split this BlockEntry

>> +      // Since splitting live block entries may cause TLB conflicts, we need to

>> +      // enter this function with the MMU disabled and flush the TLBs before

>> +      // re-enabling it. This is the responsibility of the caller.

>> +      ASSERT (!ArmMmuEnabled ());

>> +

>>        if (IndexLevel != PageLevel) {

>>          // Retrieve the attributes from the block entry

>>          Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;

>> @@ -453,6 +458,8 @@ SetMemoryAttributes (

>>    RETURN_STATUS                Status;

>>    ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;

>>    UINT64                      *TranslationTable;

>> +  UINTN                        SavedInterruptState;

>> +  BOOLEAN                      SavedMmuState;

>>

>>    MemoryRegion.PhysicalBase = BaseAddress;

>>    MemoryRegion.VirtualBase = BaseAddress;

>> @@ -461,6 +468,15 @@ SetMemoryAttributes (

>>

>>    TranslationTable = ArmGetTTBR0BaseAddress ();

>>

>> +  SavedMmuState = ArmMmuEnabled ();

>> +  if (SavedMmuState) {

>> +    SavedInterruptState = ArmGetInterruptState ();

>> +    if (SavedInterruptState) {

>> +      ArmDisableInterrupts();

>> +    }

>> +    ArmDisableMmu();

>> +  }

>

> Unless ArmDisableMmu() performs a Clean+Invalidate of all memory in use

> by EDK2 (after the MMU is disabled), this is not safe.

>

> For example, the latest version of your stack (which may be dirty in

> cache) is not guaranteed to be visible after the MMU is disabled. If any

> of that is dirty it may be naturally evicted at any time,

> masking/corrupting data written with the MMU off. Any implicit memory

> accesses generated by the compiler may go wrong.

>

> A similar problem applies to anything previously mapped with cacheable

> attributes.

>


Indeed. I can't believe I didn't spot that. I did have the clarity of
mind to put you on cc though :-)


>> +

>>    Status = FillTranslationTable (TranslationTable, &MemoryRegion);

>>    if (RETURN_ERROR (Status)) {

>>      return Status;

>> @@ -468,6 +484,12 @@ SetMemoryAttributes (

>>

>>    // Invalidate all TLB entries so changes are synced

>>    ArmInvalidateTlb ();

>> +  if (SavedMmuState) {

>> +    ArmEnableMmu();

>> +    if (SavedInterruptState) {

>> +      ArmEnableInterrupts ();

>> +    }

>> +  }

>

> Likewise, now everything written with the MMU off is not guaranteed to

> be visible to subsequent cacheable accesses, due to stale lines

> allocated before the MMU was disabled. That includes the modifications

> to the page tables, which may become eventually become visible to

> cacheable accesses in an unpredictable fashion (e.g. you might still end

> up with the TLBs filling with conflicting entries).

>

>>    return RETURN_SUCCESS;

>>  }

>> @@ -483,9 +505,20 @@ SetMemoryRegionAttribute (

>>  {

>>    RETURN_STATUS                Status;

>>    UINT64                       *RootTable;

>> +  UINTN                        SavedInterruptState;

>> +  BOOLEAN                      SavedMmuState;

>>

>>    RootTable = ArmGetTTBR0BaseAddress ();

>>

>> +  SavedMmuState = ArmMmuEnabled ();

>> +  if (SavedMmuState) {

>> +    SavedInterruptState = ArmGetInterruptState ();

>> +    if (SavedInterruptState) {

>> +      ArmDisableInterrupts();

>> +    }

>> +    ArmDisableMmu();

>> +  }

>> +

>>    Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask);

>>    if (RETURN_ERROR (Status)) {

>>      return Status;

>> @@ -493,6 +526,12 @@ SetMemoryRegionAttribute (

>>

>>    // Invalidate all TLB entries so changes are synced

>>    ArmInvalidateTlb ();

>> +  if (SavedMmuState) {

>> +    ArmEnableMmu();

>> +    if (SavedInterruptState) {

>> +      ArmEnableInterrupts ();

>> +    }

>> +  }

>

> The same problems apply to both of these.

>


OK. So the bottom line is that keeping track of which memory to
clean/invalidate is as intractable as deciding whether
break-before-making a certain block entry is going to make some of
your code unavailable.

Since the code does seem to do the right thing with respect to
populating the table entries before overwriting the block entry, there
is a single point in the splitting code where I could insert a call to
some function that performs the break before make without use of the
stack or any data other than register contents.

Thanks for the quick feedback
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
index b7d23c6f3286..2f8f05df8aec 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Mmu.c
@@ -286,6 +286,11 @@  GetBlockEntryListFromAddress (
       }
     } else if ((*BlockEntry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
       // If we are not at the last level then we need to split this BlockEntry
+      // Since splitting live block entries may cause TLB conflicts, we need to
+      // enter this function with the MMU disabled and flush the TLBs before
+      // re-enabling it. This is the responsibility of the caller.
+      ASSERT (!ArmMmuEnabled ());
+
       if (IndexLevel != PageLevel) {
         // Retrieve the attributes from the block entry
         Attributes = *BlockEntry & TT_ATTRIBUTES_MASK;
@@ -453,6 +458,8 @@  SetMemoryAttributes (
   RETURN_STATUS                Status;
   ARM_MEMORY_REGION_DESCRIPTOR MemoryRegion;
   UINT64                      *TranslationTable;
+  UINTN                        SavedInterruptState;
+  BOOLEAN                      SavedMmuState;
 
   MemoryRegion.PhysicalBase = BaseAddress;
   MemoryRegion.VirtualBase = BaseAddress;
@@ -461,6 +468,15 @@  SetMemoryAttributes (
 
   TranslationTable = ArmGetTTBR0BaseAddress ();
 
+  SavedMmuState = ArmMmuEnabled ();
+  if (SavedMmuState) {
+    SavedInterruptState = ArmGetInterruptState ();
+    if (SavedInterruptState) {
+      ArmDisableInterrupts();
+    }
+    ArmDisableMmu();
+  }
+
   Status = FillTranslationTable (TranslationTable, &MemoryRegion);
   if (RETURN_ERROR (Status)) {
     return Status;
@@ -468,6 +484,12 @@  SetMemoryAttributes (
 
   // Invalidate all TLB entries so changes are synced
   ArmInvalidateTlb ();
+  if (SavedMmuState) {
+    ArmEnableMmu();
+    if (SavedInterruptState) {
+      ArmEnableInterrupts ();
+    }
+  }
 
   return RETURN_SUCCESS;
 }
@@ -483,9 +505,20 @@  SetMemoryRegionAttribute (
 {
   RETURN_STATUS                Status;
   UINT64                       *RootTable;
+  UINTN                        SavedInterruptState;
+  BOOLEAN                      SavedMmuState;
 
   RootTable = ArmGetTTBR0BaseAddress ();
 
+  SavedMmuState = ArmMmuEnabled ();
+  if (SavedMmuState) {
+    SavedInterruptState = ArmGetInterruptState ();
+    if (SavedInterruptState) {
+      ArmDisableInterrupts();
+    }
+    ArmDisableMmu();
+  }
+
   Status = UpdateRegionMapping (RootTable, BaseAddress, Length, Attributes, BlockEntryMask);
   if (RETURN_ERROR (Status)) {
     return Status;
@@ -493,6 +526,12 @@  SetMemoryRegionAttribute (
 
   // Invalidate all TLB entries so changes are synced
   ArmInvalidateTlb ();
+  if (SavedMmuState) {
+    ArmEnableMmu();
+    if (SavedInterruptState) {
+      ArmEnableInterrupts ();
+    }
+  }
 
   return RETURN_SUCCESS;
 }