[kernel][arm64][mmu] Fix bug where privileged executable pages are executable from EL0
Previously, was always setting UXN and PXN bits on pages explicitly
mapped as non executable, not taking into account that user (EL0) code
could access a privileged page because UXN wasn't set.
Change the logic to appropriately set PXN and UXN bits on user and
privleged executable pages, appropriately:
user/privileged non-executable page: UXN=1, PXN=1
user executable page: UXN=0, PXN=1
privileged executable page: UXN=1, PXN=0
EL2 mappings for the kernel interpret these bits slightly differently,
so simply map the non executable code as XN=1 (bit 54).
Add kernel unit test to validate that pages mapped this way at least
appear to be in sync with the aspace.Query() api.
Bug: 88451
Change-Id: Icea7a3c5b5effa8b8fe828b3ed6d8e27433caaf0
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/614141
Reviewed-by: Marco Vanotti <mvanotti@google.com>
Commit-Queue: Travis Geiselbrecht <travisg@google.com>
diff --git a/zircon/kernel/arch/arm64/BUILD.gn b/zircon/kernel/arch/arm64/BUILD.gn
index 8f9bd26..73eceeef 100644
--- a/zircon/kernel/arch/arm64/BUILD.gn
+++ b/zircon/kernel/arch/arm64/BUILD.gn
@@ -118,6 +118,7 @@
"mexec.S",
"mexec.cc",
"mmu.cc",
+ "mmu_tests.cc",
"mp.cc",
"perf_mon.cc",
"periphmap.cc",
diff --git a/zircon/kernel/arch/arm64/include/arch/arm64/mmu.h b/zircon/kernel/arch/arm64/include/arch/arm64/mmu.h
index f76caad..09a7ed3 100644
--- a/zircon/kernel/arch/arm64/include/arch/arm64/mmu.h
+++ b/zircon/kernel/arch/arm64/include/arch/arm64/mmu.h
@@ -209,6 +209,7 @@
// Block/Page attrs
#define MMU_PTE_ATTR_RES_SOFTWARE BM(55, 4, 0xf)
+#define MMU_PTE_ATTR_XN BM(54, 1, 1) // for single translation regimes
#define MMU_PTE_ATTR_UXN BM(54, 1, 1)
#define MMU_PTE_ATTR_PXN BM(53, 1, 1)
#define MMU_PTE_ATTR_CONTIGUOUS BM(52, 1, 1)
@@ -342,13 +343,15 @@
#define MMU_VTCR_EL2_FLAGS (MMU_VTCR_EL2_RES1 | MMU_VTCR_EL2_SL0_DEFAULT | MMU_VTCR_FLAGS_GUEST)
#define MMU_PTE_KERNEL_RWX_FLAGS \
- (MMU_PTE_ATTR_AF | \
+ (MMU_PTE_ATTR_UXN | \
+ MMU_PTE_ATTR_AF | \
MMU_PTE_ATTR_SH_INNER_SHAREABLE | \
MMU_PTE_ATTR_NORMAL_MEMORY | \
MMU_PTE_ATTR_AP_P_RW_U_NA)
#define MMU_PTE_KERNEL_RO_FLAGS \
(MMU_PTE_ATTR_UXN | \
+ MMU_PTE_ATTR_PXN | \
MMU_PTE_ATTR_AF | \
MMU_PTE_ATTR_SH_INNER_SHAREABLE | \
MMU_PTE_ATTR_NORMAL_MEMORY | \
diff --git a/zircon/kernel/arch/arm64/mmu.cc b/zircon/kernel/arch/arm64/mmu.cc
index 8f8d3e2..c864fd1 100644
--- a/zircon/kernel/arch/arm64/mmu.cc
+++ b/zircon/kernel/arch/arm64/mmu.cc
@@ -97,7 +97,9 @@
KCOUNTER(vm_mmu_protect_make_execute_pages, "vm.mmu.protect.make_execute_pages")
// Convert user level mmu flags to flags that go in L1 descriptors.
-pte_t mmu_flags_to_s1_pte_attr(uint flags) {
+// Hypervisor flag modifies behavior to work for single translation regimes
+// such as the mapping of kernel pages with ArmAspaceType::kHypervisor in EL2.
+pte_t mmu_flags_to_s1_pte_attr(uint flags, bool hypervisor = false) {
pte_t attr = MMU_PTE_ATTR_AF;
switch (flags & ARCH_MMU_FLAG_CACHE_MASK) {
@@ -132,9 +134,27 @@
break;
}
- if (!(flags & ARCH_MMU_FLAG_PERM_EXECUTE)) {
- attr |= MMU_PTE_ATTR_UXN | MMU_PTE_ATTR_PXN;
+ if (hypervisor) {
+ // For single translation regimes such as the hypervisor pages, only
+ // the XN bit applies.
+ if ((flags & ARCH_MMU_FLAG_PERM_EXECUTE) == 0) {
+ attr |= MMU_PTE_ATTR_XN;
+ }
+ } else {
+ if (flags & ARCH_MMU_FLAG_PERM_EXECUTE) {
+ if (flags & ARCH_MMU_FLAG_PERM_USER) {
+ // User executable page, marked privileged execute never.
+ attr |= MMU_PTE_ATTR_PXN;
+ } else {
+ // Privileged executable page, marked user execute never.
+ attr |= MMU_PTE_ATTR_UXN;
+ }
+ } else {
+ // All non executable pages are marked both privileged and user execute never.
+ attr |= MMU_PTE_ATTR_UXN | MMU_PTE_ATTR_PXN;
+ }
}
+
if (flags & ARCH_MMU_FLAG_NS) {
attr |= MMU_PTE_ATTR_NON_SECURE;
}
@@ -142,7 +162,7 @@
return attr;
}
-uint s1_pte_attr_to_mmu_flags(pte_t pte) {
+uint s1_pte_attr_to_mmu_flags(pte_t pte, bool hypervisor = false) {
uint mmu_flags = 0;
switch (pte & MMU_PTE_ATTR_ATTR_INDEX_MASK) {
case MMU_PTE_ATTR_STRONGLY_ORDERED:
@@ -176,9 +196,28 @@
break;
}
- if (!((pte & MMU_PTE_ATTR_UXN) && (pte & MMU_PTE_ATTR_PXN))) {
- mmu_flags |= ARCH_MMU_FLAG_PERM_EXECUTE;
+ if (hypervisor) {
+ // Single translation regimes such as the hypervisor only support the XN bit.
+ if ((pte & MMU_PTE_ATTR_XN) == 0) {
+ mmu_flags |= ARCH_MMU_FLAG_PERM_EXECUTE;
+ }
+ } else {
+ // Based on whether or not this is a user page, check UXN or PXN bit to determine
+ // if it's an executable page.
+ if (mmu_flags & ARCH_MMU_FLAG_PERM_USER) {
+ if ((pte & MMU_PTE_ATTR_UXN) == 0) {
+ mmu_flags |= ARCH_MMU_FLAG_PERM_EXECUTE;
+ }
+ } else if ((pte & MMU_PTE_ATTR_PXN) == 0) {
+ // Privileged page, check the PXN bit.
+ mmu_flags |= ARCH_MMU_FLAG_PERM_EXECUTE;
+ }
+
+ // TODO: fxbug.dev/88451
+ // Add additional asserts here that the translation table entries are correctly formed
+ // with regards to UXN and PXN bits and possibly other unhandled and/or ambiguous bits.
}
+
if (pte & MMU_PTE_ATTR_NON_SECURE) {
mmu_flags |= ARCH_MMU_FLAG_NS;
}
@@ -414,8 +453,9 @@
switch (type_) {
case ArmAspaceType::kUser:
case ArmAspaceType::kKernel:
- case ArmAspaceType::kHypervisor:
return s1_pte_attr_to_mmu_flags(pte);
+ case ArmAspaceType::kHypervisor:
+ return s1_pte_attr_to_mmu_flags(pte, true);
case ArmAspaceType::kGuest:
return s2_pte_attr_to_mmu_flags(pte);
}
@@ -1217,7 +1257,7 @@
}
case ArmAspaceType::kHypervisor: {
if (attrs) {
- *attrs = mmu_flags_to_s1_pte_attr(mmu_flags);
+ *attrs = mmu_flags_to_s1_pte_attr(mmu_flags, true);
}
*vaddr_base = 0;
*top_size_shift = MMU_IDENT_SIZE_SHIFT;
diff --git a/zircon/kernel/arch/arm64/mmu_tests.cc b/zircon/kernel/arch/arm64/mmu_tests.cc
new file mode 100644
index 0000000..c7b9d3e
--- /dev/null
+++ b/zircon/kernel/arch/arm64/mmu_tests.cc
@@ -0,0 +1,82 @@
+// Copyright 2021 The Fuchsia Authors
+//
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file or at
+// https://opensource.org/licenses/MIT
+
+#include <bits.h>
+#include <lib/unittest/unittest.h>
+#include <lib/zircon-internal/macros.h>
+#include <zircon/errors.h>
+#include <zircon/types.h>
+
+#include <arch/arm64/mmu.h>
+#include <arch/aspace.h>
+#include <vm/arch_vm_aspace.h>
+#include <vm/physmap.h>
+#include <vm/pmm.h>
+#include <vm/vm_aspace.h>
+
+namespace {
+
+constexpr size_t kTestAspaceSize = (1UL << 48);
+constexpr size_t kTestVirtualAddress = (1UL << 30); // arbitrary address
+
+bool arm64_test_perms() {
+ BEGIN_TEST;
+
+ ArmArchVmAspace aspace(0, kTestAspaceSize, ArmAspaceType::kUser);
+ EXPECT_EQ(ZX_OK, aspace.Init());
+
+ auto map_query_test = [&](uint mmu_perms) -> bool {
+ paddr_t pa = 0;
+ size_t count;
+ vm_page_t* vm_page;
+ pmm_alloc_page(/*alloc_flags=*/0, &vm_page, &pa);
+ EXPECT_EQ(ZX_OK, aspace.Map(kTestVirtualAddress, &pa, 1, mmu_perms,
+ ArchVmAspaceInterface::ExistingEntryAction::Error, &count));
+ EXPECT_EQ(1u, count);
+
+ paddr_t query_pa;
+ uint query_flags;
+ EXPECT_EQ(ZX_OK, aspace.Query(kTestVirtualAddress, &query_pa, &query_flags));
+ EXPECT_EQ(pa, query_pa);
+ EXPECT_EQ(mmu_perms, query_flags);
+
+ // FUTURE ENHANCEMENT: use a private api to read the terminal page table entry
+ // and validate the bits that were set.
+
+ EXPECT_EQ(ZX_OK, aspace.Unmap(kTestVirtualAddress, 1, &count));
+ EXPECT_EQ(1u, count);
+
+ return all_ok;
+ };
+
+ // map nox page, query to see that X bit isn't set
+ map_query_test(ARCH_MMU_FLAG_PERM_READ);
+ map_query_test(ARCH_MMU_FLAG_PERM_READ | ARCH_MMU_FLAG_PERM_WRITE);
+ map_query_test(ARCH_MMU_FLAG_PERM_READ | ARCH_MMU_FLAG_PERM_EXECUTE);
+ map_query_test(ARCH_MMU_FLAG_PERM_READ | ARCH_MMU_FLAG_PERM_WRITE | ARCH_MMU_FLAG_PERM_EXECUTE);
+
+ // map X page, query to see that X bit is set
+ map_query_test(ARCH_MMU_FLAG_PERM_USER | ARCH_MMU_FLAG_PERM_READ);
+ map_query_test(ARCH_MMU_FLAG_PERM_USER | ARCH_MMU_FLAG_PERM_READ | ARCH_MMU_FLAG_PERM_WRITE);
+ map_query_test(ARCH_MMU_FLAG_PERM_USER | ARCH_MMU_FLAG_PERM_READ | ARCH_MMU_FLAG_PERM_EXECUTE);
+ map_query_test(ARCH_MMU_FLAG_PERM_USER | ARCH_MMU_FLAG_PERM_READ | ARCH_MMU_FLAG_PERM_WRITE |
+ ARCH_MMU_FLAG_PERM_EXECUTE);
+
+ // TODO: fxbug.dev/88451 Add a more comprehensive test that reads back the page table entries
+ // and all the translation tables leading up to it to make sure the permission bits are set
+ // properly. Requires plumbing through some code to the ArmArchVmAspace to return a copy of all
+ // the levels of the translation tables and terminal entry.
+
+ EXPECT_EQ(ZX_OK, aspace.Destroy());
+
+ END_TEST;
+}
+
+} // anonymous namespace
+
+UNITTEST_START_TESTCASE(arm64_mmu_tests)
+UNITTEST("perms", arm64_test_perms)
+UNITTEST_END_TESTCASE(arm64_mmu_tests, "arm64_mmu", "arm64 mmu tests")
diff --git a/zircon/kernel/arch/arm64/start.S b/zircon/kernel/arch/arm64/start.S
index 20510c1..0f914ea 100644
--- a/zircon/kernel/arch/arm64/start.S
+++ b/zircon/kernel/arch/arm64/start.S
@@ -182,7 +182,7 @@
adr_global x2, __code_start
adr_global x3, _end
sub x3, x3, x2
- mov x4, MMU_PTE_KERNEL_RWX_FLAGS
+ movlit x4, MMU_PTE_KERNEL_RWX_FLAGS
bl arm64_boot_map
/* Prepare tt_trampoline page table.
