From: Leif Lindholm <leif.lindholm@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: edk2-devel@lists.01.org, lersek@redhat.com
Subject: Re: [PATCH v3 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes()
Date: Tue, 7 Mar 2017 07:57:51 +0000 [thread overview]
Message-ID: <20170307075751.GD16034@bivouac.eciton.net> (raw)
In-Reply-To: <1488821535-14795-4-git-send-email-ard.biesheuvel@linaro.org>
On Mon, Mar 06, 2017 at 06:32:14PM +0100, 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>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 178 ++++++++++----------
> 1 file changed, 86 insertions(+), 92 deletions(-)
>
> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> index a2993cf16a35..d3c307f48317 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -19,6 +19,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Library/MemoryAllocationLib.h>
> #include "CpuDxe.h"
>
> +#define CACHE_ATTRIBUTE_MASK (EFI_MEMORY_UC | \
> + EFI_MEMORY_WC | \
> + EFI_MEMORY_WT | \
> + EFI_MEMORY_WB | \
> + EFI_MEMORY_UCE | \
> + EFI_MEMORY_WP)
> +
> // First Level Descriptors
> typedef UINT32 ARM_FIRST_LEVEL_DESCRIPTOR;
>
> @@ -374,50 +381,48 @@ 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;
> - // 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
> - 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;
> + 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;
> + }
>
> - 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;
> + // Although the PI spec is unclear on this, the GCD guarantees that only
> + // one Attribute bit is set at a time, so the order of the conditionals below
> + // is irrelevant. If no memory attribute is specified, we preserve whatever
> + // memory type is set in the page tables, and update the permission attributes
> + // only.
> + if (Attributes & 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
> + } else if (Attributes & 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
> + } else if (Attributes & 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
> + } else if (Attributes & 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
> + } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
> + // catch unsupported memory type attributes
> + ASSERT (FALSE);
> + return EFI_UNSUPPORTED;
> + }
>
> - default:
> - return EFI_UNSUPPORTED;
> + 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 +525,49 @@ 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
> - 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;
> + // Although the PI spec is unclear on this, the GCD guarantees that only
> + // one Attribute bit is set at a time, so the order of the conditionals below
> + // is irrelevant. If no memory attribute is specified, we preserve whatever
> + // memory type is set in the page tables, and update the permission attributes
> + // only.
> + if (Attributes & 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
> + } else if (Attributes & 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
> + } else if (Attributes & 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
> + } else if (Attributes & 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
> + } else if (Attributes & CACHE_ATTRIBUTE_MASK) {
> + // catch unsupported memory type attributes
> + ASSERT (FALSE);
> + return EFI_UNSUPPORTED;
> + }
>
> + if (Attributes & EFI_MEMORY_RO) {
> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
> + } else {
> + EntryValue |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
> + }
>
> - default:
> - return EFI_UNSUPPORTED;
> + if (Attributes & EFI_MEMORY_XP) {
> + EntryValue |= TT_DESCRIPTOR_SECTION_XN_MASK;
> }
>
> // obtain page table base
> @@ -697,13 +698,6 @@ SetMemoryAttributes (
> return EFI_SUCCESS;
> }
>
> - //
> - // 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
>
next prev parent reply other threads:[~2017-03-07 7:57 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-06 17:32 [PATCH v3 0/4] ArmPkg, ArmVirtpkg ARM: enable strict memory protection Ard Biesheuvel
2017-03-06 17:32 ` [PATCH v3 1/4] ArmPkg/CpuDxe ARM: avoid splitting page table sections unnecessarily Ard Biesheuvel
2017-03-06 17:32 ` [PATCH v3 2/4] ArmPkg/CpuDxe ARM: avoid unnecessary cache/TLB maintenance Ard Biesheuvel
2017-03-06 17:32 ` [PATCH v3 3/4] ArmPkg/CpuDxe ARM: honour RO/XP attributes in SetMemoryAttributes() Ard Biesheuvel
2017-03-07 7:57 ` Leif Lindholm [this message]
2017-03-06 17:32 ` [PATCH v3 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=20170307075751.GD16034@bivouac.eciton.net \
--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