diff mbox series

[v3,4/4] arm64: compat: Add KUSER_HELPERS config option

Message ID 20190402162757.13491-5-vincenzo.frascino@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: compat: Add kuser helpers config option | expand

Commit Message

Vincenzo Frascino April 2, 2019, 4:27 p.m. UTC 
When kuser helpers are enabled the kernel maps the relative code at
a fixed address (0xffff0000). Making configurable the option to disable
them means that the kernel can remove this mapping and any access to
this memory area results in a sigfault.

Add a KUSER_HELPERS config option that can be used to disable the
mapping when it is turned off.

This option can be turned off if and only if the applications are
designed specifically for the platform and they do not make use of the
kuser helpers code.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/Kconfig          | 28 ++++++++++++++++++++++++++++
 arch/arm64/kernel/Makefile  |  3 ++-
 arch/arm64/kernel/kuser32.S |  7 +++----
 arch/arm64/kernel/vdso.c    | 15 +++++++++++++++
 4 files changed, 48 insertions(+), 5 deletions(-)

Comments

Catalin Marinas April 4, 2019, 10:08 a.m. UTC | #1 
On Tue, Apr 02, 2019 at 05:27:57PM +0100, Vincenzo Frascino wrote:
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7e34b9eba5de..aa28884a2376 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1494,6 +1494,34 @@ config COMPAT
>  
>  	  If you want to execute 32-bit userspace applications, say Y.
>  
> +config KUSER_HELPERS
> +	bool "Enable kuser helpers page for 32 bit applications."
> +	depends on COMPAT
> +	default y
> +	help
> +	  Warning: disabling this option may break 32-bit user programs.
> +
> +	  Provide kuser helpers to compat tasks. The kernel provides
> +	  helper code to userspace in read only form at a fixed location
> +	  to allow userspace to be independent of the CPU type fitted to
> +	  the system. This permits binaries to be run on ARMv4 through
> +	  to ARMv8 without modification.
> +
> +	  See Documentation/arm/kernel_user_helpers.txt for details.
> +
> +	  However, the fixed address nature of these helpers can be used
> +	  by ROP (return orientated programming) authors when creating
> +	  exploits.
> +
> +	  If all of the binaries and libraries which run on your platform
> +	  are built specifically for your platform, and make no use of
> +	  these helpers, then you can turn this option off to hinder
> +	  such exploits. However, in that case, if a binary or library
> +	  relying on those helpers is run, it will not function correctly.
> +
> +	  Say N here only if you are absolutely certain that you do not
> +	  need these helpers; otherwise, the safe option is to say Y.

In order to close the potential issue with the user unmapping the
vectors page in the 64K configuration, we'd need the change below as
well (on top of the TASK_SIZE_32 patch I queued for -rc4).

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 228af56ece48..fcd0e691b1ea 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -57,7 +57,7 @@
 #define TASK_SIZE_64		(UL(1) << vabits_user)
 
 #ifdef CONFIG_COMPAT
