Better page protections for arm64

The head.text section is intended to be run at early bootup before any of the regular kernel mappings have been setup. Parts of head.text may be freed back into the buddy allocator due to TEXT_OFFSET so for security requirements this memory must not be executable. The suspend/resume/hotplug code path requires some of these head.S functions to run however which means they need to be executable. Support these conflicting requirements by moving the few head.text functions that need to be executable to the text section which has the appropriate page table permissions.

Tested-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
v6: Dropped the latehead.text bit and just moved everything to the regular text section

---
 arch/arm64/kernel/head.S | 407 ++++++++++++++++++++++++-----------------------
 1 file changed, 210 insertions(+), 197 deletions(-)

arch/arm64/kernel/head.S +0 -0

index 10f5cc0..4b63d7a 100644

Quoted 11 lines · Laura Abbott

The head.text section is intended to be run at early bootup before any of the regular kernel mappings have been setup. Parts of head.text may be freed back into the buddy allocator due to TEXT_OFFSET so for security requirements this memory must not be executable. The suspend/resume/hotplug code path requires some of these head.S functions to run however which means they need to be executable. Support these conflicting requirements by moving the few head.text functions that need to be executable to the text section which has the appropriate page table permissions.

Quoted 2 lines

Tested-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

Other than a minor nit below this looks good to me, and I'm not seeing any issues with boot, hotplug, or idle, so:

Reviewed-by Mark Rutland <[email protected]>

Tested-by Mark Rutland <[email protected]>

Thanks for putting this together!

Quoted 16 lines

---
v6: Dropped the latehead.text bit and just moved everything to the regular text section

---
 arch/arm64/kernel/head.S | 407 ++++++++++++++++++++++++-----------------------
 1 file changed, 210 insertions(+), 197 deletions(-)

arch/arm64/kernel/head.S +0 -1

index 10f5cc0..4b63d7a 100644
@@ -238,7 +238,13 @@ ENTRY(stext)
        mov     x0, x22
        bl      lookup_processor_type
        mov     x23, x0                         // x23=current cpu_table
-       cbz     x23, __error_p                  // invalid processor (x23=0)?

arch/arm64/kernel/head.S ENTRY(stext) · line 244

Quoted 4 lines

arch/arm64/kernel/head.S +4 -0

+       /*
+        *      __error_p may end up out of range for cbz if text areas
+        *      are aligned up to section sizes
+        */

Nit: We don't need to align the comment text (we don't for the comment a few lines later, or any others in this file). It would be nice to keep that consistent.

From testing on Juno with defconfig, all of the early patches were

When kernel text is marked as read only, it cannot be modified directly. Use a fixmap to modify the text instead in a similar manner to x86 and arm.

Reviewed-by Kees Cook <[email protected]>

Tested-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/include/asm/fixmap.h |  1 +
 arch/arm64/include/asm/insn.h   |  2 ++
 arch/arm64/kernel/insn.c        | 72 +++++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/jump_label.c  |  2 +-
 4 files changed, 73 insertions(+), 4 deletions(-)

arch/arm64/include/asm/fixmap.h +1 -0

index db26a2f2..2cd4b0d 100644
@@ -48,6 +48,7 @@ enum fixed_addresses {
 
 	FIX_BTMAP_END = __end_of_permanent_fixed_addresses,
 	FIX_BTMAP_BEGIN = FIX_BTMAP_END + TOTAL_FIX_BTMAPS - 1,
+	FIX_TEXT_POKE0,
 	__end_of_fixed_addresses
 };
 

arch/arm64/include/asm/insn.h +2 -0

index 56a9e63..f66853b 100644
@@ -282,6 +282,7 @@ bool aarch64_insn_is_nop(u32 insn);
 
 int aarch64_insn_read(void *addr, u32 *insnp);
 int aarch64_insn_write(void *addr, u32 insn);
+int aarch64_insn_write_early(void *addr, u32 insn);
 enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
 u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 				  u32 insn, u64 imm);

@@ -352,6 +353,7 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
 
 int aarch64_insn_patch_text_nosync(void *addr, u32 insn);
+int __aarch64_insn_patch_text_nosync(void *addr, u32 insn, bool early);
 int aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt);
 int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
 #endif /* __ASSEMBLY__ */

