diff mbox series

[v5,08/23] arm64: compat: Add KUSER_HELPERS config option

Message ID 20190222122430.21180-9-vincenzo.frascino@arm.com (mailing list archive)
State New, archived
Headers show
Series Unify vDSOs across more architectures | expand

Commit Message

Vincenzo Frascino Feb. 22, 2019, 12:24 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>
---
 arch/arm64/Kconfig          | 21 +++++++++++++++++++++
 arch/arm64/kernel/Makefile  |  3 ++-
 arch/arm64/kernel/kuser32.S |  7 +++----
 arch/arm64/kernel/vdso.c    | 15 +++++++++++++++
 4 files changed, 41 insertions(+), 5 deletions(-)

Comments

Mark Rutland Feb. 22, 2019, 2:04 p.m. UTC | #1
On Fri, Feb 22, 2019 at 12:24:15PM +0000, Vincenzo Frascino wrote:
> 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>

IIUC, the KUSER bits aren't striclty dependent on the rest of the vDSO
changes.

I think the KUSER bits should be split into a preparatory series, so that they
can be merged soon, rather than tying them artificially to the rest of the vDSO
bits (which I expect will require significantly more review).

> ---
>  arch/arm64/Kconfig          | 21 +++++++++++++++++++++
>  arch/arm64/kernel/Makefile  |  3 ++-
>  arch/arm64/kernel/kuser32.S |  7 +++----
>  arch/arm64/kernel/vdso.c    | 15 +++++++++++++++
>  4 files changed, 41 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index d898da2e20f5..ed3290494f1c 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1465,6 +1465,27 @@ config COMPAT
>  
>  	  If you want to execute 32-bit userspace applications, say Y.
>  
> +config KUSER_HELPERS
> +	bool "Enable kuser helpers page for compatibility with 32 bit applications."
> +	depends on COMPAT
> +	default y
> +	help
> +	  Enables kuser helpers to be mapped in a special purpose page at a fixed
> +	  address to maintain independence from the type of CPU present in the SoC.
> +	  This feature is provided for compatibility reasons in fact allows 32 bit
> +	  applications compliant with ARMv4 up to ARMv8 to run without any
> +	  modification.
> +
> +	  Warning: Being always mapped at a fixed address makes it easier to create
> +	  exploits based on ROP type of attacks.
> +
> +	  As a consequence of this, this feature is made configurable but be aware that
> +	  it can be turned off if and only if the binaries and the libraries running on
> +	  a specific platform are designed to do not make use of these helpers, otherwise
> +	  should be left on.
> +
> +	  See Documentation/arm/kernel_user_helpers.txt for details.

This is quite divergent from the text for the 32-bit arm KUSER_HELPERS.

Could we please reuse that existing text as far as possible, with minimal
changes?

e.g.

config KUSER_HELPERS
	bool "Enable kuser helpers for compat tasks"
	depends on COMPAT
	default y
	help
	  Warning: disabling this option may break 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.

I believe that Will wanted to default the kuser helpers off, as applications
using them need the instruction emulation which is also disabled by default.

I'm also not sure which binaries we expect to work on arm64, so I'm not sure if
the ARMv4 reference is correct.

Thanks,
Mark.
Russell King (Oracle) Feb. 22, 2019, 2:09 p.m. UTC | #2
On Fri, Feb 22, 2019 at 02:04:27PM +0000, Mark Rutland wrote:
> I believe that Will wanted to default the kuser helpers off, as applications
> using them need the instruction emulation which is also disabled by default.
> 
> I'm also not sure which binaries we expect to work on arm64, so I'm not sure if
> the ARMv4 reference is correct.

Do you expect ARMv4 - even ARMv4 OABI binaries to fail on ARM64?

(I've run my OABI ARMv4 distro under KVM on Macchiatobin when I've
needed to rebuild a RPM without problems - using its own gcc.  No
observable issues.)
Vincenzo Frascino Feb. 26, 2019, 12:10 p.m. UTC | #3
Hi Mark,

thank you for your review.

On 22/02/2019 14:04, Mark Rutland wrote:
> On Fri, Feb 22, 2019 at 12:24:15PM +0000, Vincenzo Frascino wrote:
>> 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>
> 
> IIUC, the KUSER bits aren't striclty dependent on the rest of the vDSO
> changes.
> 
> I think the KUSER bits should be split into a preparatory series, so that they
> can be merged soon, rather than tying them artificially to the rest of the vDSO
> bits (which I expect will require significantly more review).
> 

This is a good point. I will extract them and post as a separate patch set.
In doing so I will update the config description below.
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index d898da2e20f5..ed3290494f1c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1465,6 +1465,27 @@  config COMPAT
 
 	  If you want to execute 32-bit userspace applications, say Y.
 
+config KUSER_HELPERS
+	bool "Enable kuser helpers page for compatibility with 32 bit applications."
+	depends on COMPAT
+	default y
+	help
+	  Enables kuser helpers to be mapped in a special purpose page at a fixed
+	  address to maintain independence from the type of CPU present in the SoC.
+	  This feature is provided for compatibility reasons in fact allows 32 bit
+	  applications compliant with ARMv4 up to ARMv8 to run without any
+	  modification.
+
+	  Warning: Being always mapped at a fixed address makes it easier to create
+	  exploits based on ROP type of attacks.
+
+	  As a consequence of this, this feature is made configurable but be aware that
+	  it can be turned off if and only if the binaries and the libraries running on
+	  a specific platform are designed to do not make use of these helpers, otherwise
+	  should be left on.
+
+	  See Documentation/arm/kernel_user_helpers.txt for details.
+
 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 f19e2b015097..7d38633bf33f 100644
--- a/arch/arm64/kernel/kuser32.S
+++ b/arch/arm64/kernel/kuser32.S
@@ -5,10 +5,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() ad 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 4c9476516e2f..523e56658b84 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -75,6 +75,7 @@  static const struct vm_special_mapping aarch32_vdso_spec[2] = {
 	},
 };
 
+#ifdef CONFIG_KUSER_HELPERS
 static int aarch32_alloc_kuser_vdso_page(void)
 {
 	extern char __kuser_helper_start[], __kuser_helper_end[];
@@ -96,6 +97,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)
 {
@@ -127,6 +134,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;
@@ -139,6 +147,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)
 {