Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1261410 > unrolled thread

[GIT PULL] x86/mm changes for v4.4

Started byIngo Molnar <mingo@kernel.org>
First post2015-11-03 12:20 +0100
Last post2015-11-10 21:20 +0100
Articles 20 on this page of 26 — 8 participants

Back to article view | Back to linux.kernel

Contents

  [GIT PULL] x86/mm changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-03 12:20 +0100
    Re: [GIT PULL] x86/mm changes for v4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-04 20:30 +0100
      Re: [GIT PULL] x86/mm changes for v4.4 Dave Jones <davej@codemonkey.org.uk> - 2015-11-05 00:40 +0100
        Re: [GIT PULL] x86/mm changes for v4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-05 02:40 +0100
          Re: [GIT PULL] x86/mm changes for v4.4 Dave Jones <davej@codemonkey.org.uk> - 2015-11-05 03:20 +0100
            Re: [GIT PULL] x86/mm changes for v4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-05 22:30 +0100
              Re: [GIT PULL] x86/mm changes for v4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-05 22:40 +0100
                Re: [GIT PULL] x86/mm changes for v4.4 Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-06 12:40 +0100
                  Re: [GIT PULL] x86/mm changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-07 08:10 +0100
                    Re: [GIT PULL] x86/mm changes for v4.4 Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-07 11:10 +0100
              Re: [GIT PULL] x86/mm changes for v4.4 Linus Torvalds <torvalds@linux-foundation.org> - 2015-11-05 23:10 +0100
                Re: [GIT PULL] x86/mm changes for v4.4 Borislav Petkov <bp@alien8.de> - 2015-11-05 23:30 +0100
              Re: [GIT PULL] x86/mm changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-06 08:00 +0100
                Re: [GIT PULL] x86/mm changes for v4.4 Andy Lutomirski <luto@amacapital.net> - 2015-11-06 08:10 +0100
                  Re: [GIT PULL] x86/mm changes for v4.4 Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-06 14:10 +0100
                    Re: [GIT PULL] x86/mm changes for v4.4 Borislav Petkov <bp@alien8.de> - 2015-11-06 14:30 +0100
                  Re: [GIT PULL] x86/mm changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-07 08:10 +0100
                Re: [GIT PULL] x86/mm changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-06 08:50 +0100
                Re: [GIT PULL] x86/mm changes for v4.4 Matt Fleming <matt@codeblueprint.co.uk> - 2015-11-06 13:40 +0100
                  Re: [GIT PULL] x86/mm changes for v4.4 Ingo Molnar <mingo@kernel.org> - 2015-11-07 08:10 +0100
                    Re: [GIT PULL] x86/mm changes for v4.4 Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-11-07 08:40 +0100
                      Re: [GIT PULL] x86/mm changes for v4.4 Kees Cook <keescook@chromium.org> - 2015-11-08 08:00 +0100
                        Re: [GIT PULL] x86/mm changes for v4.4 Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-11-08 09:00 +0100
                          Re: [GIT PULL] x86/mm changes for v4.4 Kees Cook <keescook@chromium.org> - 2015-11-09 22:10 +0100
                            Re: [GIT PULL] x86/mm changes for v4.4 Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2015-11-10 08:10 +0100
                              Re: [GIT PULL] x86/mm changes for v4.4 Kees Cook <keescook@chromium.org> - 2015-11-10 21:20 +0100

Page 1 of 2  [1] 2  Next page →

#1261410 — [GIT PULL] x86/mm changes for v4.4

FromIngo Molnar <mingo@kernel.org>
Date2015-11-03 12:20 +0100
Subject[GIT PULL] x86/mm changes for v4.4
Message-ID<qqHN8-1ZB-23@gated-at.bofh.it>
Linus,

Please pull the latest x86-mm-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86-mm-for-linus

   # HEAD: e1a58320a38dfa72be48a0f1a3a92273663ba6db x86/mm: Warn on W^X mappings

The main changes are: continued PAT work by Toshi Kani, plus a new boot time 
warning about insecure RWX kernel mappings, by Stephen Smalley.

The new CONFIG_DEBUG_WX=y warning is marked default-y if CONFIG_DEBUG_RODATA=y is 
already eanbled, as a special exception, as these bugs are hard to notice and this 
check already found several live bugs.

 Thanks,

	Ingo

------------------>
Stephen Smalley (1):
      x86/mm: Warn on W^X mappings

Toshi Kani (11):
      x86/vdso32: Define PGTABLE_LEVELS to 32bit VDSO
      x86/asm: Move PUD_PAGE macros to page_types.h
      x86/asm: Add pud/pmd mask interfaces to handle large PAT bit
      x86/asm: Fix pud/pmd interfaces to handle large PAT bit
      x86/asm: Add pud_pgprot() and pmd_pgprot()
      x86/mm: Fix page table dump to show PAT bit
      x86/mm: Fix slow_virt_to_phys() to handle large PAT bit
      x86/mm: Fix gup_huge_p?d() to handle large PAT bit
      x86/mm: Fix try_preserve_large_page() to handle large PAT bit
      x86/mm: Fix __split_large_page() to handle large PAT bit
      x86/mm: Fix no-change case in try_preserve_large_page()


 arch/x86/Kconfig.debug                      | 36 ++++++++++++-
 arch/x86/entry/vdso/vdso32/vclock_gettime.c |  2 +
 arch/x86/include/asm/page_64_types.h        |  3 --
 arch/x86/include/asm/page_types.h           |  3 ++
 arch/x86/include/asm/pgtable.h              | 25 ++++++---
 arch/x86/include/asm/pgtable_types.h        | 40 ++++++++++++--
 arch/x86/mm/Makefile                        |  2 +-
 arch/x86/mm/dump_pagetables.c               | 81 ++++++++++++++++++++++-------
 arch/x86/mm/gup.c                           | 18 +++----
 arch/x86/mm/init_32.c                       |  2 +
 arch/x86/mm/init_64.c                       |  2 +
 arch/x86/mm/pageattr.c                      | 79 +++++++++++++++++-----------
 12 files changed, 219 insertions(+), 74 deletions(-)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index d8c0d3266173..3e0baf726eef 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -65,10 +65,14 @@ config EARLY_PRINTK_EFI
 	  This is useful for kernel debugging when your machine crashes very
 	  early before the console code is initialized.
 
+config X86_PTDUMP_CORE
+	def_bool n
+
 config X86_PTDUMP
 	bool "Export kernel pagetable layout to userspace via debugfs"
 	depends on DEBUG_KERNEL
 	select DEBUG_FS
+	select X86_PTDUMP_CORE
 	---help---
 	  Say Y here if you want to show the kernel pagetable layout in a
 	  debugfs file. This information is only useful for kernel developers
@@ -79,7 +83,8 @@ config X86_PTDUMP
 
 config EFI_PGT_DUMP
 	bool "Dump the EFI pagetable"
-	depends on EFI && X86_PTDUMP
+	depends on EFI
+	select X86_PTDUMP_CORE
 	---help---
 	  Enable this if you want to dump the EFI page table before
 	  enabling virtual mode. This can be used to debug miscellaneous
@@ -105,6 +110,35 @@ config DEBUG_RODATA_TEST
 	  feature as well as for the change_page_attr() infrastructure.
 	  If in doubt, say "N"
 
+config DEBUG_WX
+	bool "Warn on W+X mappings at boot"
+	depends on DEBUG_RODATA
+	default y
+	select X86_PTDUMP_CORE
+	---help---
+	  Generate a warning if any W+X mappings are found at boot.
+
+	  This is useful for discovering cases where the kernel is leaving
+	  W+X mappings after applying NX, as such mappings are a security risk.
+
+	  Look for a message in dmesg output like this:
+
+	    x86/mm: Checked W+X mappings: passed, no W+X pages found.
+
+	  or like this, if the check failed:
+
+	    x86/mm: Checked W+X mappings: FAILED, <N> W+X pages found.
+
+	  Note that even if the check fails, your kernel is possibly
+	  still fine, as W+X mappings are not a security hole in
+	  themselves, what they do is that they make the exploitation
+	  of other unfixed kernel bugs easier.
+
+	  There is no runtime or memory usage effect of this option
+	  once the kernel has booted up - it's a one time check.
+
+	  If in doubt, say "Y".
+
 config DEBUG_SET_MODULE_RONX
 	bool "Set loadable kernel module data as NX and text as RO"
 	depends on MODULES
diff --git a/arch/x86/entry/vdso/vdso32/vclock_gettime.c b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
index 175cc72c0f68..87a86e017f0e 100644
--- a/arch/x86/entry/vdso/vdso32/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vdso32/vclock_gettime.c
@@ -14,11 +14,13 @@
  */
 #undef CONFIG_64BIT
 #undef CONFIG_X86_64
+#undef CONFIG_PGTABLE_LEVELS
 #undef CONFIG_ILLEGAL_POINTER_VALUE
 #undef CONFIG_SPARSEMEM_VMEMMAP
 #undef CONFIG_NR_CPUS
 
 #define CONFIG_X86_32 1
