public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
Date: Mon, 6 Mar 2017 16:11:50 +0100	[thread overview]
Message-ID: <CAKv+Gu8pVyitCPbOYY2s6eXKvziVz-gsFH7Up8dNT8hb6B4gKg@mail.gmail.com> (raw)
In-Reply-To: <20170306144834.GV16034@bivouac.eciton.net>

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.

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

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

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


  reply	other threads:[~2017-03-06 15:11 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-02 10:36 [PATCH v2 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
2017-03-06 14:12   ` Leif Lindholm
2017-03-06 14:55     ` Ard Biesheuvel
2017-03-06 16:24       ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance Ard Biesheuvel
2017-03-06 14:27   ` Leif Lindholm
2017-03-06 15:06     ` Ard Biesheuvel
2017-03-02 10:36 ` [PATCH v2 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() Ard Biesheuvel
2017-03-06 14:48   ` Leif Lindholm
2017-03-06 15:11     ` Ard Biesheuvel [this message]
2017-03-06 16:40       ` Leif Lindholm
2017-03-02 10:36 ` [PATCH v2 4/4] ArmVirtPkg: enable PE/COFF image and memory protection for ARM platforms Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKv+Gu8pVyitCPbOYY2s6eXKvziVz-gsFH7Up8dNT8hb6B4gKg@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox