| Message ID | 20170804183151.78804-1-salyzyn@android.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 04, 2017 at 11:31:40AM -0700, Mark Salyzyn wrote: > From: Kevin Brodsky <kevin.brodsky@arm.com> > > Make it possible to disable the kuser helpers by adding a KUSER_HELPERS > config option (enabled by default). When disabled, all kuser > helpers-related code is removed from the kernel and no mapping is done > at the fixed high address (0xffff0000); any attempt to use a kuser > helper from a 32-bit process will result in a segfault. > > Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com> > Signed-off-by: Mark Salyzyn <salyzyn@android.com> > --- > arch/arm64/Kconfig | 29 ++++++++++++++++++ > arch/arm64/kernel/Makefile | 3 +- > arch/arm64/kernel/kuser32.S | 48 ++--------------------------- > arch/arm64/kernel/sigreturn32.S | 67 +++++++++++++++++++++++++++++++++++++++++ > arch/arm64/kernel/vdso.c | 6 ++++ > 5 files changed, 107 insertions(+), 46 deletions(-) > create mode 100644 arch/arm64/kernel/sigreturn32.S > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index dfd908630631..902ad4b5b3d3 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1100,6 +1100,35 @@ config COMPAT > > If you want to execute 32-bit userspace applications, say Y. > > +config KUSER_HELPERS > + bool "Enable the kuser helpers page in 32-bit processes" > + depends on COMPAT > + default y > + help > + Warning: disabling this option may break 32-bit applications. > + > + Provide kuser helpers in a special purpose fixed-address page. 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 32-bit binaries to be run on ARMv4 through > + to ARMv8 without modification. That's a bit over-reaching, I think. You might also need e.g. swp emulation enabled to run ARMv4 binaries. > + > + 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 32-bit binaries and libraries that 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 receive a SIGSEGV signal, which will terminate the > + program. Typically, binaries compiled for ARMv7 or later do not use > + the kuser helpers. > + > + 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 think we should default this to 'N' for arm64. > + > config SYSVIPC_COMPAT > def_bool y > depends on COMPAT && SYSVIPC > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile > index f2b4e816b6de..314b9abda0a0 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) > > -arm64-obj-$(CONFIG_COMPAT) += sys32.o kuser32.o signal32.o \ > +arm64-obj-$(CONFIG_COMPAT) += sys32.o sigreturn32.o signal32.o \ > sys_compat.o entry32.o > +arm64-obj-$(CONFIG_KUSER_HELPERS) += kuser32.o > arm64-obj-$(CONFIG_FUNCTION_TRACER) += ftrace.o entry-ftrace.o > arm64-obj-$(CONFIG_MODULES) += arm64ksyms.o module.o > arm64-obj-$(CONFIG_ARM64_MODULE_PLTS) += module-plts.o > diff --git a/arch/arm64/kernel/kuser32.S b/arch/arm64/kernel/kuser32.S > index 997e6b27ff6a..d15b5c2935b3 100644 > --- a/arch/arm64/kernel/kuser32.S > +++ b/arch/arm64/kernel/kuser32.S > @@ -20,16 +20,13 @@ > * > * AArch32 user helpers. > * > - * 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. > + * These helpers are provided for compatibility with AArch32 binaries that > + * still need them. They are installed at a fixed address by > + * aarch32_setup_additional_pages(). > * > * See Documentation/arm/kernel_user_helpers.txt for formal definitions. > */ > > -#include <asm/unistd.h> > - > .align 5 > .globl __kuser_helper_start > __kuser_helper_start: > @@ -77,42 +74,3 @@ __kuser_helper_version: // 0xffff0ffc > .word ((__kuser_helper_end - __kuser_helper_start) >> 5) > .globl __kuser_helper_end > __kuser_helper_end: > - > -/* > - * AArch32 sigreturn code > - * > - * For ARM syscalls, the syscall number has to be loaded into r7. > - * We do not support an OABI userspace. > - * > - * For Thumb syscalls, we also pass the syscall number via r7. We therefore > - * need two 16-bit instructions. > - */ > - .globl __aarch32_sigret_code_start > -__aarch32_sigret_code_start: > - > - /* > - * ARM Code > - */ > - .byte __NR_compat_sigreturn, 0x70, 0xa0, 0xe3 // mov r7, #__NR_compat_sigreturn > - .byte __NR_compat_sigreturn, 0x00, 0x00, 0xef // svc #__NR_compat_sigreturn > - > - /* > - * Thumb code > - */ > - .byte __NR_compat_sigreturn, 0x27 // svc #__NR_compat_sigreturn > - .byte __NR_compat_sigreturn, 0xdf // mov r7, #__NR_compat_sigreturn > - > - /* > - * ARM code > - */ > - .byte __NR_compat_rt_sigreturn, 0x70, 0xa0, 0xe3 // mov r7, #__NR_compat_rt_sigreturn > - .byte __NR_compat_rt_sigreturn, 0x00, 0x00, 0xef // svc #__NR_compat_rt_sigreturn > - > - /* > - * Thumb code > - */ > - .byte __NR_compat_rt_sigreturn, 0x27 // svc #__NR_compat_rt_sigreturn > - .byte __NR_compat_rt_sigreturn, 0xdf // mov r7, #__NR_compat_rt_sigreturn > - > - .globl __aarch32_sigret_code_end > -__aarch32_sigret_code_end: > diff --git a/arch/arm64/kernel/sigreturn32.S b/arch/arm64/kernel/sigreturn32.S > new file mode 100644 > index 000000000000..6ecda4d84cd5 > --- /dev/null > +++ b/arch/arm64/kernel/sigreturn32.S Why do we need a new file for these? > @@ -0,0 +1,67 @@ > +/* > + * sigreturn trampolines for AArch32. > + * > + * Copyright (C) 2005-2011 Nicolas Pitre <nico@fluxnic.net> > + * Copyright (C) 2012 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + * > + * > + * AArch32 sigreturn code > + * > + * For ARM syscalls, the syscall number has to be loaded into r7. > + * We do not support an OABI userspace. > + * > + * For Thumb syscalls, we also pass the syscall number via r7. We therefore > + * need two 16-bit instructions. > + */ > + > +#include <asm/unistd.h> > + > + .globl __aarch32_sigret_code_start > +__aarch32_sigret_code_start: > + > + /* > + * ARM Code > + */ > + // mov r7, #__NR_compat_sigreturn > + .byte __NR_compat_sigreturn, 0x70, 0xa0, 0xe3 > + // svc #__NR_compat_sigreturn > + .byte __NR_compat_sigreturn, 0x00, 0x00, 0xef If there is a good reason to have this in a new file, why reformat the stuff at the same time? This looks like more churn. I have a feeling that there's a nice concise patch set in this lot that's trying to get out ;) Will
Started working on rebasing the entire set based on your comments, and to retest with a larger set of changes I have in my private branch. I had four queries regarding the requested changes that may affect the outcome; all in this patch. On 08/15/2017 06:38 AM, Will Deacon wrote: >> + >> + 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 think we should default this to 'N' for arm64. This would introduce engineering churn on defconfig updates to the subset of the 8000 Android devices that are arm64 based. We have yet to find a means to actually turn off the KUSER helpers yet and abandon the large number of armv7 and earlier applications. For non Android use, I agree whole heartedly, but can not bring myself to do so. It is there by default, and we are offering a means to turn it off for those that want the security. So I am pushing back ... > >> diff --git a/arch/arm64/kernel/sigreturn32.S b/arch/arm64/kernel/sigreturn32.S >> new file mode 100644 >> index 000000000000..6ecda4d84cd5 >> --- /dev/null >> +++ b/arch/arm64/kernel/sigreturn32.S > Why do we need a new file for these? Less ifdefs, more arm64-obj-$(CONFIG_KUSER_HELPERS) and (for a future patch) arm64-obj-$(CONFIG_VDSO32-y). It is also confusing that sigreturn32 and kuser32 could be switched off separately, and in this specific case, using an ifdef, having all kuser32 stuff wiped from the file and yet it remains to hold _unrelated_ sigreturn32 content. >> + >> + /* >> + * ARM Code >> + */ >> + // mov r7, #__NR_compat_sigreturn >> + .byte __NR_compat_sigreturn, 0x70, 0xa0, 0xe3 >> + // svc #__NR_compat_sigreturn >> + .byte __NR_compat_sigreturn, 0x00, 0x00, 0xef > If there is a good reason to have this in a new file, why reformat the stuff > at the same time? This looks like more churn. checkpatch.pl did not like the new file, so I wrapped the comments to proceed the .byte's to suppress a large amount of 80-character limit warnings. No good deed goes unpunished. > I have a feeling that there's a nice concise patch set in this lot that's > trying to get out ;) In answer to the above three, could split out into a separate patch, the splitting of sigreturn32 and kuser32, would that feel better and more concise? > Will -- Mark
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index dfd908630631..902ad4b5b3d3 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1100,6 +1100,35 @@ config COMPAT If you want to execute 32-bit userspace applications, say Y. +config KUSER_HELPERS + bool "Enable the kuser helpers page in 32-bit processes" + depends on COMPAT + default y + help + Warning: disabling this option may break 32-bit applications. + + Provide kuser helpers in a special purpose fixed-address page. 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 32-bit 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 32-bit binaries and libraries that 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 receive a SIGSEGV signal, which will terminate the + program. Typically, binaries compiled for ARMv7 or later do not use + the kuser helpers. + + 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 f2b4e816b6de..314b9abda0a0 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) -arm64-obj-$(CONFIG_COMPAT) += sys32.o kuser32.o signal32.o \ +arm64-obj-$(CONFIG_COMPAT) += sys32.o sigreturn32.o signal32.o \ sys_compat.o entry32.o +arm64-obj-$(CONFIG_KUSER_HELPERS) += kuser32.o arm64-obj-$(CONFIG_FUNCTION_TRACER) += ftrace.o entry-ftrace.o arm64-obj-$(CONFIG_MODULES) += arm64ksyms.o module.o arm64-obj-$(CONFIG_ARM64_MODULE_PLTS) += module-plts.o diff --git a/arch/arm64/kernel/kuser32.S b/arch/arm64/kernel/kuser32.S index 997e6b27ff6a..d15b5c2935b3 100644 --- a/arch/arm64/kernel/kuser32.S +++ b/arch/arm64/kernel/kuser32.S @@ -20,16 +20,13 @@ * * AArch32 user helpers. * - * 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. + * These helpers are provided for compatibility with AArch32 binaries that + * still need them. They are installed at a fixed address by + * aarch32_setup_additional_pages(). * * See Documentation/arm/kernel_user_helpers.txt for formal definitions. */ -#include <asm/unistd.h> - .align 5 .globl __kuser_helper_start __kuser_helper_start: @@ -77,42 +74,3 @@ __kuser_helper_version: // 0xffff0ffc .word ((__kuser_helper_end - __kuser_helper_start) >> 5) .globl __kuser_helper_end __kuser_helper_end: - -/* - * AArch32 sigreturn code - * - * For ARM syscalls, the syscall number has to be loaded into r7. - * We do not support an OABI userspace. - * - * For Thumb syscalls, we also pass the syscall number via r7. We therefore - * need two 16-bit instructions. - */ - .globl __aarch32_sigret_code_start -__aarch32_sigret_code_start: - - /* - * ARM Code - */ - .byte __NR_compat_sigreturn, 0x70, 0xa0, 0xe3 // mov r7, #__NR_compat_sigreturn - .byte __NR_compat_sigreturn, 0x00, 0x00, 0xef // svc #__NR_compat_sigreturn - - /* - * Thumb code - */ - .byte __NR_compat_sigreturn, 0x27 // svc #__NR_compat_sigreturn - .byte __NR_compat_sigreturn, 0xdf // mov r7, #__NR_compat_sigreturn - - /* - * ARM code - */ - .byte __NR_compat_rt_sigreturn, 0x70, 0xa0, 0xe3 // mov r7, #__NR_compat_rt_sigreturn - .byte __NR_compat_rt_sigreturn, 0x00, 0x00, 0xef // svc #__NR_compat_rt_sigreturn - - /* - * Thumb code - */ - .byte __NR_compat_rt_sigreturn, 0x27 // svc #__NR_compat_rt_sigreturn - .byte __NR_compat_rt_sigreturn, 0xdf // mov r7, #__NR_compat_rt_sigreturn - - .globl __aarch32_sigret_code_end -__aarch32_sigret_code_end: diff --git a/arch/arm64/kernel/sigreturn32.S b/arch/arm64/kernel/sigreturn32.S new file mode 100644 index 000000000000..6ecda4d84cd5 --- /dev/null +++ b/arch/arm64/kernel/sigreturn32.S @@ -0,0 +1,67 @@ +/* + * sigreturn trampolines for AArch32. + * + * Copyright (C) 2005-2011 Nicolas Pitre <nico@fluxnic.net> + * Copyright (C) 2012 ARM Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * + * AArch32 sigreturn code + * + * For ARM syscalls, the syscall number has to be loaded into r7. + * We do not support an OABI userspace. + * + * For Thumb syscalls, we also pass the syscall number via r7. We therefore + * need two 16-bit instructions. + */ + +#include <asm/unistd.h> + + .globl __aarch32_sigret_code_start +__aarch32_sigret_code_start: + + /* + * ARM Code + */ + // mov r7, #__NR_compat_sigreturn + .byte __NR_compat_sigreturn, 0x70, 0xa0, 0xe3 + // svc #__NR_compat_sigreturn + .byte __NR_compat_sigreturn, 0x00, 0x00, 0xef + + /* + * Thumb code + */ + // svc #__NR_compat_sigreturn + .byte __NR_compat_sigreturn, 0x27 + // mov r7, #__NR_compat_sigreturn + .byte __NR_compat_sigreturn, 0xdf + + /* + * ARM code + */ + // mov r7, #__NR_compat_rt_sigreturn + .byte __NR_compat_rt_sigreturn, 0x70, 0xa0, 0xe3 + // svc #__NR_compat_rt_sigreturn + .byte __NR_compat_rt_sigreturn, 0x00, 0x00, 0xef + + /* + * Thumb code + */ + // svc #__NR_compat_rt_sigreturn + .byte __NR_compat_rt_sigreturn, 0x27 + // mov r7, #__NR_compat_rt_sigreturn + .byte __NR_compat_rt_sigreturn, 0xdf + + .globl __aarch32_sigret_code_end +__aarch32_sigret_code_end: diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c index 456420b1f3f1..416183f9ae70 100644 --- a/arch/arm64/kernel/vdso.c +++ b/arch/arm64/kernel/vdso.c @@ -108,6 +108,8 @@ static int sigreturn_setup(struct mm_struct *mm) return PTR_ERR_OR_ZERO(ret); } +#ifdef CONFIG_KUSER_HELPERS + /* kuser helpers page */ static struct page *kuser_helpers_page __ro_after_init; static const struct vm_special_mapping kuser_helpers_spec = { @@ -151,6 +153,8 @@ static int kuser_helpers_setup(struct mm_struct *mm) return PTR_ERR_OR_ZERO(ret); } +#endif /* CONFIG_KUSER_HELPERS */ + int aarch32_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) { struct mm_struct *mm = current->mm; @@ -163,7 +167,9 @@ int aarch32_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) if (ret) goto out; +#ifdef CONFIG_KUSER_HELPERS ret = kuser_helpers_setup(mm); +#endif out: up_write(&mm->mmap_sem);