arch/arm64/kernel/insn.c +69 -3

index 8cd27fe..b2cad38 100644
@@ -19,12 +19,15 @@
 #include <linux/bitops.h>
 #include <linux/compiler.h>
 #include <linux/kernel.h>
+#include <linux/mm.h>
 #include <linux/smp.h>
+#include <linux/spinlock.h>
 #include <linux/stop_machine.h>
 #include <linux/uaccess.h>
 
 #include <asm/cacheflush.h>
 #include <asm/debug-monitors.h>
+#include <asm/fixmap.h>
 #include <asm/insn.h>
 
 #define AARCH64_INSN_SF_BIT	BIT(31)

@@ -72,6 +75,36 @@ bool __kprobes aarch64_insn_is_nop(u32 insn)
 	}
 }
 
+static DEFINE_SPINLOCK(patch_lock);
+
+static void __kprobes *patch_map(void *addr, int fixmap, unsigned long *flags)
+{
+	unsigned long uintaddr = (uintptr_t) addr;
+	bool module = !core_kernel_text(uintaddr);
+	struct page *page;
+
+	if (module && IS_ENABLED(CONFIG_DEBUG_SET_MODULE_RONX))
+		page = vmalloc_to_page(addr);
+	else if (!module && IS_ENABLED(CONFIG_DEBUG_RODATA))
+		page = virt_to_page(addr);
+	else
+		return addr;
+
+	if (flags)
+		spin_lock_irqsave(&patch_lock, *flags);
+
+	set_fixmap(fixmap, page_to_phys(page));
+
+	return (void *) (__fix_to_virt(fixmap) + (uintaddr & ~PAGE_MASK));
+}
+
+static void __kprobes patch_unmap(int fixmap, unsigned long *flags)
+{
+	clear_fixmap(fixmap);
+
+	if (flags)
+		spin_unlock_irqrestore(&patch_lock, *flags);
+}
 /*
  * In ARMv8-A, A64 instructions have a fixed length of 32 bits and are always
  * little-endian.

@@ -88,10 +121,34 @@ int __kprobes aarch64_insn_read(void *addr, u32 *insnp)
 	return ret;
 }
 
+static int __kprobes __aarch64_insn_write(void *addr, u32 insn, bool patch)
+{
+	void *waddr = addr;
+	unsigned long flags;
+	int ret;
+
+	if (patch)
+		waddr = patch_map(addr, FIX_TEXT_POKE0, &flags);
+
+	ret = probe_kernel_write(waddr, &insn, AARCH64_INSN_SIZE);
+
+	if (waddr != addr)
+		patch_unmap(FIX_TEXT_POKE0, &flags);
+
+	return ret;
+}
+
 int __kprobes aarch64_insn_write(void *addr, u32 insn)
 {
 	insn = cpu_to_le32(insn);
-	return probe_kernel_write(addr, &insn, AARCH64_INSN_SIZE);
+	return __aarch64_insn_write(addr, insn, true);
+}
+
+int __kprobes aarch64_insn_write_early(void *addr, u32 insn)
+{
+	insn = cpu_to_le32(insn);
+	return __aarch64_insn_write(addr, insn, false);
+
 }
 
 static bool __kprobes __aarch64_insn_hotpatch_safe(u32 insn)

@@ -124,7 +181,7 @@ bool __kprobes aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn)
 	       __aarch64_insn_hotpatch_safe(new_insn);
 }
 
-int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
+int __kprobes __aarch64_insn_patch_text_nosync(void *addr, u32 insn, bool early)
 {
 	u32 *tp = addr;
 	int ret;

@@ -133,7 +190,11 @@ int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
 	if ((uintptr_t)tp & 0x3)
 		return -EINVAL;
 
-	ret = aarch64_insn_write(tp, insn);
+	if (early)
+		ret = aarch64_insn_write_early(tp, insn);
+	else
+		ret = aarch64_insn_write(tp, insn);
+
 	if (ret == 0)
 		flush_icache_range((uintptr_t)tp,
 				   (uintptr_t)tp + AARCH64_INSN_SIZE);

@@ -141,6 +202,11 @@ int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
 	return ret;
 }
 
+int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
+{
+	return __aarch64_insn_patch_text_nosync(addr, insn, false);
+}
+
 struct aarch64_insn_patch {
 	void		**text_addrs;
 	u32		*new_insns;

arch/arm64/kernel/jump_label.c +1 -1

index 263a166..9ac30bb 100644
@@ -38,7 +38,7 @@ static void __arch_jump_label_transform(struct jump_entry *entry,
 	}
 
 	if (is_static)
-		aarch64_insn_patch_text_nosync(addr, insn);
+		__aarch64_insn_patch_text_nosync(addr, insn, true);
 	else
 		aarch64_insn_patch_text(&addr, &insn, 1);
 }

Signature · 2 lines

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Hi Laura,

It looks like "early" means before we've set up the strict page permissions, though as far as I can tell the fixmap will already be available before we do any patching. Could we not always use the fixmap for patching? Then we only need the patch_{map,unmap} additions, and not the changes to distinguish the early cases.

From testing on Juno with defconfig, all of the early patches were
avoidable NOP -> NOP changes as part of static key initialisation, which I think we can skip similarly to x86 (I'll send a patch shortly). All other patches were not early and went via the fixmap.

Even with the avoidable NOP -> NOP patching I did not see a noticeable boot time difference from forcing the use of the fixmap.

Thanks,
Mark.

Quoted 13 lines · Mark Rutland

Hi Laura,

It looks like "early" means before we've set up the strict page permissions, though as far as I can tell the fixmap will already be available before we do any patching. Could we not always use the fixmap for patching? Then we only need the patch_{map,unmap} additions, and not the changes to distinguish the early cases.

From testing on Juno with defconfig, all of the early patches were
avoidable NOP -> NOP changes as part of static key initialisation, which I think we can skip similarly to x86 (I'll send a patch shortly). All other patches were not early and went via the fixmap.

Quoted 3 lines

Even with the avoidable NOP -> NOP patching I did not see a noticeable boot time difference from forcing the use of the fixmap.

I was basing it off of the arm version which needed the early option. If arm64 doesn't need it I'll drop it.

Quoted 18 lines · Laura Abbott

Quoted 16 lines · Mark Rutland

Hi Laura,

It looks like "early" means before we've set up the strict page permissions, though as far as I can tell the fixmap will already be available before we do any patching. Could we not always use the fixmap for patching? Then we only need the patch_{map,unmap} additions, and not the changes to distinguish the early cases.

From testing on Juno with defconfig, all of the early patches were
avoidable NOP -> NOP changes as part of static key initialisation, which I think we can skip similarly to x86 (I'll send a patch shortly). All other patches were not early and went via the fixmap.

Even with the avoidable NOP -> NOP patching I did not see a noticeable boot time difference from forcing the use of the fixmap.

Quoted 2 lines

I was basing it off of the arm version which needed the early option. If arm64 doesn't need it I'll drop it.

Given that it only determines whether or not to use the fixmap, and we can always use the fixmap, I think we can drop it.

ioremap is not guaranteed to return memory with proper execution permissions. Introduce ioremap_exec which will ensure that permission bits are set as expected for EFI code sections.

Tested-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
 arch/arm64/include/asm/io.h      |  1 +
 arch/arm64/include/asm/pgtable.h |  1 +
 arch/arm64/kernel/efi.c          | 12 +++++++++++-
 arch/arm64/mm/ioremap.c          | 11 +++++++++++
 4 files changed, 24 insertions(+), 1 deletion(-)

arch/arm64/include/asm/io.h +1 -0

index 79f1d519..7dd8465 100644
@@ -230,6 +230,7 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
 extern void __iomem *__ioremap(phys_addr_t phys_addr, size_t size, pgprot_t prot);
 extern void __iounmap(volatile void __iomem *addr);
 extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
+extern void __iomem *ioremap_exec(phys_addr_t phys_addr, size_t size);
 
 #define ioremap(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
 #define ioremap_nocache(addr, size)	__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))

arch/arm64/include/asm/pgtable.h +1 -0

index 41a43bf..9b1d9d0 100644
@@ -65,6 +65,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
 #define PROT_DEVICE_nGnRE	(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_ATTRINDX(MT_DEVICE_nGnRE))
 #define PROT_NORMAL_NC		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_ATTRINDX(MT_NORMAL_NC))
 #define PROT_NORMAL		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_ATTRINDX(MT_NORMAL))
+#define PROT_NORMAL_EXEC	(PROT_DEFAULT | PTE_UXN | PTE_ATTRINDX(MT_NORMAL))
 
 #define PROT_SECT_DEVICE_nGnRE	(PROT_SECT_DEFAULT | PMD_SECT_PXN | PMD_SECT_UXN | PMD_ATTRINDX(MT_DEVICE_nGnRE))
 #define PROT_SECT_NORMAL	(PROT_SECT_DEFAULT | PMD_SECT_PXN | PMD_SECT_UXN | PMD_ATTRINDX(MT_NORMAL))

arch/arm64/kernel/efi.c +8 -0

index 95c49eb..9e41f95 100644
@@ -47,6 +47,14 @@ static int __init is_normal_ram(efi_memory_desc_t *md)
 	return 0;
 }
 
+static int __init is_code(efi_memory_desc_t *md)
+{
+	if (md->attribute & EFI_RUNTIME_SERVICES_CODE)
+		return 1;
+	return 0;
+}
+
+
 static void __init efi_setup_idmap(void)
 {
 	struct memblock_region *r;

arch/arm64/mm/ioremap.c +6 -0

+
+	return __ioremap_caller(phys_addr, size, __pgprot(PROT_NORMAL_EXEC),
+				__builtin_return_address(0));
+}
+EXPORT_SYMBOL(ioremap_exec);
+
 /*
  * Must be called after early_fixmap_init
  */

Signature · 2 lines

Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Hi Laura,

Quoted 58 lines · Laura Abbott

ioremap is not guaranteed to return memory with proper execution permissions. Introduce ioremap_exec which will ensure that permission bits are set as expected for EFI code sections.

Tested-by Kees Cook <[email protected]>

Signed-off-by Laura Abbott <[email protected]>

---
  arch/arm64/include/asm/io.h      |  1 +
  arch/arm64/include/asm/pgtable.h |  1 +
  arch/arm64/kernel/efi.c          | 12 +++++++++++-
  arch/arm64/mm/ioremap.c          | 11 +++++++++++
  4 files changed, 24 insertions(+), 1 deletion(-)

arch/arm64/include/asm/io.h +1 -0

index 79f1d519..7dd8465 100644
@@ -230,6 +230,7 @@ extern void __memset_io(volatile void __iomem *, int, size_t);
  extern void __iomem *__ioremap(phys_addr_t phys_addr, size_t size, pgprot_t prot);
  extern void __iounmap(volatile void __iomem *addr);
  extern void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size);
+extern void __iomem *ioremap_exec(phys_addr_t phys_addr, size_t size);
 
  #define ioremap(addr, size)		__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))
  #define ioremap_nocache(addr, size)	__ioremap((addr), (size), __pgprot(PROT_DEVICE_nGnRE))

