Re: [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()

[Leif Lindholm <leif.lindholm@linaro.org>]

On Mon, Mar 06, 2017 at 04:11:50PM +0100, Ard Biesheuvel wrote:
> On 6 March 2017 at 15:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Mar 02, 2017 at 10:36:15AM +0000, Ard Biesheuvel wrote:
> >> Enable the use of strict memory permissions on ARM by processing the
> >> EFI_MEMORY_RO and EFI_MEMORY_XP rather than ignoring them. As before,
> >> calls to CpuArchProtocol::SetMemoryAttributes that only set RO/XP
> >> bits will preserve the cacheability attributes. Permissions attributes
> >> are not preserved when setting the memory type only: the way the memory
> >> permission attributes are defined does not allows for that, and so this
> >> situation does not deviate from other architectures.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 151 ++++++++------------
> >>  1 file changed, 62 insertions(+), 89 deletions(-)
> >>
> >> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> index 26b637e7658f..6dd749dadf8b 100644
> >> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> >> @@ -374,50 +374,41 @@ UpdatePageEntries (
> >>
> >>    // EntryMask: bitmask of values to change (1 = change this value, 0 = leave alone)
> >>    // EntryValue: values at bit positions specified by EntryMask
> >> -  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK;
> >> -  EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> >> +  EntryMask = TT_DESCRIPTOR_PAGE_TYPE_MASK | TT_DESCRIPTOR_PAGE_AP_MASK;
> >> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> >> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE_XN;
> >> +  } else {
> >> +    EntryValue = TT_DESCRIPTOR_PAGE_TYPE_PAGE;
> >> +  }
> >> +
> >>    // Although the PI spec is unclear on this the GCD guarantees that only
> >>    // one Attribute bit is set at a time, so we can safely use a switch statement
> >
> > But the switch statement is going away.
> >
> > The change effectively introduces a guaranteed priority of
> > interpretation if the spec is violated. Say something about this order
> > being arbitrarily decided due to PI spce guarantee instead?
> >
> 
> Indeed.

OK, I'll hold back to see what you come up with :)

> >> -  switch (Attributes) {
> >> -    case EFI_MEMORY_UC:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> -      // map to strongly ordered
> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WC:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> -      // map to normal non-cachable
> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WT:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> -      // write through with no-allocate
> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WB:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> -      // write back (with allocate)
> >> -      EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WP:
> >> -    case EFI_MEMORY_XP:
> >> -    case EFI_MEMORY_UCE:
> >> -      // cannot be implemented UEFI definition unclear for ARM
> >> -      // Cause a page fault if these ranges are accessed.
> >> -      EntryValue = TT_DESCRIPTOR_PAGE_TYPE_FAULT;
> >> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting page %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> >> -      break;
> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> >
> > Or should these be "Attributes & Mask" == "ATTRIBUTE"?
> >
> 
> I know this is more idiomatic for EDK2, but the mask *is* the
> attribute in this case, and so the former implies the latter. If the
> mask were

OK ... in that case, could you just drop the != 0?

> (EFI_MEMORY_WB | EFI_MEMORY_WT | etc
> 
> it would make more sense do to the latter I think
> 
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // map to strongly ordered
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // map to normal non-cachable
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // write through with no-allocate
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_MASK;
> >> +    // write back (with allocate)
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> +  }
> >>
> >> -    default:
> >> -      return EFI_UNSUPPORTED;
> >
> > Do we not want a fallback handling for the if-form as well?
> >
> 
> No, and that is actually the point of this patch. SetMemoryAttributes
> may be called without a memory type argument, in which case it only
> modifies permission attributes, and leaves the memory type attributes
> only.

But should there not be a final "else if (Attributes &
CACHE_ATTRIBUTE_MASK)" error path then?

/
    Leif

> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RO_RO;
> >> +  } else {
> >> +    EntryValue |= TT_DESCRIPTOR_PAGE_AP_RW_RW;
> >>    }
> >>
> >>    // Obtain page table base
> >> @@ -520,53 +511,42 @@ UpdateSectionEntries (
> >>    // EntryValue: values at bit positions specified by EntryMask
> >>
> >>    // Make sure we handle a section range that is unmapped
> >> -  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK;
> >> +  EntryMask = TT_DESCRIPTOR_SECTION_TYPE_MASK | TT_DESCRIPTOR_SECTION_XN_MASK |
> >> +              TT_DESCRIPTOR_SECTION_AP_MASK;
> >>    EntryValue = TT_DESCRIPTOR_SECTION_TYPE_SECTION;
> >>
> >>    // Although the PI spec is unclear on this the GCD guarantees that only
> >>    // one Attribute bit is set at a time, so we can safely use a switch statement
> >
> > Repeat of above.
> >
> >> -  switch(Attributes) {
> >> -    case EFI_MEMORY_UC:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> -      // map to strongly ordered
> >> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WC:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> -      // map to normal non-cachable
> >> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WT:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> -      // write through with no-allocate
> >> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WB:
> >> -      // modify cacheability attributes
> >> -      EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> -      // write back (with allocate)
> >> -      EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> -      break;
> >> -
> >> -    case EFI_MEMORY_WP:
> >> -    case EFI_MEMORY_XP:
> >> -    case EFI_MEMORY_RP:
> >> -    case EFI_MEMORY_UCE:
> >> -      // cannot be implemented UEFI definition unclear for ARM
> >> -      // Cause a page fault if these ranges are accessed.
> >> -      EntryValue = TT_DESCRIPTOR_SECTION_TYPE_FAULT;
> >> -      DEBUG ((EFI_D_PAGE, "SetMemoryAttributes(): setting section %lx with unsupported attribute %x will page fault on access\n", BaseAddress, Attributes));
> >> -      break;
> >> +  if ((Attributes & EFI_MEMORY_UC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // map to strongly ordered
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_STRONGLY_ORDERED; // TEX[2:0] = 0, C=0, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WC) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // map to normal non-cachable
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_NON_CACHEABLE; // TEX [2:0]= 001 = 0x2, B=0, C=0
> >> +  } else if ((Attributes & EFI_MEMORY_WT) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // write through with no-allocate
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_THROUGH_NO_ALLOC; // TEX [2:0] = 0, C=1, B=0
> >> +  } else if ((Attributes & EFI_MEMORY_WB) != 0) {
> >> +    // modify cacheability attributes
> >> +    EntryMask |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_MASK;
> >> +    // write back (with allocate)
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
> >> +  }
> >
> > (Again, same questions as above.)
> >
> >>
> >> +  if ((Attributes & EFI_MEMORY_RO) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> >> +  } else {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> >> +  }
> >>
> >> -    default:
> >> -      return EFI_UNSUPPORTED;
> >> +  if ((Attributes & EFI_MEMORY_XP) != 0) {
> >> +    EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> >>    }
> >>
> >>    // obtain page table base
> >> @@ -693,13 +673,6 @@ SetMemoryAttributes (
> >>    UINT64        ChunkLength;
> >>    BOOLEAN       FlushTlbs;
> >>
> >> -  //
> >> -  // Ignore invocations that only modify permission bits
> >> -  //
> >> -  if ((Attributes & EFI_MEMORY_CACHETYPE_MASK) == 0) {
> >> -    return EFI_SUCCESS;
> >> -  }
> >> -
> >>    FlushTlbs = FALSE;
> >>    while (Length > 0) {
> >>      if ((BaseAddress % TT_DESCRIPTOR_SECTION_SIZE == 0) &&
> >> --
> >> 2.7.4
> >>