x86/kaiser: Boot time disabling and debug support

PATCH In review Thomas Gleixner 9 participants 32 messages Nov 26, 2017 – Nov 29, 2017

Subsystems (5)

Memory management x86 Security (LSM) x86 MM +1 more subsystems

lore

Share & tools

lore

Thomas Gleixner

author

2017-11-26 23:25 UTC #0 lore raw

v2 cover x86/kaiser: Boot time disabling and debug support

This patch series applies on top of

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git WIP.x86/mm

It contains the following updates:

- Don't set NX/PAGE_GLOBAL unconditionally

- Get rid of the compile time PAGE_GLOBAL disabling

- Add debug support for WX mappings in the KAISER shadow table

- Provide debug files to dump the kernel and the user page table for the current task.

- Add a boot time switch to disable KAISER. This does not yet take care of the 8k PGD allocations, but that can be done on top.

Changes vs. V1:

- Prevent setting PAGE_GLOBAL/NX when not supported or disabled

- Restructured the debug stuff a bit

- Extended the boot time disable to debug stuff

I tried to reenable paravirt by disabling kaiser at boot time when XEN_PV is detected. XEN_PV is the only one having CR3 access paravirtualized, which will explode nicely in the enter/exit code.

But enabling KAISER has some weird not yet debugged side effects even on KVM guests. Will look at that tomorrow morning.

Thanks,

tglx

---
 arch/x86/entry/calling.h             |    7 +++
 arch/x86/include/asm/kaiser.h        |   10 ++++
 arch/x86/include/asm/pgtable.h       |    1 
 arch/x86/include/asm/pgtable_64.h    |    9 +++
 arch/x86/include/asm/pgtable_types.h |   16 ------
 arch/x86/mm/debug_pagetables.c       |   81 ++++++++++++++++++++++++++++++++---
 arch/x86/mm/dump_pagetables.c        |   32 +++++++++++--
 arch/x86/mm/init.c                   |   14 ++++--
 arch/x86/mm/kaiser.c                 |   42 +++++++++++++++++-
 arch/x86/mm/pageattr.c               |   16 +++---
 security/Kconfig                     |    2 
 11 files changed, 190 insertions(+), 40 deletions(-)

Thomas Gleixner

author

2017-11-26 23:25 UTC #1 lore raw

PATCH v2 1/5 x86/kaiser: Respect disabled CPU features

PAGE_NX and PAGE_GLOBAL might be not supported or disabled on the command line, but KAISER sets them unconditionally.

Add proper protection against that.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/include/asm/pgtable_64.h |    3 ++-
 arch/x86/mm/kaiser.c              |   12 +++++++++++-
 2 files changed, 13 insertions(+), 2 deletions(-)

arch/x86/include/asm/pgtable_64.h +2 -1

@@ -222,7 +222,8 @@ static inline pgd_t kaiser_set_shadow_pg
 			 * wrong CR3 value, userspace will crash
 			 * instead of running.
 			 */