arch/arm64/include/asm/pgtable.h +1 -0

index 41a43bf..9b1d9d0 100644
@@ -65,6 +65,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
  #define PROT_DEVICE_nGnRE	(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_ATTRINDX(MT_DEVICE_nGnRE))
  #define PROT_NORMAL_NC		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_ATTRINDX(MT_NORMAL_NC))
  #define PROT_NORMAL		(PROT_DEFAULT | PTE_PXN | PTE_UXN | PTE_ATTRINDX(MT_NORMAL))
+#define PROT_NORMAL_EXEC	(PROT_DEFAULT | PTE_UXN | PTE_ATTRINDX(MT_NORMAL))
 
  #define PROT_SECT_DEVICE_nGnRE	(PROT_SECT_DEFAULT | PMD_SECT_PXN | PMD_SECT_UXN | PMD_ATTRINDX(MT_DEVICE_nGnRE))
  #define PROT_SECT_NORMAL	(PROT_SECT_DEFAULT | PMD_SECT_PXN | PMD_SECT_UXN | PMD_ATTRINDX(MT_NORMAL))

arch/arm64/kernel/efi.c +8 -0

index 95c49eb..9e41f95 100644
@@ -47,6 +47,14 @@ static int __init is_normal_ram(efi_memory_desc_t *md)
  	return 0;
  }
 
