[RFC][PATCH] x86: Make x32 syscall support conditional on a kernel parameter

[RFC][PATCH] x86: Make x32 syscall support conditional on a kernel parameter

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 6811 bytes --]

Enabling x32 in a distribution default kernel increases its attack
surface while providing no benefit to the vast majority of its users.
No-one seems interested in regularly checking for vulnerabilities
specific to x32 (at least no-one with a white hat).

Still, requiring a separate or custom configuration just to turn on
x32 seems wasteful.  And the only differences on syscall entry are
two instructions (mask out the x32 flag and compare the syscall
number).

So pad the standard comparison with a nop and add a kernel parameter
"syscall.x32" which controls whether this is replaced with the x32
version at boot time.  Add a Kconfig parameter to set the default.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This is currently used in Debian, where x32 is an unofficial port that
can be installed in parallel with amd64 (multiarch).  I acknowledge the
patch is a bit hacky, e.g. it has several magic numbers.  I'm sending
this to find out whether anything like this would be acceptable
upstream.

Ben.

 Documentation/kernel-parameters.txt |  4 ++++
 arch/x86/Kconfig                    |  8 +++++++
 arch/x86/include/asm/elf.h          |  8 ++++++-
 arch/x86/kernel/entry_64.S          | 26 ++++++++++++++++++----
 arch/x86/kernel/syscall_64.c        | 43 +++++++++++++++++++++++++++++++++++++
 5 files changed, 84 insertions(+), 5 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4c81a86..1e161b0 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3397,6 +3397,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	switches=	[HW,M68k]
 
+	syscall.x32=	[KNL,x86_64] Enable/disable use of x32 syscalls on
+			an x86_64 kernel where CONFIG_X86_X32 is enabled.
+			Default depends on CONFIG_X86_X32_DISABLED.
+
 	sysfs.deprecated=0|1 [KNL]
 			Enable/disable old style sysfs layout for old udev
 			on older distributions. When this option is enabled
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ded8a67..9627922 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2455,6 +2455,14 @@ config X86_X32
 	  elf32_x86_64 support enabled to compile a kernel with this
 	  option set.
 
+config X86_X32_DISABLED
+	bool "x32 ABI disabled by default"
+	depends on X86_X32
+	default n
+	help
+	  Disable the x32 ABI unless explicitly enabled using the
+	  kernel paramter "syscall.x32=y".
+
 config COMPAT
 	def_bool y
 	depends on IA32_EMULATION || X86_X32
diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
index ca3347a..2a8b89a 100644
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -154,6 +154,12 @@ do {						\
 
 #else /* CONFIG_X86_32 */
 
+#ifdef CONFIG_X86_X32_ABI
+extern bool x32_enabled;
+#else
+#define x32_enabled 0
+#endif
+
 /*
  * This is used to ensure we don't load something for the wrong architecture.
  */
@@ -162,7 +168,7 @@ do {						\
 
 #define compat_elf_check_arch(x)					\
 	(elf_check_arch_ia32(x) ||					\
-	 (IS_ENABLED(CONFIG_X86_X32_ABI) && (x)->e_machine == EM_X86_64))
+	 (x32_enabled && (x)->e_machine == EM_X86_64))
 
 #if __USER32_DS != __USER_DS
 # error "The following code assumes __USER32_DS == __USER_DS"
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index df088bb..50b265a 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -414,8 +414,12 @@ system_call_fastpath:
 #if __SYSCALL_MASK == ~0
 	cmpq $__NR_syscall_max,%rax
 #else
-	andl $__SYSCALL_MASK,%eax
-	cmpl $__NR_syscall_max,%eax
+	.globl system_call_fast_compare
+	.globl system_call_fast_compare_end
+system_call_fast_compare:
+	cmpq $511,%rax			/* x32 syscalls start at 512 */
+	.byte P6_NOP4
+system_call_fast_compare_end:
 #endif
 	ja ret_from_sys_call  /* and return regs->ax */
 	movq %r10,%rcx
@@ -520,8 +524,12 @@ tracesys_phase2:
 #if __SYSCALL_MASK == ~0
 	cmpq $__NR_syscall_max,%rax
 #else
-	andl $__SYSCALL_MASK,%eax
-	cmpl $__NR_syscall_max,%eax
+	.globl system_call_trace_compare
+	.globl system_call_trace_compare_end
+system_call_trace_compare:
+	cmpq $511,%rax			/* x32 syscalls start at 512 */
+	.byte P6_NOP4
+system_call_trace_compare_end:
 #endif
 	ja   int_ret_from_sys_call	/* RAX(%rsp) is already set */
 	movq %r10,%rcx	/* fixup for C */
@@ -593,6 +601,16 @@ int_restore_rest:
 	CFI_ENDPROC
 END(system_call)
 
+#if __SYSCALL_MASK != ~0
+	/* This replaces the usual comparisons if syscall.x32 is set */
+	.globl system_call_mask_compare
+	.globl system_call_mask_compare_end
+system_call_mask_compare:
+	andl $__SYSCALL_MASK,%eax
+	cmpl $__NR_syscall_max,%eax
+system_call_mask_compare_end:
+#endif
+
 	.macro FORK_LIKE func
 ENTRY(stub_\func)
 	CFI_STARTPROC
diff --git a/arch/x86/kernel/syscall_64.c b/arch/x86/kernel/syscall_64.c
index 4ac730b..7a6e66f 100644
--- a/arch/x86/kernel/syscall_64.c
+++ b/arch/x86/kernel/syscall_64.c
@@ -3,8 +3,14 @@
 #include <linux/linkage.h>
 #include <linux/sys.h>
 #include <linux/cache.h>
+#include <linux/moduleparam.h>
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX "syscall."
+#include <linux/bug.h>
+#include <linux/init.h>
 #include <asm/asm-offsets.h>
 #include <asm/syscall.h>
+#include <asm/alternative.h>
 
 #define __SYSCALL_COMMON(nr, sym, compat) __SYSCALL_64(nr, sym, compat)
 
@@ -30,3 +36,40 @@ asmlinkage const sys_call_ptr_t sys_call_table[__NR_syscall_max+1] = {
 	[0 ... __NR_syscall_max] = &sys_ni_syscall,
 #include <asm/syscalls_64.h>
 };
+
+#ifdef CONFIG_X86_X32_ABI
+
+/* Maybe enable x32 syscalls */
+
+bool x32_enabled = !IS_ENABLED(CONFIG_X86_X32_DISABLED);
+module_param_named(x32, x32_enabled, bool, 0444);
+
+extern char system_call_fast_compare_end[], system_call_fast_compare[],
+	system_call_trace_compare_end[], system_call_trace_compare[],
+	system_call_mask_compare_end[], system_call_mask_compare[];
+
+static int __init x32_enable(void)
+{
+	BUG_ON(system_call_fast_compare_end - system_call_fast_compare != 10);
+	BUG_ON(system_call_trace_compare_end - system_call_trace_compare != 10);
+	BUG_ON(system_call_mask_compare_end - system_call_mask_compare != 10);
+
+	if (x32_enabled) {
+		text_poke_early(system_call_fast_compare,
+				system_call_mask_compare, 10);
+		text_poke_early(system_call_trace_compare,
+				system_call_mask_compare, 10);
+#ifdef CONFIG_X86_X32_DISABLED
+		pr_info("Enabled x32 syscalls\n");
+#endif
+	}
+#ifndef CONFIG_X86_X32_DISABLED
+	else
+		pr_info("Disabled x32 syscalls\n");
+#endif
+
+	return 0;
+}
+late_initcall(x32_enable);
+
+#endif

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC][PATCH] x86: Make x32 syscall support conditional on a kernel parameter

[Geert Uytterhoeven]

On Thu, Nov 6, 2014 at 4:53 AM, Ben Hutchings <ben@decadent.org.uk> wrote:

> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2455,6 +2455,14 @@ config X86_X32
>           elf32_x86_64 support enabled to compile a kernel with this
>           option set.
>
> +config X86_X32_DISABLED
> +       bool "x32 ABI disabled by default"
> +       depends on X86_X32
> +       default n
> +       help
> +         Disable the x32 ABI unless explicitly enabled using the
> +         kernel paramter "syscall.x32=y".

parameter


> diff --git a/arch/x86/kernel/syscall_64.c b/arch/x86/kernel/syscall_64.c
> index 4ac730b..7a6e66f 100644
> --- a/arch/x86/kernel/syscall_64.c
> +++ b/arch/x86/kernel/syscall_64.c