-#ifdef CONFIG_ARM64_64K_PAGES
+#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
 /*
  * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied
  * by the compat vectors page.
Vincenzo Frascino April 4, 2019, 2:07 p.m. UTC | #2 
On 04/04/2019 11:08, Catalin Marinas wrote:
> On Tue, Apr 02, 2019 at 05:27:57PM +0100, Vincenzo Frascino wrote:
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 7e34b9eba5de..aa28884a2376 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -1494,6 +1494,34 @@ config COMPAT
>>  
>>  	  If you want to execute 32-bit userspace applications, say Y.
>>  
>> +config KUSER_HELPERS
>> +	bool "Enable kuser helpers page for 32 bit applications."
>> +	depends on COMPAT
>> +	default y
>> +	help
>> +	  Warning: disabling this option may break 32-bit user programs.
>> +
>> +	  Provide kuser helpers to compat tasks. The kernel provides
>> +	  helper code to userspace in read only form at a fixed location
>> +	  to allow userspace to be independent of the CPU type fitted to
>> +	  the system. This permits binaries to be run on ARMv4 through
>> +	  to ARMv8 without modification.
>> +
>> +	  See Documentation/arm/kernel_user_helpers.txt for details.
>> +
>> +	  However, the fixed address nature of these helpers can be used
>> +	  by ROP (return orientated programming) authors when creating
>> +	  exploits.
>> +
>> +	  If all of the binaries and libraries which run on your platform
>> +	  are built specifically for your platform, and make no use of
>> +	  these helpers, then you can turn this option off to hinder
>> +	  such exploits. However, in that case, if a binary or library
>> +	  relying on those helpers is run, it will not function correctly.
>> +
>> +	  Say N here only if you are absolutely certain that you do not
>> +	  need these helpers; otherwise, the safe option is to say Y.
> 
> In order to close the potential issue with the user unmapping the
> vectors page in the 64K configuration, we'd need the change below as
> well (on top of the TASK_SIZE_32 patch I queued for -rc4).
> 
> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> index 228af56ece48..fcd0e691b1ea 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -57,7 +57,7 @@
>  #define TASK_SIZE_64		(UL(1) << vabits_user)
>  
>  #ifdef CONFIG_COMPAT
> -#ifdef CONFIG_ARM64_64K_PAGES
> +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
>  /*
>   * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied
>   * by the compat vectors page.
> 

I agree, this change is required when the new config option is added. Do you
want me to send an additional patch or will this be folded with the existing series?
Catalin Marinas April 4, 2019, 2:51 p.m. UTC | #3 
On Thu, Apr 04, 2019 at 03:07:43PM +0100, Vincenzo Frascino wrote:
> On 04/04/2019 11:08, Catalin Marinas wrote:
> > On Tue, Apr 02, 2019 at 05:27:57PM +0100, Vincenzo Frascino wrote:
> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> >> index 7e34b9eba5de..aa28884a2376 100644
> >> --- a/arch/arm64/Kconfig
> >> +++ b/arch/arm64/Kconfig
> >> @@ -1494,6 +1494,34 @@ config COMPAT
> >>  
> >>  	  If you want to execute 32-bit userspace applications, say Y.
> >>  
> >> +config KUSER_HELPERS
> >> +	bool "Enable kuser helpers page for 32 bit applications."
> >> +	depends on COMPAT
> >> +	default y
> >> +	help
> >> +	  Warning: disabling this option may break 32-bit user programs.
> >> +
> >> +	  Provide kuser helpers to compat tasks. The kernel provides
> >> +	  helper code to userspace in read only form at a fixed location
> >> +	  to allow userspace to be independent of the CPU type fitted to
> >> +	  the system. This permits binaries to be run on ARMv4 through
> >> +	  to ARMv8 without modification.
> >> +
> >> +	  See Documentation/arm/kernel_user_helpers.txt for details.
> >> +
> >> +	  However, the fixed address nature of these helpers can be used
> >> +	  by ROP (return orientated programming) authors when creating
> >> +	  exploits.
> >> +
> >> +	  If all of the binaries and libraries which run on your platform
> >> +	  are built specifically for your platform, and make no use of
> >> +	  these helpers, then you can turn this option off to hinder
> >> +	  such exploits. However, in that case, if a binary or library
> >> +	  relying on those helpers is run, it will not function correctly.
> >> +
> >> +	  Say N here only if you are absolutely certain that you do not
> >> +	  need these helpers; otherwise, the safe option is to say Y.
> > 
> > In order to close the potential issue with the user unmapping the
> > vectors page in the 64K configuration, we'd need the change below as
> > well (on top of the TASK_SIZE_32 patch I queued for -rc4).
> > 
> > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> > index 228af56ece48..fcd0e691b1ea 100644
> > --- a/arch/arm64/include/asm/processor.h
> > +++ b/arch/arm64/include/asm/processor.h
> > @@ -57,7 +57,7 @@
> >  #define TASK_SIZE_64		(UL(1) << vabits_user)
> >  
> >  #ifdef CONFIG_COMPAT
> > -#ifdef CONFIG_ARM64_64K_PAGES
> > +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
> >  /*
> >   * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied
> >   * by the compat vectors page.
> > 
> 
> I agree, this change is required when the new config option is added. Do you
> want me to send an additional patch or will this be folded with the existing series?

Folding it into patch 4/4 would be best but our arm64 for-next/core is
based on -rc3 and the TASK_SIZE_32 reduction didn't go in yet:

https://lore.kernel.org/linux-arm-kernel/20190401113014.20866-1-vincenzo.frascino@arm.com/

I can drop the TASK_SIZE_32 patch from for-next/fixes (arguably, it's
not a regression and we cc stable anyway) and Will can merge it before
this series. Otherwise, the fixup above can go in at 5.2-rc1.

Will, any preference?
Will Deacon April 4, 2019, 3:01 p.m. UTC | #4 
On Thu, Apr 04, 2019 at 03:51:23PM +0100, Catalin Marinas wrote:
> On Thu, Apr 04, 2019 at 03:07:43PM +0100, Vincenzo Frascino wrote:
> > On 04/04/2019 11:08, Catalin Marinas wrote:
> > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> > > index 228af56ece48..fcd0e691b1ea 100644
> > > --- a/arch/arm64/include/asm/processor.h
> > > +++ b/arch/arm64/include/asm/processor.h
> > > @@ -57,7 +57,7 @@
> > >  #define TASK_SIZE_64		(UL(1) << vabits_user)
> > >  
> > >  #ifdef CONFIG_COMPAT
> > > -#ifdef CONFIG_ARM64_64K_PAGES
> > > +#if defined(CONFIG_ARM64_64K_PAGES) && defined(CONFIG_KUSER_HELPERS)
> > >  /*
> > >   * With CONFIG_ARM64_64K_PAGES enabled, the last page is occupied
> > >   * by the compat vectors page.
> > > 
> > 
> > I agree, this change is required when the new config option is added. Do you
> > want me to send an additional patch or will this be folded with the existing series?
> 
> Folding it into patch 4/4 would be best but our arm64 for-next/core is
> based on -rc3 and the TASK_SIZE_32 reduction didn't go in yet:
> 
> https://lore.kernel.org/linux-arm-kernel/20190401113014.20866-1-vincenzo.frascino@arm.com/
> 
> I can drop the TASK_SIZE_32 patch from for-next/fixes (arguably, it's
> not a regression and we cc stable anyway) and Will can merge it before
> this series. Otherwise, the fixup above can go in at 5.2-rc1.
> 
> Will, any preference?

Agreed on taking this via for-next/core instead of fixes.

Will
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7e34b9eba5de..aa28884a2376 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1494,6 +1494,34 @@  config COMPAT
 
 	  If you want to execute 32-bit userspace applications, say Y.
 
+config KUSER_HELPERS
+	bool "Enable kuser helpers page for 32 bit applications."
+	depends on COMPAT
+	default y
+	help
+	  Warning: disabling this option may break 32-bit user programs.
+
+	  Provide kuser helpers to compat tasks. The kernel provides
+	  helper code to userspace in read only form at a fixed location
+	  to allow userspace to be independent of the CPU type fitted to
+	  the system. This permits binaries to be run on ARMv4 through
+	  to ARMv8 without modification.
+
+	  See Documentation/arm/kernel_user_helpers.txt for details.
+
+	  However, the fixed address nature of these helpers can be used
+	  by ROP (return orientated programming) authors when creating
+	  exploits.
+
+	  If all of the binaries and libraries which run on your platform
+	  are built specifically for your platform, and make no use of
+	  these helpers, then you can turn this option off to hinder
+	  such exploits. However, in that case, if a binary or library
+	  relying on those helpers is run, it will not function correctly.
+
+	  Say N here only if you are absolutely certain that you do not
+	  need these helpers; otherwise, the safe option is to say Y.
+
 config SYSVIPC_COMPAT
 	def_bool y
 	depends on COMPAT && SYSVIPC
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 50f76b88a967..c7bd0794855a 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -27,8 +27,9 @@  OBJCOPYFLAGS := --prefix-symbols=__efistub_
 $(obj)/%.stub.o: $(obj)/%.o FORCE
 	$(call if_changed,objcopy)
 
-obj-$(CONFIG_COMPAT)			+= sys32.o kuser32.o signal32.o 	\
+obj-$(CONFIG_COMPAT)			+= sys32.o signal32.o			\
 					   sigreturn32.o sys_compat.o
+obj-$(CONFIG_KUSER_HELPERS)		+= kuser32.o
 obj-$(CONFIG_FUNCTION_TRACER)		+= ftrace.o entry-ftrace.o
 obj-$(CONFIG_MODULES)			+= module.o
 obj-$(CONFIG_ARM64_MODULE_PLTS)		+= module-plts.o
diff --git a/arch/arm64/kernel/kuser32.S b/arch/arm64/kernel/kuser32.S
index c5f2bbafd723..49825e9e421e 100644
--- a/arch/arm64/kernel/kuser32.S
+++ b/arch/arm64/kernel/kuser32.S
@@ -6,10 +6,9 @@ 
  * Copyright (C) 2005-2011 Nicolas Pitre <nico@fluxnic.net>
  * Copyright (C) 2012-2018 ARM Ltd.
  *
- * Each segment is 32-byte aligned and will be moved to the top of the high
- * vector page.  New segments (if ever needed) must be added in front of
- * existing ones.  This mechanism should be used only for things that are
- * really small and justified, and not be abused freely.
+ * The kuser helpers below are mapped at a fixed address by
+ * aarch32_setup_additional_pages() and are provided for compatibility
+ * reasons with 32 bit (aarch32) applications that need them.
  *
  * See Documentation/arm/kernel_user_helpers.txt for formal definitions.
  */
diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index 7ee676c345ed..f012f90000bd 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -77,6 +77,7 @@  static const struct vm_special_mapping aarch32_vdso_spec[C_PAGES] = {
 	},
 };
 
+#ifdef CONFIG_KUSER_HELPERS
 static int aarch32_alloc_kuser_vdso_page(void)
 {
 	extern char __kuser_helper_start[], __kuser_helper_end[];
@@ -98,6 +99,12 @@  static int aarch32_alloc_kuser_vdso_page(void)
 
 	return 0;
 }
+#else
+static int aarch32_alloc_kuser_vdso_page(void)
+{
+	return 0;
+}
+#endif /* CONFIG_KUSER_HELPER */
 
 static int aarch32_alloc_sigreturn_vdso_page(void)
 {
@@ -137,6 +144,7 @@  static int __init aarch32_alloc_vdso_pages(void)
 }
 arch_initcall(aarch32_alloc_vdso_pages);
 
+#ifdef CONFIG_KUSER_HELPERS
 static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 {
 	void *ret;
@@ -149,6 +157,13 @@  static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
 
 	return PTR_ERR_OR_ZERO(ret);
 }
+#else
+static int aarch32_kuser_helpers_setup(struct mm_struct *mm)
+{
+	/* kuser helpers not enabled */
+	return 0;
+}
+#endif /* CONFIG_KUSER_HELPERS */
 
 static int aarch32_sigreturn_setup(struct mm_struct *mm)
 {