+static int __init is_code(efi_memory_desc_t *md)
+{
+	if (md->attribute & EFI_RUNTIME_SERVICES_CODE)
+		return 1;
+	return 0;
+}
+
+
  static void __init efi_setup_idmap(void)
  {
  	struct memblock_region *r;

arch/arm64/kernel/efi.c static int __init remap_region(efi_memory_desc_t *md, void **new) · line 354

Quoted 10 lines

arch/arm64/kernel/efi.c +3 -1

@@ -338,7 +346,9 @@ static int __init remap_region(efi_memory_desc_t *md, void **new)
  	memrange_efi_to_native(&paddr, &npages);
  	size = npages << PAGE_SHIFT;
 
-	if (is_normal_ram(md))
+	if (is_code(md))
+		vaddr = (__force u64)ioremap_exec(paddr, size);
+	else if (is_normal_ram(md))
  		vaddr = (__force u64)ioremap_cache(paddr, size);
  	else
  		vaddr = (__force u64)ioremap(paddr, size);

All of the above looks fine to me.

Quoted 8 lines

arch/arm64/mm/ioremap.c +0 -0

index cbb99c8..b998441 100644
@@ -103,6 +103,17 @@ void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
  }
  EXPORT_SYMBOL(ioremap_cache);
 

arch/arm64/mm/ioremap.c void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size) · line 110

Quoted 5 lines

arch/arm64/mm/ioremap.c +5 -0

+void __iomem *ioremap_exec(phys_addr_t phys_addr, size_t size)
+{
+	/* For normal memory we already have a cacheable mapping. */
+	if (pfn_valid(__phys_to_pfn(phys_addr)))
+		return (void __iomem *)__phys_to_virt(phys_addr);

Is this guaranteed to be executable in all cases once the stricter page permissions are in force?

Thanks,
Mark.

arch/arm64/kernel/efi.c +8 -0

index 95c49eb..9e41f95 100644
@@ -47,6 +47,14 @@ static int __init is_normal_ram(efi_memory_desc_t *md)
 	return 0;
 }
 
+static int __init is_code(efi_memory_desc_t *md)
+{
+	if (md->attribute & EFI_RUNTIME_SERVICES_CODE)
+		return 1;
+	return 0;
+}
+
+
 static void __init efi_setup_idmap(void)
 {
 	struct memblock_region *r;

arch/arm64/mm/ioremap.c +0 -0

index cbb99c8..b998441 100644
@@ -103,6 +103,17 @@ void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
 }

Quoted 23 lines · Laura Abbott

arch/arm64/kernel/efi.c +8 -0

index 95c49eb..9e41f95 100644
@@ -47,6 +47,14 @@ static int __init is_normal_ram(efi_memory_desc_t *md)
      return 0;
 }
 
+static int __init is_code(efi_memory_desc_t *md)
+{
+     if (md->attribute & EFI_RUNTIME_SERVICES_CODE)
+             return 1;
+     return 0;
+}
+
+
 static void __init efi_setup_idmap(void)
 {
      struct memblock_region *r;

@@ -338,7 +346,9 @@ static int __init remap_region(efi_memory_desc_t *md, void **new)
      memrange_efi_to_native(&paddr, &npages);
      size = npages << PAGE_SHIFT;
 

arch/arm64/kernel/efi.c static int __init remap_region(efi_memory_desc_t *md, void **new) · line 354

Quoted 7 lines

arch/arm64/kernel/efi.c +3 -1

-     if (is_normal_ram(md))
+     if (is_code(md))
+             vaddr = (__force u64)ioremap_exec(paddr, size);
+     else if (is_normal_ram(md))
              vaddr = (__force u64)ioremap_cache(paddr, size);
      else
              vaddr = (__force u64)ioremap(paddr, size);

What I don't understand is that currently the above remap_region() function only uses ioremap_cache() and ignores any code attributes. So we end up with PROT_NORMAL which is non-executable.

Is the current remap_region() function broken or we don't expect it to be called with any EFI_RUNTIME_SERVICES_CODE region?

Quoted 8 lines

arch/arm64/mm/ioremap.c +0 -0

index cbb99c8..b998441 100644
@@ -103,6 +103,17 @@ void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size)
 }
 EXPORT_SYMBOL(ioremap_cache);
 

