[PATCH] nx-emu: clean up nx-emu for ia32-only

[Kees Cook]

The NX-emulation should only exist for the 32 bit case, and should not be
visible under any other situation. This removes the exec-shield parameter
when not running on 32-bit x86, standardizes the x86_report_nx strings,
and sets a min/max proc handler for the exec_shield parameter.

Signed-off-by: Kees Cook <kees.cook(a)canonical.com&gt;
---
 arch/x86/mm/setup_nx.c |   12 ++++++++++--
 include/linux/sched.h  |    2 ++
 kernel/sysctl.c        |    6 +++++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index e0d9cce..f068676 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -24,7 +24,9 @@ static int __init noexec_setup(char *str)
 		disable_nx = 0;
 	} else if (!strncmp(str, "off", 3)) {
 		disable_nx = 1;
+#ifdef CONFIG_X86_32
 		exec_shield = 0;
+#endif
 	}
 	x86_configure_nx();
 	return 0;
@@ -42,12 +44,18 @@ void __cpuinit x86_configure_nx(void)
 void __init x86_report_nx(void)
 {
 	if (!cpu_has_nx) {
+#ifdef CONFIG_X86_32
 		if (exec_shield)
-			printk(KERN_INFO "Using x86 segment limits to approximate NX
protection\n");
+			printk(KERN_INFO "NX (Execute Disable) protection: "
+			       "approximated by x86 segment limits\n");
 		else
-
+			printk(KERN_INFO "NX (Execute Disable) protection: "
+			       "approximation disabled by kernel command "
+			       "line option\n");
+#else
 		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
 		       "missing in CPU or disabled in BIOS!\n");
+#endif
 	} else {
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
 		if (disable_nx) {
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2102309..5606aa7 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -101,7 +101,9 @@ struct bio_list;
 struct fs_struct;
 struct perf_event_context;
 
+#ifdef CONFIG_X86_32
 extern int exec_shield;
+#endif
 extern int print_fatal_signals;
 
 /*
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index c7f0d4a..68f020f 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -104,6 +104,7 @@ extern int sysctl_nr_open_min, sysctl_nr_open_max;
 extern int sysctl_nr_trim_pages;
 #endif
 
+#ifdef CONFIG_X86_32
 int exec_shield = 1;
 
 static int __init setup_exec_shield(char *str)
@@ -113,6 +114,7 @@ static int __init setup_exec_shield(char *str)
 	return 1;
 }
 __setup("exec-shield=", setup_exec_shield);
+#endif
 
 #ifdef CONFIG_BLOCK
 extern int blk_iopoll_enabled;
@@ -447,7 +449,9 @@ static struct ctl_table kern_table[] = {
 		.data		= &exec_shield,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= &proc_dointvec,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
 	},
 #endif
 
-- 
1.7.1


-- 
Kees Cook
Ubuntu Security Team

[Dave Jones]

On Fri, Aug 27, 2010 at 04:56:31PM -0700, Kees Cook wrote:
>  The NX-emulation should only exist for the 32 bit case, and should
> not be
>  visible under any other situation. This removes the exec-shield parameter
>  when not running on 32-bit x86, standardizes the x86_report_nx strings,
>  and sets a min/max proc handler for the exec_shield parameter. 
 
I think we should just kill the sysctl and the boot parameter completely,
and make it unconditional.

If we want a switch to disable it, we can overload disable_nx 

	Dave

[Kees Cook]

Hi Dave,

On Fri, Aug 27, 2010 at 08:18:10PM -0400, Dave Jones wrote:
>  On Fri, Aug 27, 2010 at 04:56:31PM -0700, Kees Cook wrote:
>   > The NX-emulation should only exist for the 32 bit case, and should not be
>   > visible under any other situation. This removes the exec-shield parameter
>   > when not running on 32-bit x86, standardizes the x86_report_nx strings,
>   > and sets a min/max proc handler for the exec_shield parameter.
>   
>  I think we should just kill the sysctl and the boot parameter completely,
>  and make it unconditional.
>  
>  If we want a switch to disable it, we can overload disable_nx  
How does this look? I haven't done a build test yet...

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index aedc466..39e0381 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -828,7 +828,7 @@ static void __cpuinit identify_cpu(struct cpuinfo_x86 *c)
 	 *  If we have either disabled exec-shield on the boot command line,
 	 *  or we have NX, then we don't need to do this.
 	 */
-	if (exec_shield != 0) {
+	if (!disable_nx) {
 #ifdef CONFIG_X86_PAE
 		if (!test_cpu_cap(c, X86_FEATURE_NX))
 #endif
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index e0d9cce..6096e70 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -6,7 +6,7 @@
 #include <asm/pgtable.h>
 #include <asm/proto.h>
 
-static int disable_nx __cpuinitdata;
+int disable_nx __cpuinitdata;
 
 /*
  * noexec = on|off
@@ -24,7 +24,6 @@ static int __init noexec_setup(char *str)
 		disable_nx = 0;
 	} else if (!strncmp(str, "off", 3)) {
 		disable_nx = 1;
-		exec_shield = 0;
 	}
 	x86_configure_nx();
 	return 0;
@@ -42,12 +41,18 @@ void __cpuinit x86_configure_nx(void)
 void __init x86_report_nx(void)
 {
 	if (!cpu_has_nx) {
-		if (exec_shield)
-			printk(KERN_INFO "Using x86 segment limits to approximate NX
protection\n");
+#ifdef CONFIG_X86_32
+		if (disable_nx)
+			printk(KERN_INFO "NX (Execute Disable) protection: "
+			       "approximation disabled by kernel command "
+			       "line option\n");
 		else
-
+			printk(KERN_INFO "NX (Execute Disable) protection: "
+			       "approximated by x86 segment limits\n");
+#else
 		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
 		       "missing in CPU or disabled in BIOS!\n");
+#endif
 	} else {
 #if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
 		if (disable_nx) {
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 65e871f..0f464bb 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -722,7 +722,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs
*regs)
 	 * Turn off the CS limit completely if exec-shield disabled or
 	 * NX active:
 	 */
-	if (!exec_shield || executable_stack != EXSTACK_DISABLE_X || (__supported_pte_mask &
_PAGE_NX))
+	if (disable_nx || executable_stack != EXSTACK_DISABLE_X || (__supported_pte_mask &
_PAGE_NX))
 		arch_add_exec_range(current->mm, -1);
 #endif
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2102309..5ae0dce 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -101,7 +101,9 @@ struct bio_list;
 struct fs_struct;
 struct perf_event_context;
 
-extern int exec_shield;
+#ifdef CONFIG_X86_32
+extern int disable_nx;
+#endif
 extern int print_fatal_signals;
 
 /*
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index c7f0d4a..62a5a54 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -104,16 +104,6 @@ extern int sysctl_nr_open_min, sysctl_nr_open_max;
 extern int sysctl_nr_trim_pages;
 #endif
 
-int exec_shield = 1;
-
-static int __init setup_exec_shield(char *str)
-{
-	get_option(&str, &exec_shield);
-
-	return 1;
-}
-__setup("exec-shield=", setup_exec_shield);
-
 #ifdef CONFIG_BLOCK
 extern int blk_iopoll_enabled;
 #endif
@@ -441,15 +431,6 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
-#ifdef CONFIG_X86_32
-	{
-		.procname	= "exec-shield",
-		.data		= &exec_shield,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= &proc_dointvec,
-	},
-#endif
 
 #ifdef CONFIG_PROC_SYSCTL
 	{

-- 
Kees Cook
Ubuntu Security Team

[Roland McGrath]

Dave beat me to the punch, but I was going to say the same thing.  The only
thing we need with the exec-shield option/sysctl is for it to go away.  If
anybody needs more configurability, it can be something like "noexec=emul"
to ignore NX hardware to test out the segmentation hack, or "noexec=noemul"
to only use real hardware support if it's there and never do segmentation.
But aside from convenience of smoke-testing the segmentation hack on
current (NX-capable) hardware (without tweaking the firmware to suppress it
or whatever)--which really only benefits us and we don't seem to care--I
have never heard of any users asking for any flexibility on this option.

Your patch looks fine to me, though I of course didn't test it either.


Thanks,
Roland