[PATCH v3] x86/boot/32: Rewrite test_wp_bit()

[PATCH v3] x86/boot/32: Rewrite test_wp_bit()

[Andy Lutomirski]

This code seems to be very old and has gotten only minor updates.
It's overcomplicated and has a bunch of comments that are, at best,
of purely historical interest.  Nowadays we have a shiny function
probe_kernel_write() that does more or less exactly what we need.
Use it.

I switched the page that we test from swapper_pg_dir to
empty_zero_page because writing zero to empty_zero_page is more
obviously safe than writing to the paging structures.  (It's
extremely unlikely that any of this would cause problems in practice
because the write will fail on any supported CPU.)

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---

Take 3: rebased to tip/master

 arch/x86/mm/init_32.c | 41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index cfb98149855e..e37db9f9f883 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -56,8 +56,6 @@
 
 unsigned long highstart_pfn, highend_pfn;
 
-static noinline int do_test_wp_bit(void);
-
 bool __read_mostly __vmalloc_start_set = false;
 
 /*
@@ -726,22 +724,21 @@ void __init paging_init(void)
  */
 static void __init test_wp_bit(void)
 {
-	int wp_works_ok;
+	char z = 0;
 
 	printk(KERN_INFO
   "Checking if this processor honours the WP bit even in supervisor mode...");
 
-	/* Any page-aligned address will do, the test is non-destructive */
-	__set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_KERNEL_RO);
-	wp_works_ok = do_test_wp_bit();
-	clear_fixmap(FIX_WP_TEST);
+	__set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
 
-	if (!wp_works_ok) {
+	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {
 		printk(KERN_CONT "No.\n");
 		panic("Linux doesn't support CPUs with broken WP.");
-	} else {
-		printk(KERN_CONT "Ok.\n");
 	}
+
+	clear_fixmap(FIX_WP_TEST);
+
+	printk(KERN_CONT "Ok.\n");
 }
 
 void __init mem_init(void)
@@ -851,30 +848,6 @@ int arch_remove_memory(u64 start, u64 size)
 #endif
 #endif
 
-/*
- * This function cannot be __init, since exceptions don't work in that
- * section.  Put this after the callers, so that it cannot be inlined.
- */
-static noinline int do_test_wp_bit(void)
-{
-	char tmp_reg;
-	int flag;
-
-	__asm__ __volatile__(
-		"	movb %0, %1	\n"
-		"1:	movb %1, %0	\n"
-		"	xorl %2, %2	\n"
-		"2:			\n"
-		_ASM_EXTABLE(1b,2b)
-		:"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
-		 "=q" (tmp_reg),
-		 "=r" (flag)
-		:"2" (1)
-		:"memory");
-
-	return flag;
-}
-
 int kernel_set_to_readonly __read_mostly;
 
 void set_kernel_text_rw(void)
-- 
2.9.3

[tip:x86/mm] x86/boot/32: Rewrite test_wp_bit()

[tip-bot for Andy Lutomirski]

Commit-ID:  4af171105144a6475704c1e6024132883d50499e
Gitweb:     http://git.kernel.org/tip/4af171105144a6475704c1e6024132883d50499e
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Wed, 29 Mar 2017 16:47:35 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 30 Mar 2017 09:08:33 +0200

x86/boot/32: Rewrite test_wp_bit()

This code seems to be very old and has gotten only minor updates.
It's overcomplicated and has a bunch of comments that are, at best,
of purely historical interest.  Nowadays we have a shiny function
probe_kernel_write() that does more or less exactly what we need.
Use it.

I switched the page that we test from swapper_pg_dir to
empty_zero_page because writing zero to empty_zero_page is more
obviously safe than writing to the paging structures.  (It's
extremely unlikely that any of this would cause problems in practice
because the write will fail on any supported CPU.)

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Juergen Gross <jgross@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Garnier <thgarnie@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/0b9e64ab0236de30e7572213cea77bf95ae2e990.1490831211.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/mm/init_32.c | 41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 7116a72..097089a 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -56,8 +56,6 @@
 
 unsigned long highstart_pfn, highend_pfn;
 