+#define CONFIG_PGTABLE_LEVELS 2
 #define CONFIG_PAGE_OFFSET 0
 #define CONFIG_ILLEGAL_POINTER_VALUE 0
 #define CONFIG_NR_CPUS 1
diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 4edd53b79a81..4928cf0d5af0 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -26,9 +26,6 @@
 #define MCE_STACK 4
 #define N_EXCEPTION_STACKS 4  /* hw limit: 7 */
 
-#define PUD_PAGE_SIZE		(_AC(1, UL) << PUD_SHIFT)
-#define PUD_PAGE_MASK		(~(PUD_PAGE_SIZE-1))
-
 /*
  * Set __PAGE_OFFSET to the most negative possible address +
  * PGDIR_SIZE*16 (pgd slot 272).  The gap is to allow a space for a
diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
index c7c712f2648b..c5b7fb2774d0 100644
--- a/arch/x86/include/asm/page_types.h
+++ b/arch/x86/include/asm/page_types.h
@@ -20,6 +20,9 @@
 #define PMD_PAGE_SIZE		(_AC(1, UL) << PMD_SHIFT)
 #define PMD_PAGE_MASK		(~(PMD_PAGE_SIZE-1))
 
+#define PUD_PAGE_SIZE		(_AC(1, UL) << PUD_SHIFT)
+#define PUD_PAGE_MASK		(~(PUD_PAGE_SIZE-1))
+
 #define HPAGE_SHIFT		PMD_SHIFT
 #define HPAGE_SIZE		(_AC(1,UL) << HPAGE_SHIFT)
 #define HPAGE_MASK		(~(HPAGE_SIZE - 1))
diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 867da5bbb4a3..c0b41f111a9a 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -19,6 +19,13 @@
 #include <asm/x86_init.h>
 
 void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
+void ptdump_walk_pgd_level_checkwx(void);
+
+#ifdef CONFIG_DEBUG_WX
+#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
+#else
+#define debug_checkwx() do { } while (0)
+#endif
 
 /*
  * ZERO_PAGE is a global shared page that is always zero: used
@@ -142,12 +149,12 @@ static inline unsigned long pte_pfn(pte_t pte)
 
 static inline unsigned long pmd_pfn(pmd_t pmd)
 {
-	return (pmd_val(pmd) & PTE_PFN_MASK) >> PAGE_SHIFT;
+	return (pmd_val(pmd) & pmd_pfn_mask(pmd)) >> PAGE_SHIFT;
 }
 
 static inline unsigned long pud_pfn(pud_t pud)
 {
-	return (pud_val(pud) & PTE_PFN_MASK) >> PAGE_SHIFT;
+	return (pud_val(pud) & pud_pfn_mask(pud)) >> PAGE_SHIFT;
 }
 
 #define pte_page(pte)	pfn_to_page(pte_pfn(pte))
@@ -379,7 +386,9 @@ static inline pgprot_t pgprot_modify(pgprot_t oldprot, pgprot_t newprot)
 	return __pgprot(preservebits | addbits);
 }
 
-#define pte_pgprot(x) __pgprot(pte_flags(x) & PTE_FLAGS_MASK)
+#define pte_pgprot(x) __pgprot(pte_flags(x))
+#define pmd_pgprot(x) __pgprot(pmd_flags(x))
+#define pud_pgprot(x) __pgprot(pud_flags(x))
 
 #define canon_pgprot(p) __pgprot(massage_pgprot(p))
 
@@ -502,14 +511,15 @@ static inline int pmd_none(pmd_t pmd)
 
 static inline unsigned long pmd_page_vaddr(pmd_t pmd)
 {
-	return (unsigned long)__va(pmd_val(pmd) & PTE_PFN_MASK);
+	return (unsigned long)__va(pmd_val(pmd) & pmd_pfn_mask(pmd));
 }
 
 /*
  * Currently stuck as a macro due to indirect forward reference to
  * linux/mmzone.h's __section_mem_map_addr() definition:
  */
-#define pmd_page(pmd)	pfn_to_page((pmd_val(pmd) & PTE_PFN_MASK) >> PAGE_SHIFT)
+#define pmd_page(pmd)		\
+	pfn_to_page((pmd_val(pmd) & pmd_pfn_mask(pmd)) >> PAGE_SHIFT)
 
 /*
  * the pmd page can be thought of an array like this: pmd_t[PTRS_PER_PMD]
@@ -570,14 +580,15 @@ static inline int pud_present(pud_t pud)
 
 static inline unsigned long pud_page_vaddr(pud_t pud)
 {
-	return (unsigned long)__va((unsigned long)pud_val(pud) & PTE_PFN_MASK);
+	return (unsigned long)__va(pud_val(pud) & pud_pfn_mask(pud));
 }
 
 /*
  * Currently stuck as a macro due to indirect forward reference to
  * linux/mmzone.h's __section_mem_map_addr() definition:
  */
-#define pud_page(pud)		pfn_to_page(pud_val(pud) >> PAGE_SHIFT)
+#define pud_page(pud)		\
+	pfn_to_page((pud_val(pud) & pud_pfn_mask(pud)) >> PAGE_SHIFT)
 
 /* Find an entry in the second-level page table.. */
 static inline pmd_t *pmd_offset(pud_t *pud, unsigned long address)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index 13f310bfc09a..dd5b0aa9dd2f 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -209,10 +209,10 @@ enum page_cache_mode {
 
 #include <linux/types.h>
 
-/* PTE_PFN_MASK extracts the PFN from a (pte|pmd|pud|pgd)val_t */
+/* Extracts the PFN from a (pte|pmd|pud|pgd)val_t of a 4KB page */
 #define PTE_PFN_MASK		((pteval_t)PHYSICAL_PAGE_MASK)
 
-/* PTE_FLAGS_MASK extracts the flags from a (pte|pmd|pud|pgd)val_t */
+/* Extracts the flags from a (pte|pmd|pud|pgd)val_t of a 4KB page */
 #define PTE_FLAGS_MASK		(~PTE_PFN_MASK)
 
 typedef struct pgprot { pgprotval_t pgprot; } pgprot_t;
@@ -276,14 +276,46 @@ static inline pmdval_t native_pmd_val(pmd_t pmd)
 }
 #endif
 
+static inline pudval_t pud_pfn_mask(pud_t pud)
+{
+	if (native_pud_val(pud) & _PAGE_PSE)
+		return PUD_PAGE_MASK & PHYSICAL_PAGE_MASK;
+	else
+		return PTE_PFN_MASK;
+}
+
+static inline pudval_t pud_flags_mask(pud_t pud)
+{
+	if (native_pud_val(pud) & _PAGE_PSE)
+		return ~(PUD_PAGE_MASK & (pudval_t)PHYSICAL_PAGE_MASK);
+	else
+		return ~PTE_PFN_MASK;
+}
+
 static inline pudval_t pud_flags(pud_t pud)
 {
-	return native_pud_val(pud) & PTE_FLAGS_MASK;
+	return native_pud_val(pud) & pud_flags_mask(pud);
+}
+
+static inline pmdval_t pmd_pfn_mask(pmd_t pmd)
+{
+	if (native_pmd_val(pmd) & _PAGE_PSE)
+		return PMD_PAGE_MASK & PHYSICAL_PAGE_MASK;
+	else
+		return PTE_PFN_MASK;
+}
+
+static inline pmdval_t pmd_flags_mask(pmd_t pmd)
+{
+	if (native_pmd_val(pmd) & _PAGE_PSE)
+		return ~(PMD_PAGE_MASK & (pmdval_t)PHYSICAL_PAGE_MASK);
+	else
+		return ~PTE_PFN_MASK;
 }
 
 static inline pmdval_t pmd_flags(pmd_t pmd)
 {
-	return native_pmd_val(pmd) & PTE_FLAGS_MASK;
+	return native_pmd_val(pmd) & pmd_flags_mask(pmd);
 }
 
 static inline pte_t native_make_pte(pteval_t val)
diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
index a482d105172b..65c47fda26fc 100644
--- a/arch/x86/mm/Makefile
+++ b/arch/x86/mm/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_SMP)		+= tlb.o
 obj-$(CONFIG_X86_32)		+= pgtable_32.o iomap_32.o
 
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
-obj-$(CONFIG_X86_PTDUMP)	+= dump_pagetables.o
+obj-$(CONFIG_X86_PTDUMP_CORE)	+= dump_pagetables.o
 
 obj-$(CONFIG_HIGHMEM)		+= highmem_32.o
 
diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
index f0cedf3395af..1bf417e9cc13 100644
--- a/arch/x86/mm/dump_pagetables.c
+++ b/arch/x86/mm/dump_pagetables.c
@@ -32,6 +32,8 @@ struct pg_state {
 	const struct addr_marker *marker;
 	unsigned long lines;
 	bool to_dmesg;
+	bool check_wx;
+	unsigned long wx_pages;
 };
 
 struct addr_marker {
@@ -155,7 +157,7 @@ static void printk_prot(struct seq_file *m, pgprot_t prot, int level, bool dmsg)
 			pt_dump_cont_printf(m, dmsg, "    ");
 		if ((level == 4 && pr & _PAGE_PAT) ||
 		    ((level == 3 || level == 2) && pr & _PAGE_PAT_LARGE))
-			pt_dump_cont_printf(m, dmsg, "pat ");
+			pt_dump_cont_printf(m, dmsg, "PAT ");
 		else
 			pt_dump_cont_printf(m, dmsg, "    ");
 		if (pr & _PAGE_GLOBAL)
@@ -198,8 +200,8 @@ static void note_page(struct seq_file *m, struct pg_state *st,
 	 * we have now. "break" is either changing perms, levels or
 	 * address space marker.
 	 */
-	prot = pgprot_val(new_prot) & PTE_FLAGS_MASK;
-	cur = pgprot_val(st->current_prot) & PTE_FLAGS_MASK;
+	prot = pgprot_val(new_prot);
+	cur = pgprot_val(st->current_prot);
 
 	if (!st->level) {
 		/* First entry */
@@ -214,6 +216,16 @@ static void note_page(struct seq_file *m, struct pg_state *st,
 		const char *unit = units;
 		unsigned long delta;
 		int width = sizeof(unsigned long) * 2;
+		pgprotval_t pr = pgprot_val(st->current_prot);
+
+		if (st->check_wx && (pr & _PAGE_RW) && !(pr & _PAGE_NX)) {
+			WARN_ONCE(1,
+				  "x86/mm: Found insecure W+X mapping at address %p/%pS\n",
+				  (void *)st->start_address,
+				  (void *)st->start_address);
+			st->wx_pages += (st->current_address -
+					 st->start_address) / PAGE_SIZE;
+		}
 
 		/*
 		 * Now print the actual finished series
@@ -269,13 +281,13 @@ static void walk_pte_level(struct seq_file *m, struct pg_state *st, pmd_t addr,
 {
 	int i;
 	pte_t *start;
+	pgprotval_t prot;
 
 	start = (pte_t *) pmd_page_vaddr(addr);
 	for (i = 0; i < PTRS_PER_PTE; i++) {
-		pgprot_t prot = pte_pgprot(*start);
-
+		prot = pte_flags(*start);
 		st->current_address = normalize_addr(P + i * PTE_LEVEL_MULT);
-		note_page(m, st, prot, 4);
+		note_page(m, st, __pgprot(prot), 4);
 		start++;
 	}
 }
@@ -287,18 +299,19 @@ static void walk_pmd_level(struct seq_file *m, struct pg_state *st, pud_t addr,
 {
 	int i;
 	pmd_t *start;
+	pgprotval_t prot;
 
 	start = (pmd_t *) pud_page_vaddr(addr);
 	for (i = 0; i < PTRS_PER_PMD; i++) {
 		st->current_address = normalize_addr(P + i * PMD_LEVEL_MULT);
 		if (!pmd_none(*start)) {
-			pgprotval_t prot = pmd_val(*start) & PTE_FLAGS_MASK;
-
-			if (pmd_large(*start) || !pmd_present(*start))
+			if (pmd_large(*start) || !pmd_present(*start)) {
+				prot = pmd_flags(*start);
 				note_page(m, st, __pgprot(prot), 3);
-			else
+			} else {
 				walk_pte_level(m, st, *start,
 					       P + i * PMD_LEVEL_MULT);
+			}
 		} else
 			note_page(m, st, __pgprot(0), 3);
 		start++;
@@ -318,19 +331,20 @@ static void walk_pud_level(struct seq_file *m, struct pg_state *st, pgd_t addr,
 {
 	int i;
 	pud_t *start;
+	pgprotval_t prot;
 
 	start = (pud_t *) pgd_page_vaddr(addr);
 
 	for (i = 0; i < PTRS_PER_PUD; i++) {
 		st->current_address = normalize_addr(P + i * PUD_LEVEL_MULT);
 		if (!pud_none(*start)) {
-			pgprotval_t prot = pud_val(*start) & PTE_FLAGS_MASK;
-
-			if (pud_large(*start) || !pud_present(*start))
+			if (pud_large(*start) || !pud_present(*start)) {
+				prot = pud_flags(*start);
 				note_page(m, st, __pgprot(prot), 2);
-			else
+			} else {
 				walk_pmd_level(m, st, *start,
 					       P + i * PUD_LEVEL_MULT);
+			}
 		} else
 			note_page(m, st, __pgprot(0), 2);
 
@@ -344,13 +358,15 @@ static void walk_pud_level(struct seq_file *m, struct pg_state *st, pgd_t addr,
 #define pgd_none(a)  pud_none(__pud(pgd_val(a)))
 #endif
 
-void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
+static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
+				       bool checkwx)
 {
 #ifdef CONFIG_X86_64
 	pgd_t *start = (pgd_t *) &init_level4_pgt;
 #else
 	pgd_t *start = swapper_pg_dir;
 #endif
+	pgprotval_t prot;
 	int i;
 	struct pg_state st = {};
 
@@ -359,16 +375,20 @@ void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
 		st.to_dmesg = true;
 	}
 
+	st.check_wx = checkwx;
+	if (checkwx)
+		st.wx_pages = 0;
+
 	for (i = 0; i < PTRS_PER_PGD; i++) {
 		st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
 		if (!pgd_none(*start)) {
-			pgprotval_t prot = pgd_val(*start) & PTE_FLAGS_MASK;
-
-			if (pgd_large(*start) || !pgd_present(*start))
+			if (pgd_large(*start) || !pgd_present(*start)) {
+				prot = pgd_flags(*start);
 				note_page(m, &st, __pgprot(prot), 1);
-			else
+			} else {
 				walk_pud_level(m, &st, *start,
 					       i * PGD_LEVEL_MULT);
+			}
 		} else
 			note_page(m, &st, __pgprot(0), 1);
 
@@ -378,8 +398,26 @@ void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
 	/* Flush out the last page */
 	st.current_address = normalize_addr(PTRS_PER_PGD*PGD_LEVEL_MULT);
 	note_page(m, &st, __pgprot(0), 0);
+	if (!checkwx)
+		return;
+	if (st.wx_pages)
+		pr_info("x86/mm: Checked W+X mappings: FAILED, %lu W+X pages found.\n",
+			st.wx_pages);
+	else
+		pr_info("x86/mm: Checked W+X mappings: passed, no W+X pages found.\n");
 }
 
+void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
+{
+	ptdump_walk_pgd_level_core(m, pgd, false);
+}
+
+void ptdump_walk_pgd_level_checkwx(void)
+{
+	ptdump_walk_pgd_level_core(NULL, NULL, true);
+}
+
+#ifdef CONFIG_X86_PTDUMP
 static int ptdump_show(struct seq_file *m, void *v)
 {
 	ptdump_walk_pgd_level(m, NULL);
@@ -397,10 +435,13 @@ static const struct file_operations ptdump_fops = {
 	.llseek		= seq_lseek,
 	.release	= single_release,
 };
+#endif
 
 static int pt_dump_init(void)
 {
+#ifdef CONFIG_X86_PTDUMP
 	struct dentry *pe;
+#endif
 
 #ifdef CONFIG_X86_32
 	/* Not a compile-time constant on x86-32 */
@@ -412,10 +453,12 @@ static int pt_dump_init(void)
 	address_markers[FIXADDR_START_NR].start_address = FIXADDR_START;
 #endif
 
+#ifdef CONFIG_X86_PTDUMP
 	pe = debugfs_create_file("kernel_page_tables", 0600, NULL, NULL,
 				 &ptdump_fops);
 	if (!pe)
 		return -ENOMEM;
+#endif
 
 	return 0;
 }
diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c
index 81bf3d2af3eb..ae9a37bf1371 100644
--- a/arch/x86/mm/gup.c
+++ b/arch/x86/mm/gup.c
@@ -118,21 +118,20 @@ static noinline int gup_huge_pmd(pmd_t pmd, unsigned long addr,
 		unsigned long end, int write, struct page **pages, int *nr)
 {
 	unsigned long mask;
-	pte_t pte = *(pte_t *)&pmd;
 	struct page *head, *page;
 	int refs;
 
 	mask = _PAGE_PRESENT|_PAGE_USER;
 	if (write)
 		mask |= _PAGE_RW;
-	if ((pte_flags(pte) & mask) != mask)
+	if ((pmd_flags(pmd) & mask) != mask)
 		return 0;
 	/* hugepages are never "special" */
-	VM_BUG_ON(pte_flags(pte) & _PAGE_SPECIAL);
-	VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
+	VM_BUG_ON(pmd_flags(pmd) & _PAGE_SPECIAL);
+	VM_BUG_ON(!pfn_valid(pmd_pfn(pmd)));
 
 	refs = 0;
-	head = pte_page(pte);
+	head = pmd_page(pmd);
 	page = head + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
 	do {
 		VM_BUG_ON_PAGE(compound_head(page) != head, page);
@@ -195,21 +194,20 @@ static noinline int gup_huge_pud(pud_t pud, unsigned long addr,
 		unsigned long end, int write, struct page **pages, int *nr)
 {
 	unsigned long mask;
-	pte_t pte = *(pte_t *)&pud;
 	struct page *head, *page;
 	int refs;
 
 	mask = _PAGE_PRESENT|_PAGE_USER;
 	if (write)
 		mask |= _PAGE_RW;
-	if ((pte_flags(pte) & mask) != mask)
+	if ((pud_flags(pud) & mask) != mask)
 		return 0;
 	/* hugepages are never "special" */
-	VM_BUG_ON(pte_flags(pte) & _PAGE_SPECIAL);
-	VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
+	VM_BUG_ON(pud_flags(pud) & _PAGE_SPECIAL);
+	VM_BUG_ON(!pfn_valid(pud_pfn(pud)));
 
 	refs = 0;
-	head = pte_page(pte);
+	head = pud_page(pud);
 	page = head + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
 	do {
 		VM_BUG_ON_PAGE(compound_head(page) != head, page);
diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 7562f42914b4..cb4ef3de61f9 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -957,6 +957,8 @@ void mark_rodata_ro(void)
 	set_pages_ro(virt_to_page(start), size >> PAGE_SHIFT);
 #endif
 	mark_nxdata_nx();
+	if (__supported_pte_mask & _PAGE_NX)
+		debug_checkwx();
 }
 #endif
 
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 30564e2752d3..f8b157366700 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1150,6 +1150,8 @@ void mark_rodata_ro(void)
 	free_init_pages("unused kernel",
 			(unsigned long) __va(__pa_symbol(rodata_end)),
 			(unsigned long) __va(__pa_symbol(_sdata)));
+
+	debug_checkwx();
 }
 
 #endif
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index 2c44c0792301..e2621a8e8213 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -414,18 +414,28 @@ pmd_t *lookup_pmd_address(unsigned long address)
 phys_addr_t slow_virt_to_phys(void *__virt_addr)
 {
 	unsigned long virt_addr = (unsigned long)__virt_addr;
-	phys_addr_t phys_addr;
-	unsigned long offset;
+	unsigned long phys_addr, offset;
 	enum pg_level level;
-	unsigned long pmask;
 	pte_t *pte;
 
 	pte = lookup_address(virt_addr, &level);
 	BUG_ON(!pte);
-	pmask = page_level_mask(level);
-	offset = virt_addr & ~pmask;
-	phys_addr = (phys_addr_t)pte_pfn(*pte) << PAGE_SHIFT;
-	return (phys_addr | offset);
+
+	switch (level) {
+	case PG_LEVEL_1G:
+		phys_addr = pud_pfn(*(pud_t *)pte) << PAGE_SHIFT;
+		offset = virt_addr & ~PUD_PAGE_MASK;
+		break;
+	case PG_LEVEL_2M:
+		phys_addr = pmd_pfn(*(pmd_t *)pte) << PAGE_SHIFT;
+		offset = virt_addr & ~PMD_PAGE_MASK;
+		break;
+	default:
+		phys_addr = pte_pfn(*pte) << PAGE_SHIFT;
+		offset = virt_addr & ~PAGE_MASK;
+	}
+
+	return (phys_addr_t)(phys_addr | offset);
 }
 EXPORT_SYMBOL_GPL(slow_virt_to_phys);
 
@@ -458,7 +468,7 @@ static int
 try_preserve_large_page(pte_t *kpte, unsigned long address,
 			struct cpa_data *cpa)
 {
-	unsigned long nextpage_addr, numpages, pmask, psize, addr, pfn;
+	unsigned long nextpage_addr, numpages, pmask, psize, addr, pfn, old_pfn;
 	pte_t new_pte, old_pte, *tmp;
 	pgprot_t old_prot, new_prot, req_prot;
 	int i, do_split = 1;
@@ -478,17 +488,21 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 
 	switch (level) {
 	case PG_LEVEL_2M:
-#ifdef CONFIG_X86_64
+		old_prot = pmd_pgprot(*(pmd_t *)kpte);
+		old_pfn = pmd_pfn(*(pmd_t *)kpte);
+		break;
 	case PG_LEVEL_1G:
-#endif
-		psize = page_level_size(level);
-		pmask = page_level_mask(level);
+		old_prot = pud_pgprot(*(pud_t *)kpte);
+		old_pfn = pud_pfn(*(pud_t *)kpte);
 		break;
 	default:
 		do_split = -EINVAL;
 		goto out_unlock;
 	}
 
+	psize = page_level_size(level);
+	pmask = page_level_mask(level);
+
 	/*
 	 * Calculate the number of pages, which fit into this large
 	 * page starting at address:
@@ -504,7 +518,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 	 * up accordingly.
 	 */
 	old_pte = *kpte;
-	old_prot = req_prot = pgprot_large_2_4k(pte_pgprot(old_pte));
+	req_prot = pgprot_large_2_4k(old_prot);
 
 	pgprot_val(req_prot) &= ~pgprot_val(cpa->mask_clr);
 	pgprot_val(req_prot) |= pgprot_val(cpa->mask_set);
@@ -530,10 +544,10 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 	req_prot = canon_pgprot(req_prot);
 
 	/*
-	 * old_pte points to the large page base address. So we need
+	 * old_pfn points to the large page base pfn. So we need
 	 * to add the offset of the virtual address:
 	 */
-	pfn = pte_pfn(old_pte) + ((address & (psize - 1)) >> PAGE_SHIFT);
+	pfn = old_pfn + ((address & (psize - 1)) >> PAGE_SHIFT);
 	cpa->pfn = pfn;
 
 	new_prot = static_protections(req_prot, address, pfn);
@@ -544,7 +558,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 	 * the pages in the range we try to preserve:
 	 */
 	addr = address & pmask;
-	pfn = pte_pfn(old_pte);
+	pfn = old_pfn;
 	for (i = 0; i < (psize >> PAGE_SHIFT); i++, addr += PAGE_SIZE, pfn++) {
 		pgprot_t chk_prot = static_protections(req_prot, addr, pfn);
 
@@ -574,7 +588,7 @@ try_preserve_large_page(pte_t *kpte, unsigned long address,
 		 * The address is aligned and the number of pages
 		 * covers the full page.
 		 */
-		new_pte = pfn_pte(pte_pfn(old_pte), new_prot);
+		new_pte = pfn_pte(old_pfn, new_prot);
 		__set_pmd_pte(kpte, address, new_pte);
 		cpa->flags |= CPA_FLUSHTLB;
 		do_split = 0;
@@ -591,7 +605,7 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte, unsigned long address,
 		   struct page *base)
 {
 	pte_t *pbase = (pte_t *)page_address(base);
-	unsigned long pfn, pfninc = 1;
+	unsigned long ref_pfn, pfn, pfninc = 1;
 	unsigned int i, level;
 	pte_t *tmp;
 	pgprot_t ref_prot;
@@ -608,26 +622,33 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte, unsigned long address,
 	}
 
 	paravirt_alloc_pte(&init_mm, page_to_pfn(base));
-	ref_prot = pte_pgprot(pte_clrhuge(*kpte));
 
-	/* promote PAT bit to correct position */
-	if (level == PG_LEVEL_2M)
+	switch (level) {
+	case PG_LEVEL_2M:
+		ref_prot = pmd_pgprot(*(pmd_t *)kpte);
+		/* clear PSE and promote PAT bit to correct position */
 		ref_prot = pgprot_large_2_4k(ref_prot);
+		ref_pfn = pmd_pfn(*(pmd_t *)kpte);
+		break;
 
-#ifdef CONFIG_X86_64
-	if (level == PG_LEVEL_1G) {
+	case PG_LEVEL_1G:
+		ref_prot = pud_pgprot(*(pud_t *)kpte);
+		ref_pfn = pud_pfn(*(pud_t *)kpte);
 		pfninc = PMD_PAGE_SIZE >> PAGE_SHIFT;
+
 		/*
-		 * Set the PSE flags only if the PRESENT flag is set
+		 * Clear the PSE flags if the PRESENT flag is not set
 		 * otherwise pmd_present/pmd_huge will return true
 		 * even on a non present pmd.
 		 */
-		if (pgprot_val(ref_prot) & _PAGE_PRESENT)
-			pgprot_val(ref_prot) |= _PAGE_PSE;
-		else
+		if (!(pgprot_val(ref_prot) & _PAGE_PRESENT))
 			pgprot_val(ref_prot) &= ~_PAGE_PSE;
+		break;
+
+	default:
+		spin_unlock(&pgd_lock);
+		return 1;
 	}
-#endif
 
 	/*
 	 * Set the GLOBAL flags only if the PRESENT flag is set
@@ -643,7 +664,7 @@ __split_large_page(struct cpa_data *cpa, pte_t *kpte, unsigned long address,
 	/*
 	 * Get the target pfn from the original entry:
 	 */
-	pfn = pte_pfn(*kpte);
+	pfn = ref_pfn;
 	for (i = 0; i < PTRS_PER_PTE; i++, pfn += pfninc)
 		set_pte(&pbase[i], pfn_pte(pfn, canon_pgprot(ref_prot)));
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1262537

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-11-04 20:30 +0100
Message-ID<qrbUR-4Bi-17@gated-at.bofh.it>
In reply to#1261410
On Tue, Nov 3, 2015 at 3:16 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> The new CONFIG_DEBUG_WX=y warning is marked default-y if CONFIG_DEBUG_RODATA=y is
> already eanbled, as a special exception, as these bugs are hard to notice and this
> check already found several live bugs.

So this seems to be not very useful.

  ...
  Freeing unused kernel memory: 1068K (ffffffff8bcc0000 -
ffffffff8bdcb000)
  Write protecting the kernel read-only data: 12288k
  Freeing unused kernel memory: 1944K (ffff88000b61a000 - ffff88000b800000)
  Freeing unused kernel memory: 1372K (ffff88000baa9000 - ffff88000bc00000)
  ------------[ cut here ]------------
  WARNING: CPU: 7 PID: 1 at arch/x86/mm/dump_pagetables.c:225
note_page+0x5dc/0x780()
  x86/mm: Found insecure W+X mapping at address
ffff88000005f000/0xffff88000005f000
  ...
  x86/mm: Checked W+X mappings: FAILED, 165660 W+X pages found.
  ...

because it doesn't seem to give you any idea where to look for the
issue. Those 165660 pages come out to 647MB. Odd number.

Is this *supposed* to come out clean?

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262740

FromDave Jones <davej@codemonkey.org.uk>
Date2015-11-05 00:40 +0100
Message-ID<qrfON-78K-13@gated-at.bofh.it>
In reply to#1262537
On Wed, Nov 04, 2015 at 11:26:12AM -0800, Linus Torvalds wrote:
 > On Tue, Nov 3, 2015 at 3:16 AM, Ingo Molnar <mingo@kernel.org> wrote:
 > >
 > > The new CONFIG_DEBUG_WX=y warning is marked default-y if CONFIG_DEBUG_RODATA=y is
 > > already eanbled, as a special exception, as these bugs are hard to notice and this
 > > check already found several live bugs.
 > 
 > So this seems to be not very useful.
 > 
 >   ...
 >   Freeing unused kernel memory: 1068K (ffffffff8bcc0000 -
 > ffffffff8bdcb000)
 >   Write protecting the kernel read-only data: 12288k
 >   Freeing unused kernel memory: 1944K (ffff88000b61a000 - ffff88000b800000)
 >   Freeing unused kernel memory: 1372K (ffff88000baa9000 - ffff88000bc00000)
 >   ------------[ cut here ]------------
 >   WARNING: CPU: 7 PID: 1 at arch/x86/mm/dump_pagetables.c:225
 > note_page+0x5dc/0x780()
 >   x86/mm: Found insecure W+X mapping at address
 > ffff88000005f000/0xffff88000005f000
 >   ...
 >   x86/mm: Checked W+X mappings: FAILED, 165660 W+X pages found.
 >   ...
 > 
 > because it doesn't seem to give you any idea where to look for the
 > issue. Those 165660 pages come out to 647MB. Odd number.
 > 
 > Is this *supposed* to come out clean?

FWIW I'm seeing this too.

[    3.293503] x86/mm: Found insecure W+X mapping at address ffff880000800000/0xffff880000800000

Looking further up the dmesg I see that range is..

[    0.468368] ---[ Low Kernel Mapping ]---
[    0.468381] 0xffff880000000000-0xffff880000800000           8M     RW                 GLB NX pte
[    0.468391] 0xffff880000800000-0xffff880000900000           1M     RW                 GLB x  pte

Linus, does that match your trace too ? The 2nd low kernel mapping?

	Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262797

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-11-05 02:40 +0100
Message-ID<qrhGV-8jJ-7@gated-at.bofh.it>
In reply to#1262740
On Wed, Nov 4, 2015 at 3:39 PM, Dave Jones <davej@codemonkey.org.uk> wrote:
>
> FWIW I'm seeing this too.
>
> [    0.468368] ---[ Low Kernel Mapping ]---
> [    0.468381] 0xffff880000000000-0xffff880000800000           8M     RW                 GLB NX pte
> [    0.468391] 0xffff880000800000-0xffff880000900000           1M     RW                 GLB x  pte
>
> Linus, does that match your trace too ? The 2nd low kernel mapping?

I don't have that later debug output at all. Presumably some config difference.

               Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262825

FromDave Jones <davej@codemonkey.org.uk>
Date2015-11-05 03:20 +0100
Message-ID<qrijD-mC-1@gated-at.bofh.it>
In reply to#1262797
On Wed, Nov 04, 2015 at 05:31:59PM -0800, Linus Torvalds wrote:
 > On Wed, Nov 4, 2015 at 3:39 PM, Dave Jones <davej@codemonkey.org.uk> wrote:
 > >
 > > FWIW I'm seeing this too.
 > >
 > > [    0.468368] ---[ Low Kernel Mapping ]---
 > > [    0.468381] 0xffff880000000000-0xffff880000800000           8M     RW                 GLB NX pte
 > > [    0.468391] 0xffff880000800000-0xffff880000900000           1M     RW                 GLB x  pte
 > >
 > > Linus, does that match your trace too ? The 2nd low kernel mapping?
 > 
 > I don't have that later debug output at all. Presumably some config difference.

CONFIG_X86_PTDUMP_CORE iirc.

	Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263561

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-11-05 22:30 +0100
Message-ID<qrAgx-3yX-3@gated-at.bofh.it>
In reply to#1262825
On Wed, Nov 4, 2015 at 6:17 PM, Dave Jones <davej@codemonkey.org.uk> wrote:
> On Wed, Nov 04, 2015 at 05:31:59PM -0800, Linus Torvalds wrote:
>  >
>  > I don't have that later debug output at all. Presumably some config difference.
>
> CONFIG_X86_PTDUMP_CORE iirc.

No, I have that. I suspect CONFIG_EFI_PGT_DUMP instead.

Anyway, as it stands now, I think the CONFIG_DEBUG_WX option should
not default to 'y' unless it is made more useful if it actually
triggers. Ingo?

                    Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263567

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-11-05 22:40 +0100
Message-ID<qrAqf-3D6-15@gated-at.bofh.it>
In reply to#1263561
On Thu, Nov 5, 2015 at 1:27 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I suspect CONFIG_EFI_PGT_DUMP instead.
>
> Anyway, as it stands now, I think the CONFIG_DEBUG_WX option should
> not default to 'y' unless it is made more useful if it actually
> triggers. Ingo?

Actually, I guess I should have cc'd Steven Smalley too, since he's the author.

Steven, the problem is that without other debug options, the
CONFIG_DEBUG_WX option doesn't seem useful. It shows a problem, but it
doesn't actually show any information about where that issue comes
from or any other details to even *guess* at what is going on.

And if this turns out to be due to EFI wanting those permissions, what
should we do? People have talked about running the EFI callbacks in
their own private page table setup, which sounds like the right idea,
but until that actually *happens*....

                  Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263929

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2015-11-06 12:40 +0100
Message-ID<qrNx7-3U3-23@gated-at.bofh.it>
In reply to#1263567
On Thu, 05 Nov, at 01:33:10PM, Linus Torvalds wrote:
> 
> And if this turns out to be due to EFI wanting those permissions, what
> should we do? People have talked about running the EFI callbacks in
> their own private page table setup, which sounds like the right idea,
> but until that actually *happens*....

We have separate page tables today, for a few reasons, but mainly it's
so that we can have an identity mapping of memory present in the
region usually used by user processes - broken firmware still uses
those identity mappings even after the kernel tells it they're
invalid.

Note that when I say "separate" I'm talking about trampoline_pgd[]
which is also used by the x86 suspend/resume code.

However, turns out that the issue with the current scheme is the fact
that trampoline_pgd[] actually shares a couple of PGD entries with
swapper_pg_dir as can be seen in setup_real_mode(),


        trampoline_pgd = (u64 *)__va(real_mode_header->trampoline_pgd);
        trampoline_pgd[0] = init_level4_pgt[pgd_index(__PAGE_OFFSET)].pgd;
        trampoline_pgd[511] = init_level4_pgt[511].pgd;


So when we map the EFI regions in efi_map_regions() we're inserting
them into swapper_pg_dir also, which is why you're seeing the
warnings.

If I remember correctly the rationale for using trampoline_pgd[] was
that it already did what we wanted (provided the identity mapping) and
would save us the overhead of maintaining more page tables for no good
reason. Obviously this entire thread is a good reason.

I suggest we stop using trampoline_pgd[] (since it has a good reason
for sharing the kernel mapping PGD entries) and create our own so that
we can isolate EFI completely.

For the immediate problem of the warnings spewing forth on all UEFI
machines, at the very least the config options needs to be disabled by
default, if not the patch reverted.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264746

FromIngo Molnar <mingo@kernel.org>
Date2015-11-07 08:10 +0100
Message-ID<qs5Nn-7zp-5@gated-at.bofh.it>
In reply to#1263929
* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> On Thu, 05 Nov, at 01:33:10PM, Linus Torvalds wrote:
> > 
> > And if this turns out to be due to EFI wanting those permissions, what should 
> > we do? People have talked about running the EFI callbacks in their own private 
> > page table setup, which sounds like the right idea, but until that actually 
> > *happens*....
> 
> We have separate page tables today, for a few reasons, but mainly it's
> so that we can have an identity mapping of memory present in the
> region usually used by user processes - broken firmware still uses
> those identity mappings even after the kernel tells it they're
> invalid.
> 
> Note that when I say "separate" I'm talking about trampoline_pgd[]
> which is also used by the x86 suspend/resume code.
> 
> However, turns out that the issue with the current scheme is the fact
> that trampoline_pgd[] actually shares a couple of PGD entries with
> swapper_pg_dir as can be seen in setup_real_mode(),
> 
> 
>         trampoline_pgd = (u64 *)__va(real_mode_header->trampoline_pgd);
>         trampoline_pgd[0] = init_level4_pgt[pgd_index(__PAGE_OFFSET)].pgd;
>         trampoline_pgd[511] = init_level4_pgt[511].pgd;
> 
> 
> So when we map the EFI regions in efi_map_regions() we're inserting
> them into swapper_pg_dir also, which is why you're seeing the
> warnings.
> 
> If I remember correctly the rationale for using trampoline_pgd[] was
> that it already did what we wanted (provided the identity mapping) and
> would save us the overhead of maintaining more page tables for no good
> reason. Obviously this entire thread is a good reason.
> 
> I suggest we stop using trampoline_pgd[] (since it has a good reason
> for sharing the kernel mapping PGD entries) and create our own so that
> we can isolate EFI completely.

Ok. Could you please make this fix a priority for upcoming EFI changes?

> For the immediate problem of the warnings spewing forth on all UEFI
> machines, at the very least the config options needs to be disabled by
> default, if not the patch reverted.

We'll certainly flip around the default, but reverting would be shooting
the messenger: the EFI code is endangering everyone else today, and for
no good reason as it appears... so the warning very much served its
purpose in pointing out a valid problem.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264793

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2015-11-07 11:10 +0100
Message-ID<qs8Bz-Te-1@gated-at.bofh.it>
In reply to#1264746
On Sat, 07 Nov, at 08:05:54AM, Ingo Molnar wrote:
> 
> * Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > On Thu, 05 Nov, at 01:33:10PM, Linus Torvalds wrote:
> > > 
> > > And if this turns out to be due to EFI wanting those permissions, what should 
> > > we do? People have talked about running the EFI callbacks in their own private 
> > > page table setup, which sounds like the right idea, but until that actually 
> > > *happens*....
> > 
> > We have separate page tables today, for a few reasons, but mainly it's
> > so that we can have an identity mapping of memory present in the
> > region usually used by user processes - broken firmware still uses
> > those identity mappings even after the kernel tells it they're
> > invalid.
> > 
> > Note that when I say "separate" I'm talking about trampoline_pgd[]
> > which is also used by the x86 suspend/resume code.
> > 
> > However, turns out that the issue with the current scheme is the fact
> > that trampoline_pgd[] actually shares a couple of PGD entries with
> > swapper_pg_dir as can be seen in setup_real_mode(),
> > 
> > 
> >         trampoline_pgd = (u64 *)__va(real_mode_header->trampoline_pgd);
> >         trampoline_pgd[0] = init_level4_pgt[pgd_index(__PAGE_OFFSET)].pgd;
> >         trampoline_pgd[511] = init_level4_pgt[511].pgd;
> > 
> > 
> > So when we map the EFI regions in efi_map_regions() we're inserting
> > them into swapper_pg_dir also, which is why you're seeing the
> > warnings.
> > 
> > If I remember correctly the rationale for using trampoline_pgd[] was
> > that it already did what we wanted (provided the identity mapping) and
> > would save us the overhead of maintaining more page tables for no good
> > reason. Obviously this entire thread is a good reason.
> > 
> > I suggest we stop using trampoline_pgd[] (since it has a good reason
> > for sharing the kernel mapping PGD entries) and create our own so that
> > we can isolate EFI completely.
> 
> Ok. Could you please make this fix a priority for upcoming EFI changes?

Yep, I'll get on it.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263574

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2015-11-05 23:10 +0100
Message-ID<qrATh-44X-19@gated-at.bofh.it>
In reply to#1263561
On Thu, Nov 5, 2015 at 1:27 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> No, I have that. I suspect CONFIG_EFI_PGT_DUMP instead.

Yes, that seems to show the tables, and agrees with the problem address.

So for me I have:

    WARNING: CPU: 1 PID: 1 at arch/x86/mm/dump_pagetables.c:225
note_page+0x5dc/0x780()
    x86/mm: Found insecure W+X mapping at address
ffff88000005f000/0xffff88000005f000

and that 005f000 address also shows up in the firmware tables:

    ACPI: 6 ACPI AML tables successfully acquired and loaded
    ---[ User Space ]---
    0x0000000000000000-0x000000000005f000         380K     RW
       GLB NX pte
    0x000000000005f000-0x0000000000060000           4K     RW
       GLB x  pte
    0x0000000000060000-0x0000000000200000        1664K     RW
       GLB NX pte
    ...
    ---[ Low Kernel Mapping ]---
    0xffff880000000000-0xffff88000005f000         380K     RW
       GLB NX pte
    0xffff88000005f000-0xffff880000060000           4K     RW
       GLB x  pte
    0xffff880000060000-0xffff880000200000        1664K     RW
       GLB NX pte
    ...

and there's quite a few other pages there that are RW but not marked
NX. I suspect they come from the EFI runtime services because the
pattern seems to match what I see in that area, but there's at least a
PSE mapping at START_KERNEL_map too, etc.

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263585

FromBorislav Petkov <bp@alien8.de>
Date2015-11-05 23:30 +0100
Message-ID<qrBcB-4gD-1@gated-at.bofh.it>
In reply to#1263574
On Thu, Nov 05, 2015 at 02:04:55PM -0800, Linus Torvalds wrote:
> and there's quite a few other pages there that are RW but not marked
> NX. I suspect they come from the EFI runtime services because the

Yeah, at least the EFI mappings would need a bit more fiddling until
they're NX:

https://lkml.kernel.org/r/20151012124113.GD2579@codeblueprint.co.uk

Perhaps we don't even have to make them NX - just set the PGD entry
containing them before the EFI call and unset it after its done.
I.e., something like that.

> pattern seems to match what I see in that area, but there's at least a
> PSE mapping at START_KERNEL_map too, etc.

Maybe we should default n CONFIG_DEBUG_WX until stuff has been fixed...?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263786

FromIngo Molnar <mingo@kernel.org>
Date2015-11-06 08:00 +0100
Message-ID<qrJaa-13t-7@gated-at.bofh.it>
In reply to#1263561
* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Nov 4, 2015 at 6:17 PM, Dave Jones <davej@codemonkey.org.uk> wrote:
> > On Wed, Nov 04, 2015 at 05:31:59PM -0800, Linus Torvalds wrote:
> >  >
> >  > I don't have that later debug output at all. Presumably some config difference.
> >
> > CONFIG_X86_PTDUMP_CORE iirc.
> 
> No, I have that. I suspect CONFIG_EFI_PGT_DUMP instead.
> 
> Anyway, as it stands now, I think the CONFIG_DEBUG_WX option should
> not default to 'y' unless it is made more useful if it actually
> triggers. Ingo?

Yeah, agreed absolutely.

So this is a bit sad because RWX pages are a real problem in practice, especially 
since the EFI addresses are well predictable, but generating a warning without 
being able to fix it quickly is counterproductive as well, as it only annoys 
people and makes them turn off the option. (Which we could do as well to begin 
with, without the annoyance factor...)

So the plan would be:

 1) Make it default-n.

 2) We should try to further improve the messages to make it easier to determine
    what's wrong. We _do_ try to output symbolic information in the warning, to 
    make it easier to find buggy mappings, but these are not standard kernel
    mappings. So I think we need an e820 mappings based semi-symbolic printout of
    bad addresses - maybe even correlate it with the MMIO resource tree.

 3) We should fix the EFI permission problem without relying on the firmware: it 
    appears we could just mark everything R-X optimistically, and if a write fault 
    happens (it's pretty rare in fact, only triggers when we write to an EFI 
    variable and so), we can mark the faulting page RW- on the fly, because it 
    appears that writable EFI sections, while not enumerated very well in 'old' 
    firmware, are still supposed to be page granular. (Even 'new' firmware I 
    wouldn't automatically trust to get the enumeration right...)

    If that 'supposed to be' turns out to be 'not true' (not unheard of in
    firmware land), then plan B would be to mark pages that generate write faults 
    RWX as well, to not break functionality. (This 'mark it RWX' is not something 
    that exploits would have easy access to, and we could also generate a warning
    [after the EFI call has finished] if it ever triggers.)

    Admittedly this approach might not be without its own complications, but it 
    looks reasonably simple (I don't think we need per EFI call page tables, 
    etc.), and does not assume much about the firmware being able to enumerate its 
    permissions properly. Were we to merge EFI support today I'd have insisted on 
    trying such an approach from day 1 on.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263793

FromAndy Lutomirski <luto@amacapital.net>
Date2015-11-06 08:10 +0100
Message-ID<qrJjQ-1lQ-11@gated-at.bofh.it>
In reply to#1263786
On Thu, Nov 5, 2015 at 10:55 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> On Wed, Nov 4, 2015 at 6:17 PM, Dave Jones <davej@codemonkey.org.uk> wrote:
>> > On Wed, Nov 04, 2015 at 05:31:59PM -0800, Linus Torvalds wrote:
>> >  >
>> >  > I don't have that later debug output at all. Presumably some config difference.
>> >
>> > CONFIG_X86_PTDUMP_CORE iirc.
>>
>> No, I have that. I suspect CONFIG_EFI_PGT_DUMP instead.
>>
>> Anyway, as it stands now, I think the CONFIG_DEBUG_WX option should
>> not default to 'y' unless it is made more useful if it actually
>> triggers. Ingo?
>
> Yeah, agreed absolutely.
>
> So this is a bit sad because RWX pages are a real problem in practice, especially
> since the EFI addresses are well predictable, but generating a warning without
> being able to fix it quickly is counterproductive as well, as it only annoys
> people and makes them turn off the option. (Which we could do as well to begin
> with, without the annoyance factor...)
>
> So the plan would be:
>
>  1) Make it default-n.
>
>  2) We should try to further improve the messages to make it easier to determine
>     what's wrong. We _do_ try to output symbolic information in the warning, to
>     make it easier to find buggy mappings, but these are not standard kernel
>     mappings. So I think we need an e820 mappings based semi-symbolic printout of
>     bad addresses - maybe even correlate it with the MMIO resource tree.
>
>  3) We should fix the EFI permission problem without relying on the firmware: it
>     appears we could just mark everything R-X optimistically, and if a write fault
>     happens (it's pretty rare in fact, only triggers when we write to an EFI
>     variable and so), we can mark the faulting page RW- on the fly, because it
>     appears that writable EFI sections, while not enumerated very well in 'old'
>     firmware, are still supposed to be page granular. (Even 'new' firmware I
>     wouldn't automatically trust to get the enumeration right...)

I think it was Borislav who pointed out that this idea, which might
have been mine, is a bit silly.  Why not just skip mapping the EFI
stuff in the init_pgd entirely and only map it in the EFI pgd?

We'll have RWX stuff in the EFI pgd, but so what?  If we're exposing
anything that runs with the EFI pgd loaded to untrusted input, I think
we've already lost.

Admittedly, we might need to use a certain amount of care to avoid
interesting conflicts with the vmap mechanism.  We might need to vmap
all of the EFI stuff, and possibly even all the top-level entries that
contain EFI stuff (i.e. exactly one of them unless EFI ends up *huge*)
as a blank not-present region to avoid overlaps, but that's not a big
deal.

>
>     If that 'supposed to be' turns out to be 'not true' (not unheard of in
>     firmware land), then plan B would be to mark pages that generate write faults
>     RWX as well, to not break functionality. (This 'mark it RWX' is not something
>     that exploits would have easy access to, and we could also generate a warning
>     [after the EFI call has finished] if it ever triggers.)
>
>     Admittedly this approach might not be without its own complications, but it
>     looks reasonably simple (I don't think we need per EFI call page tables,
>     etc.), and does not assume much about the firmware being able to enumerate its
>     permissions properly. Were we to merge EFI support today I'd have insisted on
>     trying such an approach from day 1 on.

I think we have separate EFI page tables already for other reasons.  I
could be wrong -- I've never really understood the EFI mapping layout
very well.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264000

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2015-11-06 14:10 +0100
Message-ID<qrOWd-4V2-7@gated-at.bofh.it>
In reply to#1263793
On Thu, 05 Nov, at 11:05:35PM, Andy Lutomirski wrote:
> 
> Admittedly, we might need to use a certain amount of care to avoid
> interesting conflicts with the vmap mechanism.  We might need to vmap
> all of the EFI stuff, and possibly even all the top-level entries that
> contain EFI stuff (i.e. exactly one of them unless EFI ends up *huge*)
> as a blank not-present region to avoid overlaps, but that's not a big
> deal.

There shouldn't be any room for conflicting with vmap() because the VA
region where we map EFI regions is still carved out especially for us.

Right Boris?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264007

FromBorislav Petkov <bp@alien8.de>
Date2015-11-06 14:30 +0100
Message-ID<qrPfA-52O-1@gated-at.bofh.it>
In reply to#1264000
On Fri, Nov 06, 2015 at 01:09:48PM +0000, Matt Fleming wrote:
> On Thu, 05 Nov, at 11:05:35PM, Andy Lutomirski wrote:
> > 
> > Admittedly, we might need to use a certain amount of care to avoid
> > interesting conflicts with the vmap mechanism.  We might need to vmap
> > all of the EFI stuff, and possibly even all the top-level entries that
> > contain EFI stuff (i.e. exactly one of them unless EFI ends up *huge*)
> > as a blank not-present region to avoid overlaps, but that's not a big
> > deal.
> 
> There shouldn't be any room for conflicting with vmap() because the VA
> region where we map EFI regions is still carved out especially for us.
> 
> Right Boris?

Yap:

ffffea0000000000 - ffffeaffffffffff (=40 bits) virtual memory map (1TB)

vs

ffffffef00000000 - ffffffff00000000 EFI region in trampoline_pgd

the new pagetable will make that issue moot too.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264747

FromIngo Molnar <mingo@kernel.org>
Date2015-11-07 08:10 +0100
Message-ID<qs5No-7zp-11@gated-at.bofh.it>
In reply to#1263793
* Andy Lutomirski <luto@amacapital.net> wrote:

> On Thu, Nov 5, 2015 at 10:55 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >> On Wed, Nov 4, 2015 at 6:17 PM, Dave Jones <davej@codemonkey.org.uk> wrote:
> >> > On Wed, Nov 04, 2015 at 05:31:59PM -0800, Linus Torvalds wrote:
> >> >  >
> >> >  > I don't have that later debug output at all. Presumably some config difference.
> >> >
> >> > CONFIG_X86_PTDUMP_CORE iirc.
> >>
> >> No, I have that. I suspect CONFIG_EFI_PGT_DUMP instead.
> >>
> >> Anyway, as it stands now, I think the CONFIG_DEBUG_WX option should
> >> not default to 'y' unless it is made more useful if it actually
> >> triggers. Ingo?
> >
> > Yeah, agreed absolutely.
> >
> > So this is a bit sad because RWX pages are a real problem in practice, especially
> > since the EFI addresses are well predictable, but generating a warning without
> > being able to fix it quickly is counterproductive as well, as it only annoys
> > people and makes them turn off the option. (Which we could do as well to begin
> > with, without the annoyance factor...)
> >
> > So the plan would be:
> >
> >  1) Make it default-n.
> >
> >  2) We should try to further improve the messages to make it easier to determine
> >     what's wrong. We _do_ try to output symbolic information in the warning, to
> >     make it easier to find buggy mappings, but these are not standard kernel
> >     mappings. So I think we need an e820 mappings based semi-symbolic printout of
> >     bad addresses - maybe even correlate it with the MMIO resource tree.
> >
> >  3) We should fix the EFI permission problem without relying on the firmware: it
> >     appears we could just mark everything R-X optimistically, and if a write fault
> >     happens (it's pretty rare in fact, only triggers when we write to an EFI
> >     variable and so), we can mark the faulting page RW- on the fly, because it
> >     appears that writable EFI sections, while not enumerated very well in 'old'
> >     firmware, are still supposed to be page granular. (Even 'new' firmware I
> >     wouldn't automatically trust to get the enumeration right...)
> 
> I think it was Borislav who pointed out that this idea, which might
> have been mine, is a bit silly.  Why not just skip mapping the EFI
> stuff in the init_pgd entirely and only map it in the EFI pgd?
> 
> We'll have RWX stuff in the EFI pgd, but so what?  If we're exposing
> anything that runs with the EFI pgd loaded to untrusted input, I think
> we've already lost.

That's certainly true, I was simply confused about the life time of these 
mappings: I assumed they have to stay around. If they are meant to be and are 
partly temporary today already, we should go the whole mile and make that really 
so, because _today_ the mappings are permanent, so this is a real problem ...

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263812

FromIngo Molnar <mingo@kernel.org>
Date2015-11-06 08:50 +0100
Message-ID<qrJWx-1yT-3@gated-at.bofh.it>
In reply to#1263786
(resent with Matt's email address fixed.)

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Wed, Nov 4, 2015 at 6:17 PM, Dave Jones <davej@codemonkey.org.uk> wrote:
> > > On Wed, Nov 04, 2015 at 05:31:59PM -0800, Linus Torvalds wrote:
> > >  >
> > >  > I don't have that later debug output at all. Presumably some config difference.
> > >
> > > CONFIG_X86_PTDUMP_CORE iirc.
> > 
> > No, I have that. I suspect CONFIG_EFI_PGT_DUMP instead.
> > 
> > Anyway, as it stands now, I think the CONFIG_DEBUG_WX option should
> > not default to 'y' unless it is made more useful if it actually
> > triggers. Ingo?
> 
> Yeah, agreed absolutely.
> 
> So this is a bit sad because RWX pages are a real problem in practice, especially 
> since the EFI addresses are well predictable, but generating a warning without 
> being able to fix it quickly is counterproductive as well, as it only annoys 
> people and makes them turn off the option. (Which we could do as well to begin 
> with, without the annoyance factor...)
> 
> So the plan would be:
> 
>  1) Make it default-n.
> 
>  2) We should try to further improve the messages to make it easier to determine
>     what's wrong. We _do_ try to output symbolic information in the warning, to 
>     make it easier to find buggy mappings, but these are not standard kernel
>     mappings. So I think we need an e820 mappings based semi-symbolic printout of
>     bad addresses - maybe even correlate it with the MMIO resource tree.
> 
>  3) We should fix the EFI permission problem without relying on the firmware: it 
>     appears we could just mark everything R-X optimistically, and if a write fault 
>     happens (it's pretty rare in fact, only triggers when we write to an EFI 
>     variable and so), we can mark the faulting page RW- on the fly, because it 
>     appears that writable EFI sections, while not enumerated very well in 'old' 
>     firmware, are still supposed to be page granular. (Even 'new' firmware I 
>     wouldn't automatically trust to get the enumeration right...)
> 
>     If that 'supposed to be' turns out to be 'not true' (not unheard of in
>     firmware land), then plan B would be to mark pages that generate write faults 
>     RWX as well, to not break functionality. (This 'mark it RWX' is not something 
>     that exploits would have easy access to, and we could also generate a warning
>     [after the EFI call has finished] if it ever triggers.)
> 
>     Admittedly this approach might not be without its own complications, but it 
>     looks reasonably simple (I don't think we need per EFI call page tables, 
>     etc.), and does not assume much about the firmware being able to enumerate its 
>     permissions properly. Were we to merge EFI support today I'd have insisted on 
>     trying such an approach from day 1 on.
> 
> Thanks,
> 
> 	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263981

FromMatt Fleming <matt@codeblueprint.co.uk>
Date2015-11-06 13:40 +0100
Message-ID<qrOtc-4v4-35@gated-at.bofh.it>
In reply to#1263786
On Fri, 06 Nov, at 07:55:50AM, Ingo Molnar wrote:
> 
>  3) We should fix the EFI permission problem without relying on the firmware: it 
>     appears we could just mark everything R-X optimistically, and if a write fault 
>     happens (it's pretty rare in fact, only triggers when we write to an EFI 
>     variable and so), we can mark the faulting page RW- on the fly, because it 
>     appears that writable EFI sections, while not enumerated very well in 'old' 
>     firmware, are still supposed to be page granular. (Even 'new' firmware I 
>     wouldn't automatically trust to get the enumeration right...)

Sorry, this isn't true. I misled you with one of my earlier posts on
this topic. Let me try and clear things up...

Writing to EFI regions has to do with every invocation of the EFI
runtime services - it's not limited to when you read/write/delete EFI
variables. In fact, EFI variables really have nothing to do with this
discussion, they're a completely opaque concept to the OS, we have no
idea how the firmware implements them. Everything is done via the EFI
boot/runtime services.

The firmware itself will attempt to write to EFI regions when we
invoke the EFI services because that's where the PE/COFF ".data" and
".bss" sections live along with the heap. There's even some relocation
fixups that occur as SetVirtualAddressMap() time so it'll write to
".text" too.

Now, the above PE/COFF sections are usually (always?) contained within
EFI regions of type EfiRuntimeServicesCode. We know this is true
because the firmware folks have told us so, and because stopping that
is the motivation behind the new EFI_PROPERTIES_TABLE feature in UEFI
V2.5.

The data sections within the region are also *not* guaranteed to be
page granular because work was required in Tianocore for emitting
sections with 4k alignment as part of the EFI_PROPERTIES_TABLE
support.

Ultimately, what this means is that if you were to attempt to
dynamically fixup those regions that required write permission, you'd
have to modify the mappings for the majority of the EFI regions
anyway. And if you're blindly allowing write permission as a fixup,
there's not much security to be had.

>     If that 'supposed to be' turns out to be 'not true' (not unheard of in
>     firmware land), then plan B would be to mark pages that generate write faults 
>     RWX as well, to not break functionality. (This 'mark it RWX' is not something 
>     that exploits would have easy access to, and we could also generate a warning
>     [after the EFI call has finished] if it ever triggers.)
> 
>     Admittedly this approach might not be without its own complications, but it 
>     looks reasonably simple (I don't think we need per EFI call page tables, 
>     etc.), and does not assume much about the firmware being able to enumerate its 
>     permissions properly. Were we to merge EFI support today I'd have insisted on 
>     trying such an approach from day 1 on.

We already have separate EFI page tables, though with the caveat that
we share some of swapper_pg_dir's PGD entries. The best solution would
be to stop sharing entries and isolate the EFI mappings from every
other page table structure, so that they're only used during the EFI
service calls.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264745

FromIngo Molnar <mingo@kernel.org>
Date2015-11-07 08:10 +0100
Message-ID<qs5Nn-7zp-3@gated-at.bofh.it>
In reply to#1263981
* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> On Fri, 06 Nov, at 07:55:50AM, Ingo Molnar wrote:
> > 
> >  3) We should fix the EFI permission problem without relying on the firmware: it 
> >     appears we could just mark everything R-X optimistically, and if a write fault 
> >     happens (it's pretty rare in fact, only triggers when we write to an EFI 
> >     variable and so), we can mark the faulting page RW- on the fly, because it 
> >     appears that writable EFI sections, while not enumerated very well in 'old' 
> >     firmware, are still supposed to be page granular. (Even 'new' firmware I 
> >     wouldn't automatically trust to get the enumeration right...)
> 
> Sorry, this isn't true. I misled you with one of my earlier posts on
> this topic. Let me try and clear things up...
> 
> Writing to EFI regions has to do with every invocation of the EFI
> runtime services - it's not limited to when you read/write/delete EFI
> variables. In fact, EFI variables really have nothing to do with this
> discussion, they're a completely opaque concept to the OS, we have no
> idea how the firmware implements them. Everything is done via the EFI
> boot/runtime services.
> 
> The firmware itself will attempt to write to EFI regions when we
> invoke the EFI services because that's where the PE/COFF ".data" and
> ".bss" sections live along with the heap. There's even some relocation
> fixups that occur as SetVirtualAddressMap() time so it'll write to
> ".text" too.
> 
> Now, the above PE/COFF sections are usually (always?) contained within
> EFI regions of type EfiRuntimeServicesCode. We know this is true
> because the firmware folks have told us so, and because stopping that
> is the motivation behind the new EFI_PROPERTIES_TABLE feature in UEFI
> V2.5.
> 
> The data sections within the region are also *not* guaranteed to be
> page granular because work was required in Tianocore for emitting
> sections with 4k alignment as part of the EFI_PROPERTIES_TABLE
> support.
> 
> Ultimately, what this means is that if you were to attempt to
> dynamically fixup those regions that required write permission, you'd
> have to modify the mappings for the majority of the EFI regions
> anyway. And if you're blindly allowing write permission as a fixup,
> there's not much security to be had.

I think you misunderstood my suggestion: the 'fixup' would be changing it from R-X 
to RW-, i.e. it would add 'write' permission but remove 'execute' permission.

Note that there would be no 'RWX' permission at any given moment - which is the 
dangerous combination.

> >     If that 'supposed to be' turns out to be 'not true' (not unheard of in
> >     firmware land), then plan B would be to mark pages that generate write faults 
> >     RWX as well, to not break functionality. (This 'mark it RWX' is not something 
> >     that exploits would have easy access to, and we could also generate a warning
> >     [after the EFI call has finished] if it ever triggers.)
> > 
> >     Admittedly this approach might not be without its own complications, but it 
> >     looks reasonably simple (I don't think we need per EFI call page tables, 
> >     etc.), and does not assume much about the firmware being able to enumerate its 
> >     permissions properly. Were we to merge EFI support today I'd have insisted on 
> >     trying such an approach from day 1 on.
> 
> We already have separate EFI page tables, though with the caveat that
> we share some of swapper_pg_dir's PGD entries. The best solution would
> be to stop sharing entries and isolate the EFI mappings from every
> other page table structure, so that they're only used during the EFI
> service calls.

Absolutely. Can you try to fix this for v4.3?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web