arch/arm64/mm/ioremap.c void __iomem *ioremap_cache(phys_addr_t phys_addr, size_t size) · line 113

Quoted 8 lines

arch/arm64/mm/ioremap.c +8 -0

+void __iomem *ioremap_exec(phys_addr_t phys_addr, size_t size)
+{
+     /* For normal memory we already have a cacheable mapping. */
+     if (pfn_valid(__phys_to_pfn(phys_addr)))
+             return (void __iomem *)__phys_to_virt(phys_addr);
+
+     return __ioremap_caller(phys_addr, size, __pgprot(PROT_NORMAL_EXEC),
+                             __builtin_return_address(0));

In addition to what I said above, do we expect ioremap_exec() to be called on non-pfn_valid() pages?

arch/arm64/mm/mmu.c +1 -0

@@ -26,6 +26,7 @@
 #include <linux/memblock.h>
 #include <linux/fs.h>
 #include <linux/io.h>
+#include <linux/stop_machine.h>
 
 #include <asm/cputype.h>
 #include <asm/fixmap.h>

arch/arm64/mm/mmu.c +13 -0

+{
+       pte_t *pte, *start_pte;
+       unsigned long pfn;
+       int i = 0;
+
+       start_pte = pte = alloc(PTRS_PER_PTE*sizeof(pte_t));
+       BUG_ON(!pte);
+
+       pfn = pmd_pfn(*pmd);
+
+       do {
+               /*
+                * Need to have the least restrictive permissions available

arch/arm64/mm/mmu.c +4 -0

+static inline bool use_1G_block(unsigned long addr, unsigned long next,
+                       unsigned long phys, pgprot_t sect_prot,
+                       pgprot_t pte_prot)
+{

arch/arm64/mm/mmu.c +24 -0

+#ifdef CONFIG_DEBUG_RODATA
+static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
+{
+       /*
+        * Set up the executable regions using the existing section mappings
+        * for now. This will get more fine grained later once all memory
+        * is mapped
+        */
+       unsigned long kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
+       unsigned long kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);
+
+       if (end < kernel_x_start) {
+               create_mapping(start, __phys_to_virt(start),
+                       end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+       } else if (start >= kernel_x_end) {
+               create_mapping(start, __phys_to_virt(start),
+                       end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+       } else {
+               if (start < kernel_x_start)
+                       create_mapping(start, __phys_to_virt(start),
+                               kernel_x_start - start,
+                               PROT_SECT_NORMAL,
+                               PAGE_KERNEL);
+               create_mapping(kernel_x_start,

arch/arm64/mm/mmu.c +17 -0

+struct flush_addr {
+       unsigned long start;
+       unsigned long end;
+};
+
+static int __flush_mappings(void *val)
+{
+       struct flush_addr *data = val;
+
+       flush_tlb_kernel_range(data->start, data->end);
+       return 0;
+}
+
+static void adjust_mem(unsigned long vstart, unsigned long vend,
+                       phys_addr_t phys,
+                       pgprot_t sect_prot, pgprot_t pte_prot)
+{

Quoted 14 lines · Laura Abbott

arch/arm64/mm/mmu.c +1 -0

@@ -26,6 +26,7 @@
  #include <linux/memblock.h>
  #include <linux/fs.h>
  #include <linux/io.h>
+#include <linux/stop_machine.h>
 
  #include <asm/cputype.h>
  #include <asm/fixmap.h>

@@ -137,17 +138,50 @@ static void __init *early_alloc(unsigned long sz)
         return ptr;
  }
 

arch/arm64/mm/mmu.c static void __init *early_alloc(unsigned long sz) · line 145

Quoted 6 lines

arch/arm64/mm/mmu.c +5 -1

-static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
+/*
+ * remap a PMD into pages
+ */
+static noinline void split_pmd(pmd_t *pmd,
+                               void *(*alloc)(unsigned long size))

Please drop the inline/noinline annotations, just let the compiler do the work.

Quoted 18 lines

arch/arm64/mm/mmu.c +18 -0

+{
+       pte_t *pte, *start_pte;
+       unsigned long pfn;
+       int i = 0;
+
+       start_pte = pte = alloc(PTRS_PER_PTE*sizeof(pte_t));
+       BUG_ON(!pte);
+
+       pfn = pmd_pfn(*pmd);
+
+       do {
+               /*
+                * Need to have the least restrictive permissions available
+                * permissions will be fixed up later
+                */
+               set_pte(pte, pfn_pte(pfn, PAGE_KERNEL_EXEC));
+               pfn++;
+       } while (pte++, i++, i < PTRS_PER_PTE);

arch/arm64/mm/mmu.c static void __init *early_alloc(unsigned long sz) · line 168

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+
+       __pmd_populate(pmd, __pa(start_pte), PMD_TYPE_TABLE);
+       flush_tlb_all();
+}

For consistency, can we not have split_pmd similar to split_pud (i.e. avoid allocation in split_pmd, just populate with the given pmd)?

Quoted 9 lines

arch/arm64/mm/mmu.c +4 -1

+
+static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
                                   unsigned long end, unsigned long pfn,
-                                 pgprot_t prot)
+                                 pgprot_t prot,
+                                 void *(*alloc)(unsigned long size))
  {
         pte_t *pte;
 

arch/arm64/mm/mmu.c static void __init *early_alloc(unsigned long sz) · line 179

Quoted 4 lines

arch/arm64/mm/mmu.c +2 -1

         if (pmd_none(*pmd)) {
-               pte = early_alloc(PTRS_PER_PTE * sizeof(pte_t));
+               pte = alloc(PTRS_PER_PTE * sizeof(pte_t));
+               BUG_ON(!pte);

I wonder whether we should put the BUG_ON in the alloc functions to keep the code simpler.

arch/arm64/mm/mmu.c static void __init *early_alloc(unsigned long sz) · line 184

Quoted 6 lines

arch/arm64/mm/mmu.c +3 -1

                 __pmd_populate(pmd, __pa(pte), PMD_TYPE_TABLE);
         }
-       BUG_ON(pmd_bad(*pmd));
+
+       if (pmd_bad(*pmd))
+               split_pmd(pmd, alloc);

I would use pmd_sect(*pmd) instead, it gives an indication on why it needs splitting.

Quoted 9 lines

arch/arm64/mm/mmu.c +9 -0

+static inline bool use_1G_block(unsigned long addr, unsigned long next,
+                       unsigned long phys, pgprot_t sect_prot,
+                       pgprot_t pte_prot)
+{
+       if (PAGE_SHIFT != 12)
+               return false;
+
+       if (((addr | next | phys) & ~PUD_MASK) != 0)
+               return false;

arch/arm64/mm/mmu.c static void __init alloc_init_pmd(pud_t *pud, unsigned long addr, · line 264

Quoted 5 lines

arch/arm64/mm/mmu.c +5 -0

+
+       /*
+        * The assumption here is that if the memory is anything other
+        * than normal we should not be using a block type
+        */

Why? I know the original code had a !map_io check but I don't remember why we actually need this limitation.

Quoted 24 lines

arch/arm64/mm/mmu.c +24 -0

+#ifdef CONFIG_DEBUG_RODATA
+static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
+{
+       /*
+        * Set up the executable regions using the existing section mappings
+        * for now. This will get more fine grained later once all memory
+        * is mapped
+        */
+       unsigned long kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
+       unsigned long kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);
+
+       if (end < kernel_x_start) {
+               create_mapping(start, __phys_to_virt(start),
+                       end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+       } else if (start >= kernel_x_end) {
+               create_mapping(start, __phys_to_virt(start),
+                       end - start, PROT_SECT_NORMAL, PAGE_KERNEL);
+       } else {
+               if (start < kernel_x_start)
+                       create_mapping(start, __phys_to_virt(start),
+                               kernel_x_start - start,
+                               PROT_SECT_NORMAL,
+                               PAGE_KERNEL);
+               create_mapping(kernel_x_start,

arch/arm64/mm/mmu.c static void __init __create_mapping(pgd_t *pgd, phys_addr_t phys, · line 426

Quoted 10 lines

arch/arm64/mm/mmu.c +10 -0

+                               __phys_to_virt(kernel_x_start),
+                               kernel_x_end - kernel_x_start,
+                               PROT_SECT_NORMAL_EXEC, PAGE_KERNEL_EXEC);
+               if (kernel_x_end < end)
+                       create_mapping(kernel_x_end,
+                               __phys_to_virt(kernel_x_end),
+                               end - kernel_x_end,
+                               PROT_SECT_NORMAL,
+                               PAGE_KERNEL);
+       }

Do we care about the init section? It will get freed eventually (though I guess we could spot early bugs modifying such code).

Anyway, can you not do something similar here for the rest of the kernel text to avoid re-adjusting the page attributes later?

Quoted 23 lines

arch/arm64/mm/mmu.c +23 -0

+struct flush_addr {
+       unsigned long start;
+       unsigned long end;
+};
+
+static int __flush_mappings(void *val)
+{
+       struct flush_addr *data = val;
+
+       flush_tlb_kernel_range(data->start, data->end);
+       return 0;
+}
+
+static void adjust_mem(unsigned long vstart, unsigned long vend,
+                       phys_addr_t phys,
+                       pgprot_t sect_prot, pgprot_t pte_prot)
+{
+       struct flush_addr f;
+
+       create_mapping_late(phys, vstart, vend - vstart,
+                       sect_prot, pte_prot);
+
+       if (!IS_ALIGNED(vstart, SECTION_SIZE) || !IS_ALIGNED(vend, SECTION_SIZE)) {

arch/arm64/mm/mmu.c static void __init map_mem(void) · line 550

Quoted 4 lines

arch/arm64/mm/mmu.c +4 -0

+               f.start = vstart;
+               f.end = vend;
+               stop_machine(__flush_mappings, &f, NULL);
+       }

Maybe this was discussed on earlier versions of this patch but why do you need stop_machine() here? A TLB flush would be broadcast by hardware automatically to all the other CPUs.

I need to go again through this patch and see if there is a better option than overloading the alloc_init_*() functions for adjusting ptes (or maybe avoid adjusting altogether if we create them with the right attributes from the beginning).

Hi,

This is v6 of the series to add stricter page protections for arm64. The goal is to have text be RO/NX and everything else be RW/NX. The biggest change here is the addition of ioremap_exec for EFI services. The series did its job and caught EFI services trying to use regular RAM for code purposes.

Hi,

This is v6 of the series to add stricter page protections for arm64. The goal is to have text be RO/NX and everything else be RW/NX. The biggest change here is the addition of ioremap_exec for EFI services. The series did its job and caught EFI services trying to use regular RAM for code purposes.

Laura Abbott (8):
arm64: Treat handle_arch_irq as a function pointer
arm64: Switch to adrp for loading the stub vectors
arm64: Move cpu_resume into the text section
arm64: Move some head.text functions to executable section
arm64: Factor out fixmap initialiation from ioremap
arm64: use fixmap for text patching when text is RO
arm64: efi: Use ioremap_exec for code sections
arm64: add better page protections to arm64

 arch/arm64/Kconfig.debug            |  23 ++
 arch/arm64/include/asm/cacheflush.h |   7 +
 arch/arm64/include/asm/fixmap.h     |   8 +-
 arch/arm64/include/asm/insn.h       |   2 +
 arch/arm64/include/asm/io.h         |   1 +
 arch/arm64/include/asm/irq.h        |   1 -
 arch/arm64/include/asm/pgtable.h    |   1 +
 arch/arm64/kernel/efi.c             |  12 +-
 arch/arm64/kernel/entry.S           |   6 +-
 arch/arm64/kernel/head.S            | 410 +++++++++++++++++++-----------------
 arch/arm64/kernel/insn.c            |  72 ++++++-
 arch/arm64/kernel/irq.c             |   2 +
 arch/arm64/kernel/jump_label.c      |   2 +-
 arch/arm64/kernel/setup.c           |   1 +
 arch/arm64/kernel/sleep.S           |  36 +---
 arch/arm64/kernel/suspend.c         |   4 +-
 arch/arm64/kernel/vmlinux.lds.S     |  18 +-
 arch/arm64/mm/init.c                |   1 +
 arch/arm64/mm/ioremap.c             |  99 ++-------
 arch/arm64/mm/mm.h                  |   2 +
 arch/arm64/mm/mmu.c                 | 405 +++++++++++++++++++++++++++++++----
 21 files changed, 740 insertions(+), 373 deletions(-)

--
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project