-static noinline int do_test_wp_bit(void);
-
 bool __read_mostly __vmalloc_start_set = false;
 
 /*
@@ -726,22 +724,21 @@ void __init paging_init(void)
  */
 static void __init test_wp_bit(void)
 {
-	int wp_works_ok;
+	char z = 0;
 
 	printk(KERN_INFO
   "Checking if this processor honours the WP bit even in supervisor mode...");
 
-	/* Any page-aligned address will do, the test is non-destructive */
-	__set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_KERNEL_RO);
-	wp_works_ok = do_test_wp_bit();
-	clear_fixmap(FIX_WP_TEST);
+	__set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
 
-	if (!wp_works_ok) {
+	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {
 		printk(KERN_CONT "No.\n");
 		panic("Linux doesn't support CPUs with broken WP.");
-	} else {
-		printk(KERN_CONT "Ok.\n");
 	}
+
+	clear_fixmap(FIX_WP_TEST);
+
+	printk(KERN_CONT "Ok.\n");
 }
 
 void __init mem_init(void)
@@ -851,30 +848,6 @@ int arch_remove_memory(u64 start, u64 size)
 #endif
 #endif
 
-/*
- * This function cannot be __init, since exceptions don't work in that
- * section.  Put this after the callers, so that it cannot be inlined.
- */
-static noinline int do_test_wp_bit(void)
-{
-	char tmp_reg;
-	int flag;
-
-	__asm__ __volatile__(
-		"	movb %0, %1	\n"
-		"1:	movb %1, %0	\n"
-		"	xorl %2, %2	\n"
-		"2:			\n"
-		_ASM_EXTABLE(1b,2b)
-		:"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
-		 "=q" (tmp_reg),
-		 "=r" (flag)
-		:"2" (1)
-		:"memory");
-
-	return flag;
-}
-
 int kernel_set_to_readonly __read_mostly;
 
 void set_kernel_text_rw(void)

Re: [tip:x86/mm] x86/boot/32: Rewrite test_wp_bit()

[Borislav Petkov]

