External email: Use caution opening links or att= achments

Currently, depending on the size of the region being (re)mapped, the
page table manipulation code may replace a table entry with a block entry= ,
even if the existing table entry uses different mapping attributes to
= describe different parts of the region it covers. This is undesirable, an= d
instead, we should avoid doing so unless we are disregarding the original=
attributes anyway. And if we make such a replacement, we should free all<= br> the page tables that have become orphaned in the process.

So let's implement this, by taking the table entry path through the code<= br> for block sized regions if a table entry already exists, and the clear mask is set (which means we are preserving attributes from the existing mapping). And when we do replace a table entry with a block entry, free all the pages that are no longer referenced.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++= 3;+++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Li= brary/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6f6ef5b05fbc..488156e69057 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -223,8 +223,12 @@ UpdateRegionMappingRecursive (
     // than a block, and recurse to create the block= =20or page entries at
     // the next level. No block mappings are allowed= =20at all at level 0,
     // so in that case, we have to recurse unconditi= onally.
+    // If we are changing a table entry and the Attri= buteClearMask is non-zero,
+    // we cannot replace it with a block entry withou= t potentially losing
+    // attribute information, so keep the table entry= =20in that case.
     //
-    if (Level =3D=3D 0 || ((RegionStart | BlockEnd) &= =20BlockMask) !=3D 0) {
+    if (Level =3D=3D 0 || ((RegionStart | BlockEnd) &= amp; BlockMask) !=3D 0 ||
+        (IsTableEntry (*Entry, Le= vel) && AttributeClearMask !=3D 0)) {
       ASSERT (Level < 3);

       if (!IsTableEntry (*Entry, Level)) {=
@@ -245,6 +249,8 @@ UpdateRegionMappingRecursive (
           InvalidateDa= taCacheRange (TranslationTable, EFI_PAGE_SIZE);
         }

+        ZeroMem (TranslationTable= , EFI_PAGE_SIZE);
+
         if (IsBlockEntry (*Entry= , Level)) {
           //
           // We are sp= litting an existing block entry, so we have to populate
@@ -262,8 +268,6 @@ UpdateRegionMappingRecursive (
             = FreePages (TranslationTable, 1);
             = return Status;
           }
-        } else {
-          ZeroMem (Translat= ionTable, EFI_PAGE_SIZE);
         }
       } else {
         TranslationTable =3D (VO= ID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
@@ -300,7 +304,20 @@ UpdateRegionMappingRecursive (
       EntryValue |=3D (Level =3D=3D 3) ? T= T_TYPE_BLOCK_ENTRY_LEVEL3
            &= nbsp;           &n= bsp;         : TT_TYPE_BLOCK_ENTR= Y;

-      ReplaceTableEntry (Entry, EntryValue, Reg= ionStart, FALSE);
+      if (IsTableEntry (*Entry, Level)) { +        //
+        // We are replacing a tab= le entry with a block entry. This is only
+        // possible if we are kee= ping none of the original attributes.
+        // We can free the table = entry's page table, and all the ones below
+        // it, since we are dropp= ing the only possible reference to it.
+        //
+        ASSERT (AttributeClearMas= k =3D=3D 0);
+        TranslationTable =3D (VOI= D *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
+        ReplaceTableEntry (Entry,= =20EntryValue, RegionStart, TRUE);
+        FreePageTablesRecursive (= TranslationTable);
+      } else {
+        ReplaceTableEntry (Entry,= =20EntryValue, RegionStart, FALSE);
+      }
     }
   }
   return EFI_SUCCESS;
--
2.17.1