> +static int __init x32_enable(void)
> +{
> +       BUG_ON(system_call_fast_compare_end - system_call_fast_compare != 10);
> +       BUG_ON(system_call_trace_compare_end - system_call_trace_compare != 10);
> +       BUG_ON(system_call_mask_compare_end - system_call_mask_compare != 10);

BUILD_BUG_ON

> +
> +       if (x32_enabled) {
> +               text_poke_early(system_call_fast_compare,
> +                               system_call_mask_compare, 10);
> +               text_poke_early(system_call_trace_compare,
> +                               system_call_mask_compare, 10);
> +#ifdef CONFIG_X86_X32_DISABLED
> +               pr_info("Enabled x32 syscalls\n");
> +#endif

I don't think it hurts to print this unconditionally.

> +       }
> +#ifndef CONFIG_X86_X32_DISABLED
> +       else
> +               pr_info("Disabled x32 syscalls\n");
> +#endif
> +
> +       return 0;
> +}
> +late_initcall(x32_enable);
> +
> +#endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC][PATCH] x86: Make x32 syscall support conditional on a kernel parameter

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

On Thu, 2014-11-06 at 09:02 +0100, Geert Uytterhoeven wrote:
> On Thu, Nov 6, 2014 at 4:53 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
[...]
> > diff --git a/arch/x86/kernel/syscall_64.c b/arch/x86/kernel/syscall_64.c
> > index 4ac730b..7a6e66f 100644
> > --- a/arch/x86/kernel/syscall_64.c
> > +++ b/arch/x86/kernel/syscall_64.c
> 
> > +static int __init x32_enable(void)
> > +{
> > +       BUG_ON(system_call_fast_compare_end - system_call_fast_compare != 10);
> > +       BUG_ON(system_call_trace_compare_end - system_call_trace_compare != 10);
> > +       BUG_ON(system_call_mask_compare_end - system_call_mask_compare != 10);
> 
> BUILD_BUG_ON

No, these can't be evaluated at compile time because the symbols are
external.

> > +
> > +       if (x32_enabled) {
> > +               text_poke_early(system_call_fast_compare,
> > +                               system_call_mask_compare, 10);
> > +               text_poke_early(system_call_trace_compare,
> > +                               system_call_mask_compare, 10);
> > +#ifdef CONFIG_X86_X32_DISABLED
> > +               pr_info("Enabled x32 syscalls\n");
> > +#endif
> 
> I don't think it hurts to print this unconditionally.
[...]

It seemed like it would be unnecessary noise in an x32-only system.

Ben.

-- 
Ben Hutchings
Beware of programmers who carry screwdrivers. - Leonard Brandwein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [RFC][PATCH] x86: Make x32 syscall support conditional on a kernel parameter

[Andy Lutomirski]

On 11/05/2014 07:53 PM, Ben Hutchings wrote:
> Enabling x32 in a distribution default kernel increases its attack
> surface while providing no benefit to the vast majority of its users.
> No-one seems interested in regularly checking for vulnerabilities
> specific to x32 (at least no-one with a white hat).
> 
> Still, requiring a separate or custom configuration just to turn on
> x32 seems wasteful.  And the only differences on syscall entry are
> two instructions (mask out the x32 flag and compare the syscall
> number).
> 
> So pad the standard comparison with a nop and add a kernel parameter
> "syscall.x32" which controls whether this is replaced with the x32
> version at boot time.  Add a Kconfig parameter to set the default.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> This is currently used in Debian, where x32 is an unofficial port that
> can be installed in parallel with amd64 (multiarch).  I acknowledge the
> patch is a bit hacky, e.g. it has several magic numbers.  I'm sending
> this to find out whether anything like this would be acceptable
> upstream.
> 
> Ben.
> 
>  Documentation/kernel-parameters.txt |  4 ++++
>  arch/x86/Kconfig                    |  8 +++++++
>  arch/x86/include/asm/elf.h          |  8 ++++++-
>  arch/x86/kernel/entry_64.S          | 26 ++++++++++++++++++----
>  arch/x86/kernel/syscall_64.c        | 43 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 84 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 4c81a86..1e161b0 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3397,6 +3397,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  	switches=	[HW,M68k]
>  
> +	syscall.x32=	[KNL,x86_64] Enable/disable use of x32 syscalls on
> +			an x86_64 kernel where CONFIG_X86_X32 is enabled.
> +			Default depends on CONFIG_X86_X32_DISABLED.
> +
>  	sysfs.deprecated=0|1 [KNL]
>  			Enable/disable old style sysfs layout for old udev
>  			on older distributions. When this option is enabled
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ded8a67..9627922 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2455,6 +2455,14 @@ config X86_X32
>  	  elf32_x86_64 support enabled to compile a kernel with this
>  	  option set.
>  
> +config X86_X32_DISABLED
> +	bool "x32 ABI disabled by default"
> +	depends on X86_X32
> +	default n
> +	help
> +	  Disable the x32 ABI unless explicitly enabled using the
> +	  kernel paramter "syscall.x32=y".
> +
>  config COMPAT
>  	def_bool y
>  	depends on IA32_EMULATION || X86_X32
> diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> index ca3347a..2a8b89a 100644
> --- a/arch/x86/include/asm/elf.h
> +++ b/arch/x86/include/asm/elf.h
> @@ -154,6 +154,12 @@ do {						\
>  
>  #else /* CONFIG_X86_32 */
>  
> +#ifdef CONFIG_X86_X32_ABI
> +extern bool x32_enabled;
> +#else
> +#define x32_enabled 0
> +#endif
> +
>  /*
>   * This is used to ensure we don't load something for the wrong architecture.
>   */
> @@ -162,7 +168,7 @@ do {						\
>  
>  #define compat_elf_check_arch(x)					\
>  	(elf_check_arch_ia32(x) ||					\
> -	 (IS_ENABLED(CONFIG_X86_X32_ABI) && (x)->e_machine == EM_X86_64))
> +	 (x32_enabled && (x)->e_machine == EM_X86_64))
>  
>  #if __USER32_DS != __USER_DS
>  # error "The following code assumes __USER32_DS == __USER_DS"
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index df088bb..50b265a 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -414,8 +414,12 @@ system_call_fastpath:
>  #if __SYSCALL_MASK == ~0
>  	cmpq $__NR_syscall_max,%rax
>  #else
> -	andl $__SYSCALL_MASK,%eax
> -	cmpl $__NR_syscall_max,%eax
> +	.globl system_call_fast_compare
> +	.globl system_call_fast_compare_end
> +system_call_fast_compare:
> +	cmpq $511,%rax			/* x32 syscalls start at 512 */
> +	.byte P6_NOP4
> +system_call_fast_compare_end:
>  #endif
>  	ja ret_from_sys_call  /* and return regs->ax */
>  	movq %r10,%rcx
> @@ -520,8 +524,12 @@ tracesys_phase2:
>  #if __SYSCALL_MASK == ~0
>  	cmpq $__NR_syscall_max,%rax
>  #else
> -	andl $__SYSCALL_MASK,%eax
> -	cmpl $__NR_syscall_max,%eax
> +	.globl system_call_trace_compare
> +	.globl system_call_trace_compare_end
> +system_call_trace_compare:
> +	cmpq $511,%rax			/* x32 syscalls start at 512 */
> +	.byte P6_NOP4
> +system_call_trace_compare_end:

I think that, if x32 is disabled, that andl $__SYSCALL_MASK,%rax should
be nopped out.  Can this, and the comparison, use the standard
alternatives mechanism?

And why are there extra syscall numbers in x32 land?  There's an x32 bit
*and* extra numbers.  I'm confused.

That magic number is bad.  I would make it a real #define and put an
appropriate assertion into whatever generates syscalls_64.h.

Also, if you do this, can you change all four instances of %eax in there
to %rax?  %eax is just wrong AFAICT.

Also, as a heads up to anyone who actually uses this: I was confused
about the audit situation on x32-enabled kernels.  I thought that this
patch had been applied:

https://www.redhat.com/archives/linux-audit/2014-May/msg00099.html

but it wasn't.  As a result, the commit message for this fix is wrong:

81f49a8fd708 x86, x32, audit: Fix x32's AUDIT_ARCH wrt audit

It *is* a user-visible fix and should possibly go to -stable, especially
if anyone actually uses this *!$@ thing.  Of course, audit is royally
screwed up on x32-enabled kernels anyway.  I admit that I find myself
barely caring.

--Andy

Re: [RFC][PATCH] x86: Make x32 syscall support conditional on a kernel parameter

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]

On Thu, 2014-11-06 at 14:46 -0800, Andy Lutomirski wrote:
> On 11/05/2014 07:53 PM, Ben Hutchings wrote:
> > Enabling x32 in a distribution default kernel increases its attack
> > surface while providing no benefit to the vast majority of its users.
> > No-one seems interested in regularly checking for vulnerabilities
> > specific to x32 (at least no-one with a white hat).
> > 
> > Still, requiring a separate or custom configuration just to turn on
> > x32 seems wasteful.  And the only differences on syscall entry are
> > two instructions (mask out the x32 flag and compare the syscall
> > number).
> > 
> > So pad the standard comparison with a nop and add a kernel parameter
> > "syscall.x32" which controls whether this is replaced with the x32
> > version at boot time.  Add a Kconfig parameter to set the default.
[...]
> > --- a/arch/x86/kernel/entry_64.S
> > +++ b/arch/x86/kernel/entry_64.S
> > @@ -414,8 +414,12 @@ system_call_fastpath:
> >  #if __SYSCALL_MASK == ~0
> >  	cmpq $__NR_syscall_max,%rax
> >  #else
> > -	andl $__SYSCALL_MASK,%eax
> > -	cmpl $__NR_syscall_max,%eax
> > +	.globl system_call_fast_compare
> > +	.globl system_call_fast_compare_end
> > +system_call_fast_compare:
> > +	cmpq $511,%rax			/* x32 syscalls start at 512 */
> > +	.byte P6_NOP4
> > +system_call_fast_compare_end:
> >  #endif
> >  	ja ret_from_sys_call  /* and return regs->ax */
> >  	movq %r10,%rcx
> > @@ -520,8 +524,12 @@ tracesys_phase2:
> >  #if __SYSCALL_MASK == ~0
> >  	cmpq $__NR_syscall_max,%rax
> >  #else
> > -	andl $__SYSCALL_MASK,%eax
> > -	cmpl $__NR_syscall_max,%eax
> > +	.globl system_call_trace_compare
> > +	.globl system_call_trace_compare_end
> > +system_call_trace_compare:
> > +	cmpq $511,%rax			/* x32 syscalls start at 512 */
> > +	.byte P6_NOP4
> > +system_call_trace_compare_end:
> 
> I think that, if x32 is disabled, that andl $__SYSCALL_MASK,%rax should
> be nopped out.

Yes, that's what happens.

> Can this, and the comparison, use the standard alternatives mechanism?

There isn't really a standard alternatives mechanism!  There is a CPU-
features alternatives mechanism, an SMP alternatives mechanism and a PV
alternatives mechanism, but they don't share anything above the
text_poke*() functions.

> And why are there extra syscall numbers in x32 land?  There's an x32 bit
> *and* extra numbers.  I'm confused.

I think the extra numbers are assigned for system calls where adjustment
is always needed on entry and exit, while the x32 bit is used to trigger
compat handling if it's needed at a deeper level.  is_compat_task()
tests the latter.

> That magic number is bad.  I would make it a real #define and put an
> appropriate assertion into whatever generates syscalls_64.h.
> 
> Also, if you do this, can you change all four instances of %eax in there
> to %rax?  %eax is just wrong AFAICT.

I can only guess that %eax is currently used because it makes the code
slightly smaller (no REX prefix needed).

> Also, as a heads up to anyone who actually uses this: I was confused
> about the audit situation on x32-enabled kernels.  I thought that this
> patch had been applied:
> 
> https://www.redhat.com/archives/linux-audit/2014-May/msg00099.html
> 
> but it wasn't.  As a result, the commit message for this fix is wrong:
> 
> 81f49a8fd708 x86, x32, audit: Fix x32's AUDIT_ARCH wrt audit
> 
> It *is* a user-visible fix and should possibly go to -stable, especially
> if anyone actually uses this *!$@ thing.  Of course, audit is royally
> screwed up on x32-enabled kernels anyway.  I admit that I find myself
> barely caring.

Thanks; this went into 3.16.7-ckt2 and should go into a Debian kernel
update soon.

Ben.

-- 
Ben Hutchings
Experience is directly proportional to the value of equipment destroyed.
                                                         - Carolyn Scheppner

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]