Re: [PATCH v2 2/2] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Wed, 25 Mar 2020 at 13:38, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Wed, Mar 25, 2020 at 12:38:46 +0100, Ard Biesheuvel wrote:
> > 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, and
> > 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
> > the page tables that have become orphaned in the process.
> >
> > So let's implement this, by taking the table entry path through the code
> > 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>
>
> With Laszlo's Tested-by:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>
>

Thanks, but it is still not 100% correct. I'll send a v3 shortly.

> > ---
> >  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/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 or page entries at
> >      // the next level. No block mappings are allowed at all at level 0,
> >      // so in that case, we have to recurse unconditionally.
> > +    // If we are changing a table entry and the AttributeClearMask is non-zero,
> > +    // we cannot replace it with a block entry without potentially losing
> > +    // attribute information, so keep the table entry in that case.
> >      //
> > -    if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
> > +    if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
> > +        (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
> >        ASSERT (Level < 3);
> >
> >        if (!IsTableEntry (*Entry, Level)) {
> > @@ -245,6 +249,8 @@ UpdateRegionMappingRecursive (
> >            InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
> >          }
> >
> > +        ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> > +
> >          if (IsBlockEntry (*Entry, Level)) {
> >            //
> >            // We are splitting an existing block entry, so we have to populate
> > @@ -262,8 +268,6 @@ UpdateRegionMappingRecursive (
> >              FreePages (TranslationTable, 1);
> >              return Status;
> >            }
> > -        } else {
> > -          ZeroMem (TranslationTable, EFI_PAGE_SIZE);
> >          }
> >        } else {
> >          TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> > @@ -300,7 +304,20 @@ UpdateRegionMappingRecursive (
> >        EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
> >                                   : TT_TYPE_BLOCK_ENTRY;
> >
> > -      ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> > +      if (IsTableEntry (*Entry, Level)) {
> > +        //
> > +        // We are replacing a table entry with a block entry. This is only
> > +        // possible if we are keeping none of the original attributes.
> > +        // We can free the table entry's page table, and all the ones below
> > +        // it, since we are dropping the only possible reference to it.
> > +        //
> > +        ASSERT (AttributeClearMask == 0);
> > +        TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
> > +        ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
> > +        FreePageTablesRecursive (TranslationTable);
> > +      } else {
> > +        ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
> > +      }
> >      }
> >    }
> >    return EFI_SUCCESS;
> > --
> > 2.17.1
> >