On Thu, Mar 30, 2017 at 12:29:18AM -0700, tip-bot for Andy Lutomirski wrote:
> Commit-ID:  4af171105144a6475704c1e6024132883d50499e
> Gitweb:     http://git.kernel.org/tip/4af171105144a6475704c1e6024132883d50499e
> Author:     Andy Lutomirski <luto@kernel.org>
> AuthorDate: Wed, 29 Mar 2017 16:47:35 -0700
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Thu, 30 Mar 2017 09:08:33 +0200
> 
> x86/boot/32: Rewrite test_wp_bit()
> 
> This code seems to be very old and has gotten only minor updates.
> It's overcomplicated and has a bunch of comments that are, at best,
> of purely historical interest.  Nowadays we have a shiny function
> probe_kernel_write() that does more or less exactly what we need.
> Use it.
> 
> I switched the page that we test from swapper_pg_dir to
> empty_zero_page because writing zero to empty_zero_page is more
> obviously safe than writing to the paging structures.  (It's
> extremely unlikely that any of this would cause problems in practice
> because the write will fail on any supported CPU.)
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Garnier <thgarnie@google.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Link: http://lkml.kernel.org/r/0b9e64ab0236de30e7572213cea77bf95ae2e990.1490831211.git.luto@kernel.org
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/mm/init_32.c | 41 +++++++----------------------------------
>  1 file changed, 7 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
> index 7116a72..097089a 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -56,8 +56,6 @@
>  
>  unsigned long highstart_pfn, highend_pfn;
>  
> -static noinline int do_test_wp_bit(void);
> -
>  bool __read_mostly __vmalloc_start_set = false;
>  
>  /*
> @@ -726,22 +724,21 @@ void __init paging_init(void)
>   */
>  static void __init test_wp_bit(void)
>  {
> -	int wp_works_ok;
> +	char z = 0;
>  
>  	printk(KERN_INFO
>    "Checking if this processor honours the WP bit even in supervisor mode...");
>  
> -	/* Any page-aligned address will do, the test is non-destructive */
> -	__set_fixmap(FIX_WP_TEST, __pa(&swapper_pg_dir), PAGE_KERNEL_RO);
> -	wp_works_ok = do_test_wp_bit();
> -	clear_fixmap(FIX_WP_TEST);
> +	__set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
>  
> -	if (!wp_works_ok) {
> +	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {

Gah, I was just commenting on that but you committed it already...

Oh well, let's flip that logic so that we have the success case - which
we exec most of the time - if not always - not out of line but put the
panic() call at the end instead. A natural "likely" if you will :-)

---
From: Borislav Petkov <bp@suse.de>
Date: Thu, 30 Mar 2017 09:44:05 +0200
Subject: [PATCH] x86/boot/32: Flip the logic in test_wp_bit()

... to have a natural "likely()" in the code flow and thus have the
success case with a branch 99.999% of the times non-taken and function
return code following it instead of jumping to it each time.

This puts the panic() call at the end of the function - it is going to
be practically unreachable anyway.

The C code is a bit more readable too.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/mm/init_32.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 097089a5e4d5..601b8e04e5c6 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -726,19 +726,18 @@ static void __init test_wp_bit(void)
 {
 	char z = 0;
 
-	printk(KERN_INFO
-  "Checking if this processor honours the WP bit even in supervisor mode...");
+	printk(KERN_INFO "Checking if this processor honours the WP bit even in supervisor mode...");
 
 	__set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
 
-	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {
-		printk(KERN_CONT "No.\n");
-		panic("Linux doesn't support CPUs with broken WP.");
+	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1)) {
+		clear_fixmap(FIX_WP_TEST);
+		printk(KERN_CONT "Ok.\n");
+		return;
 	}
 
-	clear_fixmap(FIX_WP_TEST);
-
-	printk(KERN_CONT "Ok.\n");
+	printk(KERN_CONT "No.\n");
+	panic("Linux doesn't support CPUs with broken WP.");
 }
 
 void __init mem_init(void)
-- 
2.11.0


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[tip:x86/mm] x86/boot/32: Flip the logic in test_wp_bit()

[tip-bot for Borislav Petkov]

Commit-ID:  952a6c2c094f4eda295f20c42e6e2d73735950fa
Gitweb:     http://git.kernel.org/tip/952a6c2c094f4eda295f20c42e6e2d73735950fa
Author:     Borislav Petkov <bp@suse.de>
AuthorDate: Thu, 30 Mar 2017 09:44:05 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 31 Mar 2017 08:08:31 +0200

x86/boot/32: Flip the logic in test_wp_bit()

... to have a natural "likely()" in the code flow and thus have the
success case with a branch 99.999% of the times non-taken and function
return code following it instead of jumping to it each time.

This puts the panic() call at the end of the function - it is going to
be practically unreachable anyway.

The C code is a bit more readable too.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: boris.ostrovsky@oracle.com
Cc: jgross@suse.com
Cc: thgarnie@google.com
Link: http://lkml.kernel.org/r/20170330080101.ywsf5rg6ilzu4itk@pd.tnic
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/mm/init_32.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 097089a..601b8e0 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -726,19 +726,18 @@ static void __init test_wp_bit(void)
 {
 	char z = 0;
 
-	printk(KERN_INFO
-  "Checking if this processor honours the WP bit even in supervisor mode...");
+	printk(KERN_INFO "Checking if this processor honours the WP bit even in supervisor mode...");
 
 	__set_fixmap(FIX_WP_TEST, __pa_symbol(empty_zero_page), PAGE_KERNEL_RO);
 
-	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1) == 0) {
-		printk(KERN_CONT "No.\n");
-		panic("Linux doesn't support CPUs with broken WP.");
+	if (probe_kernel_write((char *)fix_to_virt(FIX_WP_TEST), &z, 1)) {
+		clear_fixmap(FIX_WP_TEST);
+		printk(KERN_CONT "Ok.\n");
+		return;
 	}
 
-	clear_fixmap(FIX_WP_TEST);
-
-	printk(KERN_CONT "Ok.\n");
+	printk(KERN_CONT "No.\n");
+	panic("Linux doesn't support CPUs with broken WP.");
 }
 
 void __init mem_init(void)