-			pgd.pgd |= _PAGE_NX;
+			if (__supported_pte_mask & _PAGE_NX)
+				pgd.pgd |= _PAGE_NX;
 		}
 	} else if (pgd_userspace_access(*pgdp)) {
 		/*

arch/x86/mm/kaiser.c +11 -1

@@ -42,6 +42,8 @@
 
 #define KAISER_WALK_ATOMIC  0x1
 
+static pteval_t kaiser_pte_mask __ro_after_init = ~(_PAGE_NX | _PAGE_GLOBAL);
+
 /*
  * At runtime, the only things we map are some things for CPU
  * hotplug, and stacks for new processes.  No two CPUs will ever

@@ -244,11 +246,14 @@ static pte_t *kaiser_shadow_pagetable_wa
 int kaiser_add_user_map(const void *__start_addr, unsigned long size,
 			unsigned long flags)
 {
-	pte_t *pte;
 	unsigned long start_addr = (unsigned long)__start_addr;
 	unsigned long address = start_addr & PAGE_MASK;
 	unsigned long end_addr = PAGE_ALIGN(start_addr + size);
 	unsigned long target_address;
+	pte_t *pte;
+
+	/* Clear not supported bits */
+	flags &= kaiser_pte_mask;
 
 	for (; address < end_addr; address += PAGE_SIZE) {
 		target_address = get_pa_from_kernel_map(address);

@@ -308,6 +313,11 @@ static void __init kaiser_init_all_pgds(
 	pgd_t *pgd;
 	int i;
 
+	if (__supported_pte_mask & _PAGE_NX)
+		kaiser_pte_mask |= _PAGE_NX;
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		kaiser_pte_mask |= _PAGE_GLOBAL;
+
 	pgd = kernel_to_shadow_pgdp(pgd_offset_k(0UL));
 	for (i = PTRS_PER_PGD / 2; i < PTRS_PER_PGD; i++) {
 		/*

Peter Zijlstra

maintainer

2017-11-27 09:57 UTC #2 lore raw

Re: [patch V2 1/5] x86/kaiser: Respect disabled CPU features

Quoted 2 lines · Thomas Gleixner

PAGE_NX and PAGE_GLOBAL might be not supported or disabled on the command line, but KAISER sets them unconditionally.

So KAISER is x86_64 only, right? AFAIK there is no x86_64 without NX support. So would it not make sense to mandate NX for KAISER?, that is instead of making "noexec" + KAISER work, make "noexec" kill KAISER + emit a warning.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Thomas Gleixner

author

2017-11-27 11:48 UTC #3 lore raw

Re: [patch V2 1/5] x86/kaiser: Respect disabled CPU features

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Thomas Gleixner
PAGE_NX and PAGE_GLOBAL might be not supported or disabled on the command line, but KAISER sets them unconditionally.
So KAISER is x86_64 only, right? AFAIK there is no x86_64 without NX support. So would it not make sense to mandate NX for KAISER?, that is instead of making "noexec" + KAISER work, make "noexec" kill KAISER + emit a warning.

OTOH, disabling NX is a simple way to verify that DEBUG_WX works correctly also on the shadow maps.

But surely we can drop the PAGE_GLOBAL thing, as all 64bit systems have it.

Thanks,

tglx

Brian Gerst

2017-11-27 12:31 UTC #4 lore raw

Re: [patch V2 1/5] x86/kaiser: Respect disabled CPU features

Quoted 10 lines · Thomas Gleixner

Quoted 8 lines · Peter Zijlstra
Quoted 2 lines · Thomas Gleixner
PAGE_NX and PAGE_GLOBAL might be not supported or disabled on the command line, but KAISER sets them unconditionally.
So KAISER is x86_64 only, right? AFAIK there is no x86_64 without NX support. So would it not make sense to mandate NX for KAISER?, that is instead of making "noexec" + KAISER work, make "noexec" kill KAISER + emit a warning.

Quoted 4 lines

OTOH, disabling NX is a simple way to verify that DEBUG_WX works correctly also on the shadow maps.
But surely we can drop the PAGE_GLOBAL thing, as all 64bit systems have it.

I seem to recall that some virtualized environments (maybe Xen?) don't support global pages.

--
Brian Gerst

Thomas Gleixner

author

2017-11-27 13:18 UTC #5 lore raw

Re: [patch V2 1/5] x86/kaiser: Respect disabled CPU features

Quoted 11 lines · Brian Gerst

Quoted 10 lines · Thomas Gleixner
Quoted 8 lines · Peter Zijlstra
Quoted 2 lines · Thomas Gleixner
PAGE_NX and PAGE_GLOBAL might be not supported or disabled on the command line, but KAISER sets them unconditionally.
So KAISER is x86_64 only, right? AFAIK there is no x86_64 without NX support. So would it not make sense to mandate NX for KAISER?, that is instead of making "noexec" + KAISER work, make "noexec" kill KAISER + emit a warning.

Quoted 7 lines

Quoted 4 lines
OTOH, disabling NX is a simple way to verify that DEBUG_WX works correctly also on the shadow maps.
But surely we can drop the PAGE_GLOBAL thing, as all 64bit systems have it.
I seem to recall that some virtualized environments (maybe Xen?) don't support global pages.

Uuurg. Ok, we leave it as is for now. Better safe than sorry. It does no harm.

Thanks,

tglx

Dave Hansen

maintainer

2017-11-27 18:11 UTC #6 lore raw

Re: [patch V2 1/5] x86/kaiser: Respect disabled CPU features

In reply to Thomas Gleixner

arch/x86/include/asm/pgtable_64.h static inline pgd_t kaiser_set_shadow_pg · line 227

Quoted 10 lines

arch/x86/include/asm/pgtable_64.h +2 -1

@@ -222,7 +222,8 @@ static inline pgd_t kaiser_set_shadow_pg
 			 * wrong CR3 value, userspace will crash
 			 * instead of running.
 			 */
-			pgd.pgd |= _PAGE_NX;
+			if (__supported_pte_mask & _PAGE_NX)
+				pgd.pgd |= _PAGE_NX;
 		}

Thanks for catching that. It's definitely a bug. Although, practically, it's hard to hit, right? I think everything 64-bit supports NX unless the hypervisor disabled it or something.

arch/x86/mm/kaiser.c line 45

Quoted 9 lines

arch/x86/include/asm/pgtable_64.h +0 -0

 	} else if (pgd_userspace_access(*pgdp)) {
 		/*

arch/x86/mm/kaiser.c +1 -0

@@ -42,6 +42,8 @@
 
 #define KAISER_WALK_ATOMIC  0x1
 
+static pteval_t kaiser_pte_mask __ro_after_init = ~(_PAGE_NX | _PAGE_GLOBAL);

Do we need a comment on this, like:

/*
* The NX and GLOBAL bits are not supported on all CPUs.
* We will add them back to this mask at runtime in
* kaiser_init_all_pgds() if supported.
*/

Quoted 6 lines

arch/x86/mm/kaiser.c +0 -0

 /*
  * At runtime, the only things we map are some things for CPU
  * hotplug, and stacks for new processes.  No two CPUs will ever

@@ -244,11 +246,14 @@ static pte_t *kaiser_shadow_pagetable_wa
 int kaiser_add_user_map(const void *__start_addr, unsigned long size,
 			unsigned long flags)

arch/x86/mm/kaiser.c static pte_t *kaiser_shadow_pagetable_wa · line 256

Quoted 10 lines

arch/x86/mm/kaiser.c +4 -1

 {
-	pte_t *pte;
 	unsigned long start_addr = (unsigned long)__start_addr;
 	unsigned long address = start_addr & PAGE_MASK;
 	unsigned long end_addr = PAGE_ALIGN(start_addr + size);
 	unsigned long target_address;
+	pte_t *pte;
+
+	/* Clear not supported bits */
+	flags &= kaiser_pte_mask;

Should we be warning on these if we clear them? Seems kinda funky to silently fix them up.

arch/x86/mm/kaiser.c static void __init kaiser_init_all_pgds( · line 319

Quoted 10 lines

arch/x86/mm/kaiser.c +4 -0

 	for (; address < end_addr; address += PAGE_SIZE) {
 		target_address = get_pa_from_kernel_map(address);

@@ -308,6 +313,11 @@ static void __init kaiser_init_all_pgds(
 	pgd_t *pgd;
 	int i;
 
+	if (__supported_pte_mask & _PAGE_NX)
+		kaiser_pte_mask |= _PAGE_NX;
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		kaiser_pte_mask |= _PAGE_GLOBAL;

Practically, I guess boot_cpu_has(X86_FEATURE_PGE) == (cr4_read() & X86_CR4_PGE). But, in a slow path like this, is it perhaps better to just be checking CR4 directly?

Looks functionally fine to me, though. Feel free to add my Reviewed-by or Acked-by.

Kees Cook

2017-11-27 18:37 UTC #7 lore raw

Re: [patch V2 1/5] x86/kaiser: Respect disabled CPU features

Quoted 6 lines · Dave Hansen

Quoted 6 lines

arch/x86/include/asm/pgtable_64.h +0 -0

@@ -222,7 +222,8 @@ static inline pgd_t kaiser_set_shadow_pg
                       * wrong CR3 value, userspace will crash
                       * instead of running.
                       */

Quoted 8 lines

arch/x86/include/asm/pgtable_64.h static inline pgd_t kaiser_set_shadow_pg · line 227
Quoted 4 lines
arch/x86/include/asm/pgtable_64.h +2 -1
-                     pgd.pgd |= _PAGE_NX;
+                     if (__supported_pte_mask & _PAGE_NX)
+                             pgd.pgd |= _PAGE_NX;
              }
Thanks for catching that. It's definitely a bug. Although, practically, it's hard to hit, right? I think everything 64-bit supports NX unless the hypervisor disabled it or something.

There was a very narrow window where x86_64 machines were made without NX. :( This is reflected in x86_report_nx(), though maybe we should add a "OMG, why?" when 64-bit but no NX. ;)

-Kees

Signature · 8 lines

Kees Cook
Pixel Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Thomas Gleixner

author

2017-11-26 23:25 UTC #8 lore raw

PATCH v2 2/5 x86/kaiser: Simplify disabling of global pages

In reply to Thomas Gleixner

The current way of disabling global pages at compile time prevents boot time disabling of kaiser and creates unnecessary indirections.

Global pages can be supressed by __supported_pte_mask as well. The shadow mappings set PAGE_GLOBAL for the minimal kernel mappings which are required for entry/exit. These mappings are setup manually so the filtering does not take place.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/include/asm/pgtable_types.h |   16 +---------------
 arch/x86/mm/init.c                   |   13 ++++++++++---
 arch/x86/mm/pageattr.c               |   16 ++++++++--------
 3 files changed, 19 insertions(+), 26 deletions(-)

arch/x86/include/asm/pgtable_types.h +1 ~~-15~~

@@ -191,23 +191,9 @@ enum page_cache_mode {
 #define PAGE_READONLY_EXEC	__pgprot(_PAGE_PRESENT | _PAGE_USER |	\
 					 _PAGE_ACCESSED)
 
-/*
- * Disable global pages for anything using the default
- * __PAGE_KERNEL* macros.
- *
- * PGE will still be enabled and _PAGE_GLOBAL may still be used carefully
- * for a few selected kernel mappings which must be visible to userspace,
- * when KAISER is enabled, like the entry/exit code and data.
- */
-#ifdef CONFIG_KAISER
-#define __PAGE_KERNEL_GLOBAL	0
-#else
-#define __PAGE_KERNEL_GLOBAL	_PAGE_GLOBAL
-#endif
-
 #define __PAGE_KERNEL_EXEC						\
 	(_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED |	\
-	 __PAGE_KERNEL_GLOBAL)
+	 _PAGE_GLOBAL)
 #define __PAGE_KERNEL		(__PAGE_KERNEL_EXEC | _PAGE_NX)
 
 #define __PAGE_KERNEL_RO		(__PAGE_KERNEL & ~_PAGE_RW)

arch/x86/mm/init.c +10 -3

@@ -161,6 +161,13 @@ struct map_range {
 
 static int page_size_mask;
 
+static void enable_global_pages(void)
+{
+#ifndef CONFIG_KAISER
+	__supported_pte_mask |= _PAGE_GLOBAL;
+#endif
+}
+
 static void __init probe_page_size_mask(void)
 {
 	/*

@@ -179,11 +186,11 @@ static void __init probe_page_size_mask(
 		cr4_set_bits_and_update_boot(X86_CR4_PSE);
 
 	/* Enable PGE if available */
+	__supported_pte_mask |= _PAGE_GLOBAL;
 	if (boot_cpu_has(X86_FEATURE_PGE)) {
 		cr4_set_bits_and_update_boot(X86_CR4_PGE);
-		__supported_pte_mask |= _PAGE_GLOBAL;
-	} else
-		__supported_pte_mask &= ~_PAGE_GLOBAL;
+		enable_global_pages();
+	}
 
 	/* Enable 1 GB linear kernel mappings if available: */
 	if (direct_gbpages && boot_cpu_has(X86_FEATURE_GBPAGES)) {

arch/x86/mm/pageattr.c +8 -8

@@ -585,9 +585,9 @@ try_preserve_large_page(pte_t *kpte, uns
 	 * for the ancient hardware that doesn't support it.
 	 */
 	if (pgprot_val(req_prot) & _PAGE_PRESENT)
-		pgprot_val(req_prot) |= _PAGE_PSE | __PAGE_KERNEL_GLOBAL;
+		pgprot_val(req_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
 	else
-		pgprot_val(req_prot) &= ~(_PAGE_PSE | __PAGE_KERNEL_GLOBAL);
+		pgprot_val(req_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
 
 	req_prot = canon_pgprot(req_prot);

@@ -705,9 +705,9 @@ static int
 	 * for the ancient hardware that doesn't support it.
 	 */
 	if (pgprot_val(ref_prot) & _PAGE_PRESENT)
-		pgprot_val(ref_prot) |= __PAGE_KERNEL_GLOBAL;
+		pgprot_val(ref_prot) |= _PAGE_GLOBAL;
 	else
-		pgprot_val(ref_prot) &= ~__PAGE_KERNEL_GLOBAL;
+		pgprot_val(ref_prot) &= ~_PAGE_GLOBAL;
 
 	/*
 	 * Get the target pfn from the original entry:

@@ -938,9 +938,9 @@ static void populate_pte(struct cpa_data
 	 * support it.
 	 */
 	if (pgprot_val(pgprot) & _PAGE_PRESENT)
-		pgprot_val(pgprot) |= __PAGE_KERNEL_GLOBAL;
+		pgprot_val(pgprot) |= _PAGE_GLOBAL;
 	else
-		pgprot_val(pgprot) &= ~__PAGE_KERNEL_GLOBAL;
+		pgprot_val(pgprot) &= ~_PAGE_GLOBAL;
 
 	pgprot = canon_pgprot(pgprot);

@@ -1242,9 +1242,9 @@ static int __change_page_attr(struct cpa
 		 * support it.
 		 */
 		if (pgprot_val(new_prot) & _PAGE_PRESENT)
-			pgprot_val(new_prot) |= __PAGE_KERNEL_GLOBAL;
+			pgprot_val(new_prot) |= _PAGE_GLOBAL;
 		else
-			pgprot_val(new_prot) &= ~__PAGE_KERNEL_GLOBAL;
+			pgprot_val(new_prot) &= ~_PAGE_GLOBAL;
 
 		/*
 		 * We need to keep the pfn from the existing PTE,

Thomas Gleixner

author

2017-11-27 11:49 UTC #9 lore raw

Re: [patch V2 2/5] x86/kaiser: Simplify disabling of global pages

arch/x86/mm/init.c static void __init probe_page_size_mask( · line 189

Quoted 6 lines · Thomas Gleixner

arch/x86/mm/init.c +1 -0

/*

@@ -179,11 +186,11 @@ static void __init probe_page_size_mask(
 		cr4_set_bits_and_update_boot(X86_CR4_PSE);
 
 	/* Enable PGE if available */
+	__supported_pte_mask |= _PAGE_GLOBAL;

Bah. Late night reject fixup wreckage. That wants to be

	__supported_pte_mask &= ~_PAGE_GLOBAL;

Thanks,

tglx

Dave Hansen

maintainer

2017-11-27 18:15 UTC #10 lore raw

Re: [patch V2 2/5] x86/kaiser: Simplify disabling of global pages

In reply to Thomas Gleixner

Quoted 12 lines · Thomas Gleixner

arch/x86/mm/init.c +7 -0

+static void enable_global_pages(void)
+{
+#ifndef CONFIG_KAISER
+	__supported_pte_mask |= _PAGE_GLOBAL;
+#endif
+}
+
 static void __init probe_page_size_mask(void)
 {
 	/*

@@ -179,11 +186,11 @@ static void __init probe_page_size_mask(
 		cr4_set_bits_and_update_boot(X86_CR4_PSE);

arch/x86/mm/init.c static void __init probe_page_size_mask( · line 193

Quoted 10 lines

arch/x86/mm/init.c +3 -3

 
 	/* Enable PGE if available */
+	__supported_pte_mask |= _PAGE_GLOBAL;
 	if (boot_cpu_has(X86_FEATURE_PGE)) {
 		cr4_set_bits_and_update_boot(X86_CR4_PGE);
-		__supported_pte_mask |= _PAGE_GLOBAL;
-	} else
-		__supported_pte_mask &= ~_PAGE_GLOBAL;
+		enable_global_pages();
+	}

This looks a little funky. Doesn't this or _PAGE_GLOBAL into __supported_pte_mask twice? Once before the if(), and then a second time via enable_global_pages() inside the if?

Did you intend for it to be masked *out* in the first one and then or'd back in via enable_global_pages()?

Thomas Gleixner

author

2017-11-27 20:28 UTC #11 lore raw

Re: [patch V2 2/5] x86/kaiser: Simplify disabling of global pages

Quoted 28 lines · Dave Hansen

Quoted 12 lines · Thomas Gleixner
arch/x86/mm/init.c +7 -0
+static void enable_global_pages(void)
+{
+#ifndef CONFIG_KAISER
+	__supported_pte_mask |= _PAGE_GLOBAL;
+#endif
+}
+
 static void __init probe_page_size_mask(void)
 {
 	/*
@@ -179,11 +186,11 @@ static void __init probe_page_size_mask(
 		cr4_set_bits_and_update_boot(X86_CR4_PSE);
arch/x86/mm/init.c static void __init probe_page_size_mask( · line 193
Quoted 10 lines
arch/x86/mm/init.c +3 -3
 
 	/* Enable PGE if available */
+	__supported_pte_mask |= _PAGE_GLOBAL;
 	if (boot_cpu_has(X86_FEATURE_PGE)) {
 		cr4_set_bits_and_update_boot(X86_CR4_PGE);
-		__supported_pte_mask |= _PAGE_GLOBAL;
-	} else
-		__supported_pte_mask &= ~_PAGE_GLOBAL;
+		enable_global_pages();
+	}
This looks a little funky. Doesn't this or _PAGE_GLOBAL into __supported_pte_mask twice? Once before the if(), and then a second time via enable_global_pages() inside the if?

Quoted 2 lines

Did you intend for it to be masked *out* in the first one and then or'd back in via enable_global_pages()?

It's fixed already ...

Thomas Gleixner

author

2017-11-26 23:25 UTC #12 lore raw

PATCH v2 3/5 x86/dump_pagetables: Check KAISER shadow page table for WX pages

In reply to Thomas Gleixner

ptdump_walk_pgd_level_checkwx() checks the kernel page table for WX pages, but does not check the KAISER shadow page table.

Restructure the code so that dmesg output is selected by an explicit argument and not implicit via checking the pgd argument for !NULL. Add the check for the shadow page table.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/include/asm/pgtable.h |    1 +
 arch/x86/mm/debug_pagetables.c |    2 +-
 arch/x86/mm/dump_pagetables.c  |   27 ++++++++++++++++++++++-----
 3 files changed, 24 insertions(+), 6 deletions(-)

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

@@ -28,6 +28,7 @@ extern pgd_t early_top_pgt[PTRS_PER_PGD]
 int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
 
 void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
+void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd);
 void ptdump_walk_pgd_level_checkwx(void);
 
 #ifdef CONFIG_DEBUG_WX

arch/x86/mm/debug_pagetables.c +1 -1

@@ -5,7 +5,7 @@
 
 static int ptdump_show(struct seq_file *m, void *v)
 {
-	ptdump_walk_pgd_level(m, NULL);
+	ptdump_walk_pgd_level_debugfs(m, NULL);
 	return 0;
 }

arch/x86/mm/dump_pagetables.c +22 -5

@@ -447,7 +447,7 @@ static inline bool is_hypervisor_range(i
 }
 
 static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
-				       bool checkwx)
+				       bool checkwx, bool dmesg)
 {
 #ifdef CONFIG_X86_64
 	pgd_t *start = (pgd_t *) &init_top_pgt;

@@ -460,7 +460,7 @@ static void ptdump_walk_pgd_level_core(s
 
 	if (pgd) {
 		start = pgd;
-		st.to_dmesg = true;
+		st.to_dmesg = dmesg;
 	}
 
 	st.check_wx = checkwx;

@@ -498,13 +498,30 @@ static void ptdump_walk_pgd_level_core(s
 
 void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
 {
-	ptdump_walk_pgd_level_core(m, pgd, false);
+	ptdump_walk_pgd_level_core(m, pgd, false, true);
+}
+
+void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd)
+{
+	ptdump_walk_pgd_level_core(m, pgd, false, false);
+}
+EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);
+
+void ptdump_walk_shadow_pgd_level_checkwx(void)
+{
+#ifdef CONFIG_KAISER
+	pgd_t *pgd = (pgd_t *) &init_top_pgt;
+
+	pr_info("x86/mm: Checking shadow page tables\n");
+	pgd += PTRS_PER_PGD;
+	ptdump_walk_pgd_level_core(NULL, pgd, true, false);
+#endif
 }
-EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level);
 
 void ptdump_walk_pgd_level_checkwx(void)
 {
-	ptdump_walk_pgd_level_core(NULL, NULL, true);
+	ptdump_walk_pgd_level_core(NULL, NULL, true, false);
+	ptdump_walk_shadow_pgd_level_checkwx();
 }
 
 static int __init pt_dump_init(void)

Dave Hansen

maintainer

2017-11-27 18:17 UTC #13 lore raw

Re: [patch V2 3/5] x86/dump_pagetables: Check KAISER shadow page table for WX pages

arch/x86/mm/dump_pagetables.c static void ptdump_walk_pgd_level_core(s · line 519

Quoted 10 lines · Thomas Gleixner

arch/x86/mm/dump_pagetables.c +9 -0

+void ptdump_walk_shadow_pgd_level_checkwx(void)
+{
+#ifdef CONFIG_KAISER
+	pgd_t *pgd = (pgd_t *) &init_top_pgt;
+
+	pr_info("x86/mm: Checking shadow page tables\n");
+	pgd += PTRS_PER_PGD;
+	ptdump_walk_pgd_level_core(NULL, pgd, true, false);
+#endif
 }

We have the kernel_to_shadow_pgdp() function to use instead of "pgd += PTRS_PER_PGD;". Should it be used instead?

Otherwise, looks good to me.

Thomas Gleixner

author

2017-11-26 23:25 UTC #14 lore raw

PATCH v2 4/5 x86/mm/debug_pagetables: Allow dumping current pagetables

In reply to Thomas Gleixner

Add two debugfs files which allow to dump the pagetable of the current task.

current_page_tables_knl dumps the regular page table. This is the page table which is normally shared between kernel and user space. If KAISER is enabled this is the kernel space mapping.

If KAISER is enabled the second file, current_page_tables_usr, dumps the user space page table.

These files allow to verify the resulting page tables for KAISER, but even in the non KAISER case its useful to be able to inspect user space page tables of current for debugging purposes.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/include/asm/pgtable.h |    2 -
 arch/x86/mm/debug_pagetables.c |   81 +++++++++++++++++++++++++++++++++++++----
 arch/x86/mm/dump_pagetables.c  |    4 +-
 3 files changed, 79 insertions(+), 8 deletions(-)

arch/x86/include/asm/pgtable.h +1 -1

@@ -28,7 +28,7 @@ extern pgd_t early_top_pgt[PTRS_PER_PGD]
 int __init __early_make_pgtable(unsigned long address, pmdval_t pmd);
 
 void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
-void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd);
+void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool shadow);
 void ptdump_walk_pgd_level_checkwx(void);
 
 #ifdef CONFIG_DEBUG_WX

arch/x86/mm/debug_pagetables.c +75 -6

103 lines not on this page · raw message

arch/x86/mm/dump_pagetables.c +3 -1

@@ -501,8 +501,10 @@ void ptdump_walk_pgd_level(struct seq_fi
 	ptdump_walk_pgd_level_core(m, pgd, false, true);
 }
 
-void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd)
+void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool shadow)
 {
+	if (shadow)
+		pgd += PTRS_PER_PGD;
 	ptdump_walk_pgd_level_core(m, pgd, false, false);
 }
 EXPORT_SYMBOL_GPL(ptdump_walk_pgd_level_debugfs);

Peter Zijlstra

maintainer

2017-11-27 09:42 UTC #15 lore raw

Re: [patch V2 4/5] x86/mm/debug_pagetables: Allow dumping current pagetables

Quoted 13 lines · Thomas Gleixner

static int __init pt_dump_debug_init(void)
{

arch/x86/mm/debug_pagetables.c +9 -0

+	pe_knl = debugfs_create_file("kernel_page_tables", S_IRUSR, NULL, NULL,
+				     &ptdump_fops);
+	if (!pe_knl)
 		return -ENOMEM;
 
+	pe_curknl = debugfs_create_file("current_page_tables_knl", S_IRUSR,
+					NULL, NULL, &ptdump_curknl_fops);
+	if (!pe_curknl)
+		goto err;
+
+#ifdef CONFIG_KAISER

arch/x86/mm/debug_pagetables.c static const struct file_operations ptdu · line 104

Quoted 10 lines

arch/x86/mm/debug_pagetables.c +8 -0

+	pe_curusr = debugfs_create_file("current_page_tables_usr", S_IRUSR,
+					NULL, NULL, &ptdump_curusr_fops);
+	if (!pe_curusr)
+		goto err;
+#endif
 	return 0;
+err:
+	pt_dump_debug_remove_files();
+	return -ENOMEM;
 }

Could we pretty please use the octal permission thing? I can't read thise S_crap nonsense.

Ingo Molnar

2017-11-27 10:06 UTC #16 lore raw

PATCH 1/1 vfs: Add PERM_* symbolic helpers for common file mode/permissions

Quoted 20 lines · Peter Zijlstra

Quoted 19 lines · Thomas Gleixner

static int __init pt_dump_debug_init(void)
{

(quoted diff) +14 -0

+	pe_knl = debugfs_create_file("kernel_page_tables", S_IRUSR, NULL, NULL,
+				     &ptdump_fops);
+	if (!pe_knl)
 		return -ENOMEM;
 
+	pe_curknl = debugfs_create_file("current_page_tables_knl", S_IRUSR,
+					NULL, NULL, &ptdump_curknl_fops);
+	if (!pe_curknl)
+		goto err;
+
+#ifdef CONFIG_KAISER
+	pe_curusr = debugfs_create_file("current_page_tables_usr", S_IRUSR,
+					NULL, NULL, &ptdump_curusr_fops);
+	if (!pe_curusr)
+		goto err;
+#endif
 	return 0;

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +3 -0
+err:
+	pt_dump_debug_remove_files();
+	return -ENOMEM;
 }
Could we pretty please use the octal permission thing? I can't read thise S_crap nonsense.

They are completely unreadable to me too. So if we added these helpers I sent a year ago:

	https://lwn.net/Articles/696231/

Then the above could be written as:

	pe_curknl = debugfs_create_file("current_page_tables_knl", PERM_r________,
					NULL, NULL, &ptdump_curknl_fops);

... etc., which is much more readable IMHO. Not only that, it would be trivial to _write_ permission masks as well. I just wrote this:

	PERM_rw_r__r__

Which is so much more readable than "S_IRUGO|S_IWUSR" or even "0644". The former pattern is used 527 times in the kernel ...

The patch below adds it to the current kernel.

Thanks,

Ingo
---

Signed-off-by Ingo Molnar <[email protected]>

 include/linux/stat.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

include/linux/stat.h +32 -0

index 22484e44544d..fc389c3a8692 100644
@@ -6,6 +6,38 @@
 #include <asm/stat.h>
 #include <uapi/linux/stat.h>
 
+/*
+ * Human readable symbolic definitions for common
+ * file permissions:
+ */
+#define PERM_r________	0400
+#define PERM_r__r_____	0440
+#define PERM_r__r__r__	0444
+
+#define PERM_rw_______	0600
+#define PERM_rw_r_____	0640
+#define PERM_rw_r__r__	0644
+#define PERM_rw_rw_r__	0664
+#define PERM_rw_rw_rw_	0666
+
+#define PERM__w_______	0200
+#define PERM__w__w____	0220
+#define PERM__w__w__w_	0222
+
+#define PERM_r_x______	0500
+#define PERM_r_xr_x___	0550
+#define PERM_r_xr_xr_x	0555
+
+#define PERM_rwx______	0700
+#define PERM_rwxr_x___	0750
+#define PERM_rwxr_xr_x	0755
+#define PERM_rwxrwxr_x	0775
+#define PERM_rwxrwxrwx	0777
+
+#define PERM__wx______	0300
+#define PERM__wx_wx___	0330
+#define PERM__wx_wx_wx	0333
+
 #define S_IRWXUGO	(S_IRWXU|S_IRWXG|S_IRWXO)
 #define S_IALLUGO	(S_ISUID|S_ISGID|S_ISVTX|S_IRWXUGO)
 #define S_IRUGO		(S_IRUSR|S_IRGRP|S_IROTH)

Linus Torvalds

2017-11-27 19:21 UTC #17 lore raw

Re: [PATCH] vfs: Add PERM_* symbolic helpers for common file mode/permissions

include/linux/stat.h line 15

Quoted 9 lines · Ingo Molnar

include/linux/stat.h +7 -0

+/*
+ * Human readable symbolic definitions for common
+ * file permissions:
+ */
+#define PERM_r________ 0400
+#define PERM_r__r_____ 0440
+#define PERM_r__r__r__ 0444

I'm not a fan. Particularly as you have a very random set of permissions (rx and wx? Not very common), but also because it's just not that legible.

I've argued several times that we shouldn't use the defines at all. The octal format isn't any less legible than any #define I've ever seen, and is generally _more_ legible.

What's wrong with just using 0400 for "read by user"?

Linus

Ingo Molnar

2017-11-28 10:55 UTC #18 lore raw

Re: [PATCH] vfs: Add PERM_* symbolic helpers for common file mode/permissions

Quoted 15 lines · Linus Torvalds

include/linux/stat.h line 15
Quoted 9 lines · Ingo Molnar
include/linux/stat.h +7 -0
+/*
+ * Human readable symbolic definitions for common
+ * file permissions:
+ */
+#define PERM_r________ 0400
+#define PERM_r__r_____ 0440
+#define PERM_r__r__r__ 0444
I'm not a fan. Particularly as you have a very random set of permissions (rx and wx? Not very common), but also because it's just not that legible.

Quoted 5 lines

I've argued several times that we shouldn't use the defines at all. The octal format isn't any less legible than any #define I've ever seen, and is generally _more_ legible.
What's wrong with just using 0400 for "read by user"?

Yeah, the octal format isn't all that bad - at least relative to the symbolic obfuscation defines.

Thanks,

Ingo

Ingo Molnar

2017-11-28 11:12 UTC #19 lore raw

Re: [PATCH] vfs: Add PERM_* symbolic helpers for common file mode/permissions

In reply to Linus Torvalds

Quoted 13 lines · Linus Torvalds

Review
Quoted 9 lines · Ingo Molnar
(quoted diff) +7 -0
+/*
+ * Human readable symbolic definitions for common
+ * file permissions:
+ */
+#define PERM_r________ 0400
+#define PERM_r__r_____ 0440
+#define PERM_r__r__r__ 0444
I'm not a fan. Particularly as you have a very random set of permissions (rx and wx? Not very common),

So I originally created those defines based on a grep of patterns used in the kernel, and added the 'wx' variants for completeness.

We would only need a small subset. Here's a git grep based histogram of octal file permission masks used in the kernel source:

So there's literally only 14 variants used, and 0644 and 0444 make up 95% of the cases. We get the patch below if we extend these existing patterns using their natural (looking) generators to a complete group - 19 patterns that should cover all the sensible combinations.

Quoted 1 line

but also because it's just not that legible.

Fair enough.

Thanks,

Ingo

---
 include/linux/stat.h |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

include/linux/stat.h +28 -0

Index: tip/include/linux/stat.h
===================================================================
@@ -6,6 +6,34 @@
 #include <asm/stat.h>
 #include <uapi/linux/stat.h>
 
+/*
+ * Human readable symbolic definitions for common
+ * file permissions:
+ */
+#define PERM_r________	0400
+#define PERM_r__r_____	0440
+#define PERM_r__r__r__	0444
+
+#define PERM_rw_______	0600
+#define PERM_rw_r_____	0640
+#define PERM_rw_r__r__	0644
+#define PERM_rw_rw_r__	0664
+#define PERM_rw_rw_rw_	0666
+
+#define PERM__w_______	0200
+#define PERM__w__w____	0220
+#define PERM__w__w__w_	0222
+
+#define PERM_r_x______	0500
+#define PERM_r_xr_x___	0550
+#define PERM_r_xr_xr_x	0555
+
+#define PERM_rwx______	0700
+#define PERM_rwxr_x___	0750
+#define PERM_rwxr_xr_x	0755
+#define PERM_rwxrwxr_x	0775
+#define PERM_rwxrwxrwx	0777
+
 #define S_IRWXUGO	(S_IRWXU|S_IRWXG|S_IRWXO)
 #define S_IALLUGO	(S_ISUID|S_ISGID|S_ISVTX|S_IRWXUGO)
 #define S_IRUGO		(S_IRUSR|S_IRGRP|S_IROTH)

Michael Ellerman

2017-11-29 08:52 UTC #20 lore raw

Re: [PATCH] vfs: Add PERM_* symbolic helpers for common file mode/permissions

Ingo Molnar <[email protected]> writes:
...

Quoted 25 lines

Index: tip/include/linux/stat.h
===================================================================

include/linux/stat.h +17 -0

@@ -6,6 +6,34 @@
 #include <asm/stat.h>
 #include <uapi/linux/stat.h>
 
+/*
+ * Human readable symbolic definitions for common
+ * file permissions:
+ */
+#define PERM_r________	0400
+#define PERM_r__r_____	0440
+#define PERM_r__r__r__	0444
+
+#define PERM_rw_______	0600
+#define PERM_rw_r_____	0640
+#define PERM_rw_r__r__	0644
+#define PERM_rw_rw_r__	0664
+#define PERM_rw_rw_rw_	0666
+
+#define PERM__w_______	0200
+#define PERM__w__w____	0220
+#define PERM__w__w__w_	0222

include/linux/stat.h line 35

Quoted 10 lines

include/linux/stat.h +10 -0

+
+#define PERM_r_x______	0500
+#define PERM_r_xr_x___	0550
+#define PERM_r_xr_xr_x	0555
+
+#define PERM_rwx______	0700
+#define PERM_rwxr_x___	0750
+#define PERM_rwxr_xr_x	0755
+#define PERM_rwxrwxr_x	0775
+#define PERM_rwxrwxrwx	0777

I see what you're trying to do with all the explicit underscores, but it does make them look kinda ugly.

What if you just used underscores to separate the user/group/other, and the unset permission bits are just omitted.

Then the two most common cases would be:

PERM_rw_r_r
PERM_r_r_r

Both of those read nicely I think. ie. the first is "perm read write, read, read".

Full set would be:

#define PERM_r 0400
#define PERM_r_r 0440
#define PERM_r_r_r 0444

#define PERM_rw 0600
#define PERM_rw_r 0640
#define PERM_rw_r_r 0644
#define PERM_rw_rw_r 0664
#define PERM_rw_rw_rw 0666

#define PERM_w 0200
#define PERM_w_w 0220
#define PERM_w_w_w 0222

#define PERM_rx 0500
#define PERM_rx_rx 0550
#define PERM_rx_rx_rx 0555

#define PERM_rwx 0700
#define PERM_rwx_rx 0750
#define PERM_rwx_rx_rx 0755
#define PERM_rwx_rwx_rx 0775
#define PERM_rwx_rwx_rwx 0777

cheers

Dave Hansen

maintainer

2017-11-27 18:18 UTC #21 lore raw

Re: [patch V2 4/5] x86/mm/debug_pagetables: Allow dumping current pagetables

In reply to Thomas Gleixner

arch/x86/mm/dump_pagetables.c void ptdump_walk_pgd_level(struct seq_fi · line 509

Quoted 7 lines · Thomas Gleixner

arch/x86/mm/dump_pagetables.c +3 -1

-void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd)
+void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool shadow)
 {
+	if (shadow)
+		pgd += PTRS_PER_PGD;
 	ptdump_walk_pgd_level_core(m, pgd, false, false);
 }

Same comment about calculating the shadow pgd.

Thomas Gleixner

author

2017-11-26 23:26 UTC #22 lore raw

PATCH v2 5/5 x86/kaiser: Add boottime disable switch

In reply to Thomas Gleixner

KAISER comes with overhead. The most expensive part is the CR3 switching in the entry code.

Add a command line parameter which allows to disable KAISER at boot time.

Most code pathes simply check a variable, but the entry code uses a static branch. The other code pathes cannot use a static branch because they are used before jump label patching is possible. Not an issue as the code pathes are not so performance sensitive as the entry/exit code.

This makes KAISER depend on JUMP_LABEL and on a GCC which supports it, but that's a resonable requirement.

The PGD allocation is still 8k when CONFIG_KAISER is enabled. This can be addressed on top of this.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/entry/calling.h          |    7 +++++++
 arch/x86/include/asm/kaiser.h     |   10 ++++++++++
 arch/x86/include/asm/pgtable_64.h |    6 ++++++
 arch/x86/mm/dump_pagetables.c     |    5 ++++-
 arch/x86/mm/init.c                |    7 ++++---
 arch/x86/mm/kaiser.c              |   30 ++++++++++++++++++++++++++++++
 security/Kconfig                  |    2 +-
 7 files changed, 62 insertions(+), 5 deletions(-)

arch/x86/entry/calling.h +7 -0

@@ -210,18 +210,23 @@ For 32-bit we have the following convent
 .endm
 
 .macro SWITCH_TO_KERNEL_CR3 scratch_reg:req
+	STATIC_JUMP_IF_FALSE .Lend_\@, kaiser_enabled_key, def=1
 	mov	%cr3, \scratch_reg
 	ADJUST_KERNEL_CR3 \scratch_reg
 	mov	\scratch_reg, %cr3
+.Lend_\@:
 .endm
 
 .macro SWITCH_TO_USER_CR3 scratch_reg:req
+	STATIC_JUMP_IF_FALSE .Lend_\@, kaiser_enabled_key, def=1
 	mov	%cr3, \scratch_reg
 	ADJUST_USER_CR3 \scratch_reg
 	mov	\scratch_reg, %cr3
+.Lend_\@:
 .endm
 
 .macro SAVE_AND_SWITCH_TO_KERNEL_CR3 scratch_reg:req save_reg:req
+	STATIC_JUMP_IF_FALSE .Ldone_\@, kaiser_enabled_key, def=1
 	movq	%cr3, %r\scratch_reg
 	movq	%r\scratch_reg, \save_reg
 	/*

@@ -244,11 +249,13 @@ For 32-bit we have the following convent
 .endm
 
 .macro RESTORE_CR3 save_reg:req
+	STATIC_JUMP_IF_FALSE .Lend_\@, kaiser_enabled_key, def=1
 	/*
 	 * The CR3 write could be avoided when not changing its value,
 	 * but would require a CR3 read *and* a scratch register.
 	 */
 	movq	\save_reg, %cr3
+.Lend_\@:
 .endm
 
 #else /* CONFIG_KAISER=n: */

arch/x86/include/asm/kaiser.h +10 -0

@@ -56,6 +56,16 @@ extern void kaiser_remove_mapping(unsign
  */
 extern void kaiser_init(void);
 
+/* True if kaiser is enabled at boot time */
+extern struct static_key_true kaiser_enabled_key;
+extern bool kaiser_enabled;
+extern void kaiser_check_cmdline(void);
+
+#else /* CONFIG_KAISER */
+
+#define kaiser_enabled		(false)
+static inline void kaiser_check_cmdline(void) { }
+
 #endif
 
 #endif /* __ASSEMBLY__ */

arch/x86/include/asm/pgtable_64.h +6 -0

@@ -175,6 +175,9 @@ static inline p4d_t *shadow_to_kernel_p4
 {
 	return ptr_clear_bit(p4dp, KAISER_PGTABLE_SWITCH_BIT);
 }
+
+extern bool kaiser_enabled;
+
 #endif /* CONFIG_KAISER */
 
 /*

@@ -208,6 +211,9 @@ static inline bool pgd_userspace_access(
 static inline pgd_t kaiser_set_shadow_pgd(pgd_t *pgdp, pgd_t pgd)
 {
 #ifdef CONFIG_KAISER
+	if (!kaiser_enabled)
+		return pgd;
+
 	if (pgd_userspace_access(pgd)) {
 		if (pgdp_maps_userspace(pgdp)) {
 			/*

arch/x86/mm/dump_pagetables.c +4 -1

@@ -20,6 +20,7 @@
 #include <linux/seq_file.h>
 
 #include <asm/pgtable.h>
+#include <asm/kaiser.h>
 
 /*
  * The dumper groups pagetable entries of the same type into one, and for

@@ -503,7 +504,7 @@ void ptdump_walk_pgd_level(struct seq_fi
 
 void ptdump_walk_pgd_level_debugfs(struct seq_file *m, pgd_t *pgd, bool shadow)
 {
-	if (shadow)
+	if (shadow && kaiser_enabled)
 		pgd += PTRS_PER_PGD;
 	ptdump_walk_pgd_level_core(m, pgd, false, false);
 }

@@ -514,6 +515,8 @@ void ptdump_walk_shadow_pgd_level_checkw
 #ifdef CONFIG_KAISER
 	pgd_t *pgd = (pgd_t *) &init_top_pgt;
 
+	if (!kaiser_enabled)
+		return;
 	pr_info("x86/mm: Checking shadow page tables\n");
 	pgd += PTRS_PER_PGD;
 	ptdump_walk_pgd_level_core(NULL, pgd, true, false);

arch/x86/mm/init.c +4 -3

@@ -20,6 +20,7 @@
 #include <asm/kaslr.h>
 #include <asm/hypervisor.h>
 #include <asm/cpufeature.h>
+#include <asm/kaiser.h>
 
 /*
  * We need to define the tracepoints somewhere, and tlb.c

@@ -163,9 +164,8 @@ static int page_size_mask;
 
 static void enable_global_pages(void)
 {
-#ifndef CONFIG_KAISER
-	__supported_pte_mask |= _PAGE_GLOBAL;
-#endif
+	if (!kaiser_enabled)
+		__supported_pte_mask |= _PAGE_GLOBAL;
 }
 
 static void __init probe_page_size_mask(void)

@@ -656,6 +656,7 @@ void __init init_mem_mapping(void)
 {
 	unsigned long end;
 
+	kaiser_check_cmdline();
 	probe_page_size_mask();
 	setup_pcid();

arch/x86/mm/kaiser.c +30 -0

@@ -34,6 +34,7 @@
 #include <linux/mm.h>
 #include <linux/uaccess.h>
 
+#include <asm/cmdline.h>
 #include <asm/kaiser.h>
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>

@@ -44,6 +45,16 @@
 
 static pteval_t kaiser_pte_mask __ro_after_init = ~(_PAGE_NX | _PAGE_GLOBAL);
 
+/* Global flag for boot time kaiser enable/disable */
+bool kaiser_enabled __ro_after_init = true;
+DEFINE_STATIC_KEY_TRUE(kaiser_enabled_key);
+
+void __init kaiser_check_cmdline(void)
+{
+	if (cmdline_find_option_bool(boot_command_line, "nokaiser"))
+		kaiser_enabled = false;
+}
+
 /*
  * At runtime, the only things we map are some things for CPU
  * hotplug, and stacks for new processes.  No two CPUs will ever

@@ -252,6 +263,9 @@ int kaiser_add_user_map(const void *__st
 	unsigned long target_address;
 	pte_t *pte;
 
+	if (!kaiser_enabled)
+		return 0;
+
 	/* Clear not supported bits */
 	flags &= kaiser_pte_mask;

@@ -402,6 +416,9 @@ void __init kaiser_init(void)
 {
 	int cpu;
 
+	if (!kaiser_enabled)
+		return;
+
 	kaiser_init_all_pgds();
 
 	for_each_possible_cpu(cpu) {

@@ -436,6 +453,16 @@ void __init kaiser_init(void)
 	kaiser_add_mapping_cpu_entry(0);
 }
 
+static int __init kaiser_boottime_control(void)
+{
+	if (!kaiser_enabled) {
+		static_branch_disable(&kaiser_enabled_key);
+		pr_info("kaiser: Disabled on command line\n");
+	}
+	return 0;
+}
+subsys_initcall(kaiser_boottime_control);
+
 int kaiser_add_mapping(unsigned long addr, unsigned long size,
 		       unsigned long flags)
 {

@@ -446,6 +473,9 @@ void kaiser_remove_mapping(unsigned long
 {
 	unsigned long addr;
 
+	if (!kaiser_enabled)
+		return;
+
 	/* The shadow page tables always use small pages: */
 	for (addr = start; addr < start + size; addr += PAGE_SIZE) {
 		/*

security/Kconfig +1 -1

@@ -56,7 +56,7 @@ config SECURITY_NETWORK
 
 config KAISER
 	bool "Remove the kernel mapping in user mode"
-	depends on X86_64 && SMP && !PARAVIRT
+	depends on X86_64 && SMP && !PARAVIRT && JUMP_LABEL
 	help
 	  This feature reduces the number of hardware side channels by
 	  ensuring that the majority of kernel addresses are not mapped

Peter Zijlstra

maintainer

2017-11-27 09:48 UTC #23 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

Quoted 10 lines · Thomas Gleixner

KAISER comes with overhead. The most expensive part is the CR3 switching in the entry code.
Add a command line parameter which allows to disable KAISER at boot time.
Most code pathes simply check a variable, but the entry code uses a static branch. The other code pathes cannot use a static branch because they are used before jump label patching is possible. Not an issue as the code pathes are not so performance sensitive as the entry/exit code.

Quoted 5 lines

This makes KAISER depend on JUMP_LABEL and on a GCC which supports it, but that's a resonable requirement.
The PGD allocation is still 8k when CONFIG_KAISER is enabled. This can be addressed on top of this.

So in patch 15 Andy notes that we should probably also disable the SYSCALL trampoline when we disable KAISER.

https://lkml.kernel.org/r/[email protected]

Peter Zijlstra

maintainer

2017-11-27 10:22 UTC #24 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

Quoted 14 lines · Peter Zijlstra

Quoted 13 lines · Thomas Gleixner
KAISER comes with overhead. The most expensive part is the CR3 switching in the entry code.
Add a command line parameter which allows to disable KAISER at boot time.
Most code pathes simply check a variable, but the entry code uses a static branch. The other code pathes cannot use a static branch because they are used before jump label patching is possible. Not an issue as the code pathes are not so performance sensitive as the entry/exit code.
This makes KAISER depend on JUMP_LABEL and on a GCC which supports it, but that's a resonable requirement.

Quoted 7 lines

Quoted 2 lines
The PGD allocation is still 8k when CONFIG_KAISER is enabled. This can be addressed on top of this.
So in patch 15 Andy notes that we should probably also disable the SYSCALL trampoline when we disable KAISER.
https://lkml.kernel.org/r/[email protected]

Could be a simple as this.. but I've not tested.

arch/x86/kernel/cpu/common.c +4 -1

index f4f4ab8525bd..1be393a97421 100644
@@ -1442,7 +1442,10 @@ void syscall_init(void)
 		(entry_SYSCALL_64_trampoline - _entry_trampoline);
 
 	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
-	wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	if (kaiser_enabled)
+		wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	else
+		wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
 
 #ifdef CONFIG_IA32_EMULATION
 	wrmsrl(MSR_CSTAR, (unsigned long)entry_SYSCALL_compat);

Thomas Gleixner

author

2017-11-27 11:50 UTC #25 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

Quoted 18 lines · Peter Zijlstra

Quoted 17 lines · Peter Zijlstra
Quoted 15 lines · Thomas Gleixner
KAISER comes with overhead. The most expensive part is the CR3 switching in the entry code.
Add a command line parameter which allows to disable KAISER at boot time.
Most code pathes simply check a variable, but the entry code uses a static branch. The other code pathes cannot use a static branch because they are used before jump label patching is possible. Not an issue as the code pathes are not so performance sensitive as the entry/exit code.
This makes KAISER depend on JUMP_LABEL and on a GCC which supports it, but that's a resonable requirement.
The PGD allocation is still 8k when CONFIG_KAISER is enabled. This can be addressed on top of this.

Quoted 6 lines

Quoted 4 lines
So in patch 15 Andy notes that we should probably also disable the SYSCALL trampoline when we disable KAISER.
https://lkml.kernel.org/r/[email protected]
Could be a simple as this.. but I've not tested.

That's only one part of it. I think we need to fiddle with the exit side as well.

Thanks,

tglx

Quoted 17 lines

arch/x86/kernel/cpu/common.c +4 -1

index f4f4ab8525bd..1be393a97421 100644
@@ -1442,7 +1442,10 @@ void syscall_init(void)
 		(entry_SYSCALL_64_trampoline - _entry_trampoline);
 
 	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
-	wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	if (kaiser_enabled)
+		wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	else
+		wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
 
 #ifdef CONFIG_IA32_EMULATION
 	wrmsrl(MSR_CSTAR, (unsigned long)entry_SYSCALL_compat);

Peter Zijlstra

maintainer

2017-11-27 12:50 UTC #26 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

Quoted 6 lines · Thomas Gleixner

Quoted 5 lines · Peter Zijlstra
Quoted 3 lines · Peter Zijlstra
So in patch 15 Andy notes that we should probably also disable the SYSCALL trampoline when we disable KAISER.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
https://lkml.kernel.org/r/[email protected]
Could be a simple as this.. but I've not tested.
That's only one part of it. I think we need to fiddle with the exit side as well.

So I assumed that the patches were bisectable. From that I figured the exit path (patch 14 in that set) would work either way.

Peter Zijlstra

maintainer

2017-11-27 21:43 UTC #27 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

In reply to Peter Zijlstra

Quoted 9 lines · Peter Zijlstra

Could be a simple as this.. but I've not tested.
arch/x86/kernel/cpu/common.c +0 -0
index f4f4ab8525bd..1be393a97421 100644
@@ -1442,7 +1442,10 @@ void syscall_init(void)
 		(entry_SYSCALL_64_trampoline - _entry_trampoline);

arch/x86/kernel/cpu/common.c void syscall_init(void) · line 1451

Quoted 10 lines

arch/x86/kernel/cpu/common.c +4 -1

 
 	wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
-	wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	if (kaiser_enabled)
+		wrmsrl(MSR_LSTAR, SYSCALL64_entry_trampoline);
+	else
+		wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
 
 #ifdef CONFIG_IA32_EMULATION
 	wrmsrl(MSR_CSTAR, (unsigned long)entry_SYSCALL_compat);

Seems to work:

root@ivb-ep:~# rdmsr --all 0xc0000082 | uniq
ffffffff81beb780
root@ivb-ep:~# grep ffffffff81beb780 /proc/kallsyms 
ffffffff81beb780 T entry_SYSCALL_64

Dave Hansen

maintainer

2017-11-27 18:22 UTC #28 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

In reply to Thomas Gleixner

security/Kconfig config SECURITY_NETWORK · line 62

Quoted 11 lines · Thomas Gleixner

security/Kconfig +1 -1

@@ -56,7 +56,7 @@ config SECURITY_NETWORK
 
 config KAISER
 	bool "Remove the kernel mapping in user mode"
-	depends on X86_64 && SMP && !PARAVIRT
+	depends on X86_64 && SMP && !PARAVIRT && JUMP_LABEL
 	help
 	  This feature reduces the number of hardware side channels by
 	  ensuring that the majority of kernel addresses are not mapped

One of the reasons for doing the runtime-disable was to get rid of the !PARAVIRT dependency. I can add a follow-on here that will act as if we did "nokaiser" whenever Xen is in play so we can remove this dependency.

I just hope Xen is detectable early enough to do the static patching.

Thomas Gleixner

author

2017-11-27 19:00 UTC #29 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

Quoted 17 lines · Dave Hansen

security/Kconfig config SECURITY_NETWORK · line 62
Quoted 11 lines · Thomas Gleixner
security/Kconfig +1 -1
@@ -56,7 +56,7 @@ config SECURITY_NETWORK
 
 config KAISER
 	bool "Remove the kernel mapping in user mode"
-	depends on X86_64 && SMP && !PARAVIRT
+	depends on X86_64 && SMP && !PARAVIRT && JUMP_LABEL
 	help
 	  This feature reduces the number of hardware side channels by
 	  ensuring that the majority of kernel addresses are not mapped
One of the reasons for doing the runtime-disable was to get rid of the !PARAVIRT dependency. I can add a follow-on here that will act as if we did "nokaiser" whenever Xen is in play so we can remove this dependency.

Quoted 1 line

I just hope Xen is detectable early enough to do the static patching.

Yes, it is. I'm currently trying to figure out why it fails on a KVM guest.

If I boot with 'nokaiser' on the command line it works. If kaiser is runtime enabled then some early klibc user space in the ramdisk explodes. Not sure yet whats going on.

Thanks,

tglx

Josh Poimboeuf

2017-11-27 19:18 UTC #30 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

Quoted 19 lines · Thomas Gleixner

Quoted 17 lines · Dave Hansen
security/Kconfig config SECURITY_NETWORK · line 62
Quoted 11 lines · Thomas Gleixner
security/Kconfig +1 -1
@@ -56,7 +56,7 @@ config SECURITY_NETWORK
 
 config KAISER
 	bool "Remove the kernel mapping in user mode"
-	depends on X86_64 && SMP && !PARAVIRT
+	depends on X86_64 && SMP && !PARAVIRT && JUMP_LABEL
 	help
 	  This feature reduces the number of hardware side channels by
 	  ensuring that the majority of kernel addresses are not mapped
One of the reasons for doing the runtime-disable was to get rid of the !PARAVIRT dependency. I can add a follow-on here that will act as if we did "nokaiser" whenever Xen is in play so we can remove this dependency.

Quoted 7 lines

Quoted 1 line
I just hope Xen is detectable early enough to do the static patching.
Yes, it is. I'm currently trying to figure out why it fails on a KVM guest.
If I boot with 'nokaiser' on the command line it works. If kaiser is runtime enabled then some early klibc user space in the ramdisk explodes. Not sure yet whats going on.

I'm also seeing weirdness with PARAVIRT+KAISER on kvm. The symptoms aren't consistent. Sometimes it boots, sometimes it hangs before the login prompt, sometimes there are user space seg faults.

It almost seems like the interrupt handler is corrupting user space state somehow.

This is with tip/WIP.x86/mm plus a patch to remove the KAISER dependency on !PARAVIRT.

Signature · 1 line

Josh

Thomas Gleixner

author

2017-11-27 20:47 UTC #31 lore raw

Re: [patch V2 5/5] x86/kaiser: Add boottime disable switch

Quoted 32 lines · Josh Poimboeuf

Quoted 19 lines · Thomas Gleixner
Quoted 17 lines · Dave Hansen
security/Kconfig config SECURITY_NETWORK · line 62
Quoted 11 lines · Thomas Gleixner
security/Kconfig +1 -1
@@ -56,7 +56,7 @@ config SECURITY_NETWORK
 
 config KAISER
 	bool "Remove the kernel mapping in user mode"
-	depends on X86_64 && SMP && !PARAVIRT
+	depends on X86_64 && SMP && !PARAVIRT && JUMP_LABEL
 	help
 	  This feature reduces the number of hardware side channels by
 	  ensuring that the majority of kernel addresses are not mapped
One of the reasons for doing the runtime-disable was to get rid of the !PARAVIRT dependency. I can add a follow-on here that will act as if we did "nokaiser" whenever Xen is in play so we can remove this dependency.
Quoted 7 lines
Quoted 1 line
I just hope Xen is detectable early enough to do the static patching.
Yes, it is. I'm currently trying to figure out why it fails on a KVM guest.
If I boot with 'nokaiser' on the command line it works. If kaiser is runtime enabled then some early klibc user space in the ramdisk explodes. Not sure yet whats going on.
I'm also seeing weirdness with PARAVIRT+KAISER on kvm. The symptoms aren't consistent. Sometimes it boots, sometimes it hangs before the login prompt, sometimes there are user space seg faults.

Quoted 5 lines

It almost seems like the interrupt handler is corrupting user space state somehow.
This is with tip/WIP.x86/mm plus a patch to remove the KAISER dependency on !PARAVIRT.

See the patches I posted. Its the PV patching of flush_tlb_single() ...

Thanks,

tglx