| Message ID | 1c05651c53f90d07e98ee4973c2786ccf315db12.1563904656.git.andreyknvl@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 7/23/19 10:58 AM, Andrey Konovalov wrote: > +long set_tagged_addr_ctrl(unsigned long arg) > +{ > + if (!tagged_addr_prctl_allowed) > + return -EINVAL; > + if (is_compat_task()) > + return -EINVAL; > + if (arg & ~PR_TAGGED_ADDR_ENABLE) > + return -EINVAL; > + > + update_thread_flag(TIF_TAGGED_ADDR, arg & PR_TAGGED_ADDR_ENABLE); > + > + return 0; > +} Instead of a plain enable/disable, a more flexible ABI would be to have the tag mask be passed in. That way, an implementation that has a flexible tag size can select it. It also ensures that userspace actually knows what the tag size is and isn't surprised if a hardware implementation changes the tag size or position. Also, this whole set deals with tagging/untagging, but there's an effective loss of address space when you do this. Is that dealt with anywhere? How do we ensure that allocations don't get placed at a tagged address before this gets turned on? Where's that checking?
On 31/07/2019 18:05, Dave Hansen wrote: > On 7/23/19 10:58 AM, Andrey Konovalov wrote: >> +long set_tagged_addr_ctrl(unsigned long arg) >> +{ >> + if (!tagged_addr_prctl_allowed) >> + return -EINVAL; >> + if (is_compat_task()) >> + return -EINVAL; >> + if (arg & ~PR_TAGGED_ADDR_ENABLE) >> + return -EINVAL; >> + >> + update_thread_flag(TIF_TAGGED_ADDR, arg & PR_TAGGED_ADDR_ENABLE); >> + >> + return 0; >> +} > Instead of a plain enable/disable, a more flexible ABI would be to have > the tag mask be passed in. That way, an implementation that has a > flexible tag size can select it. It also ensures that userspace > actually knows what the tag size is and isn't surprised if a hardware > implementation changes the tag size or position. > > Also, this whole set deals with tagging/untagging, but there's an > effective loss of address space when you do this. Is that dealt with > anywhere? How do we ensure that allocations don't get placed at a > tagged address before this gets turned on? Where's that checking? This patch series only changes what is allowed or not at the syscall interface. It does not change the address space size. On arm64, TBI (Top Byte Ignore) has always been enabled for userspace, so it has never been possible to use the upper 8 bits of user pointers for addressing. If other architectures were to support a similar functionality, then I agree that a common and more generic interface (if needed) would be helpful, but as it stands this is an arm64-specific prctl, and on arm64 the address tag is defined by the architecture as bits [63:56]. Kevin
On Thu, Aug 01, 2019 at 09:45:05AM -0700, Dave Hansen wrote: > On 8/1/19 5:38 AM, Kevin Brodsky wrote: > > This patch series only changes what is allowed or not at the syscall > > interface. It does not change the address space size. On arm64, TBI (Top > > Byte Ignore) has always been enabled for userspace, so it has never been > > possible to use the upper 8 bits of user pointers for addressing. > > Oh, so does the address space that's available already chop that out? Yes. Currently the hardware only supports 52-bit virtual addresses. It could be expanded (though it needs a 5th page table level) to 56-bit VA but it's not currently on our (hardware) plans. Beyond 56-bit, it cannot be done without breaking the software expectations (and hopefully I'll retire before we need this ;)). > > If other architectures were to support a similar functionality, then I > > agree that a common and more generic interface (if needed) would be > > helpful, but as it stands this is an arm64-specific prctl, and on arm64 > > the address tag is defined by the architecture as bits [63:56]. > > It should then be an arch_prctl(), no? I guess you just want renaming SET_TAGGED_ADDR_CTRL() to arch_prctl_tagged_addr_ctrl_set()? (similarly for 'get') -- Catalin
On Tue, Jul 23, 2019 at 07:58:39PM +0200, Andrey Konovalov wrote: > From: Catalin Marinas <catalin.marinas@arm.com> > > It is not desirable to relax the ABI to allow tagged user addresses into > the kernel indiscriminately. This patch introduces a prctl() interface > for enabling or disabling the tagged ABI with a global sysctl control > for preventing applications from enabling the relaxed ABI (meant for > testing user-space prctl() return error checking without reconfiguring > the kernel). The ABI properties are inherited by threads of the same > application and fork()'ed children but cleared on execve(). A Kconfig > option allows the overall disabling of the relaxed ABI. > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > MTE-specific settings like imprecise vs precise exceptions. > > Reviewed-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> Following several discussions on the list and in private, I'm proposing the update below. I can send it as a patch on top of the current series since Will has already queued this. ---------------8<------------------------------------- From 1b3f57ab0c2c51f8b31c19fb34d270e1f3ee57fe Mon Sep 17 00:00:00 2001 From: Catalin Marinas <catalin.marinas@arm.com> Date: Fri, 9 Aug 2019 15:09:15 +0100 Subject: [PATCH] fixup! arm64: Introduce prctl() options to control the tagged user addresses ABI Rename abi.tagged_addr sysctl control to abi.tagged_addr_disabled, defaulting to 0. Only prevent prctl(PR_TAGGED_ADDR_ENABLE)from being called when abi.tagged_addr_disabled==1. Force unused arg* of the new prctl() to 0. Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> --- arch/arm64/kernel/process.c | 17 ++++++++++------- kernel/sys.c | 4 ++++ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 76b7c55026aa..03689c0beb34 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -579,17 +579,22 @@ void arch_setup_new_exec(void) /* * Control the relaxed ABI allowing tagged user addresses into the kernel. */ -static unsigned int tagged_addr_prctl_allowed = 1; +static unsigned int tagged_addr_disabled; long set_tagged_addr_ctrl(unsigned long arg) { - if (!tagged_addr_prctl_allowed) - return -EINVAL; if (is_compat_task()) return -EINVAL; if (arg & ~PR_TAGGED_ADDR_ENABLE) return -EINVAL; + /* + * Do not allow the enabling of the tagged address ABI if globally + * disabled via sysctl abi.tagged_addr_disabled. + */ + if (arg & PR_TAGGED_ADDR_ENABLE && tagged_addr_disabled) + return -EINVAL; + update_thread_flag(TIF_TAGGED_ADDR, arg & PR_TAGGED_ADDR_ENABLE); return 0; @@ -597,8 +602,6 @@ long set_tagged_addr_ctrl(unsigned long arg) long get_tagged_addr_ctrl(void) { - if (!tagged_addr_prctl_allowed) - return -EINVAL; if (is_compat_task()) return -EINVAL; @@ -618,9 +621,9 @@ static int one = 1; static struct ctl_table tagged_addr_sysctl_table[] = { { - .procname = "tagged_addr", + .procname = "tagged_addr_disabled", .mode = 0644, - .data = &tagged_addr_prctl_allowed, + .data = &tagged_addr_disabled, .maxlen = sizeof(int), .proc_handler = proc_dointvec_minmax, .extra1 = &zero, diff --git a/kernel/sys.c b/kernel/sys.c index c6c4d5358bd3..ec48396b4943 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2499,9 +2499,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, error = PAC_RESET_KEYS(me, arg2); break; case PR_SET_TAGGED_ADDR_CTRL: + if (arg3 || arg4 || arg5) + return -EINVAL; error = SET_TAGGED_ADDR_CTRL(arg2); break; case PR_GET_TAGGED_ADDR_CTRL: + if (arg2 || arg3 || arg4 || arg5) + return -EINVAL; error = GET_TAGGED_ADDR_CTRL(); break; default:
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 3adcec05b1f6..5d254178b9ca 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1110,6 +1110,15 @@ config ARM64_SW_TTBR0_PAN zeroed area and reserved ASID. The user access routines restore the valid TTBR0_EL1 temporarily. +config ARM64_TAGGED_ADDR_ABI + bool "Enable the tagged user addresses syscall ABI" + default y + help + When this option is enabled, user applications can opt in to a + relaxed ABI via prctl() allowing tagged addresses to be passed + to system calls as pointer arguments. For details, see + Documentation/arm64/tagged-address-abi.txt. + menuconfig COMPAT bool "Kernel support for 32-bit EL0" depends on ARM64_4K_PAGES || EXPERT diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index fd5b1a4efc70..ee86070a28d4 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -296,6 +296,14 @@ extern void __init minsigstksz_setup(void); /* PR_PAC_RESET_KEYS prctl */ #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) +#ifdef CONFIG_ARM64_TAGGED_ADDR_ABI +/* PR_{SET,GET}_TAGGED_ADDR_CTRL prctl */ +long set_tagged_addr_ctrl(unsigned long arg); +long get_tagged_addr_ctrl(void); +#define SET_TAGGED_ADDR_CTRL(arg) set_tagged_addr_ctrl(arg) +#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl() +#endif + /* * For CONFIG_GCC_PLUGIN_STACKLEAK * diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h index 180b34ec5965..012238d8e58d 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -90,6 +90,7 @@ void arch_release_task_struct(struct task_struct *tsk); #define TIF_SVE 23 /* Scalable Vector Extension in use */ #define TIF_SVE_VL_INHERIT 24 /* Inherit sve_vl_onexec across exec */ #define TIF_SSBD 25 /* Wants SSB mitigation */ +#define TIF_TAGGED_ADDR 26 /* Allow tagged user addresses */ #define _TIF_SIGPENDING (1 << TIF_SIGPENDING) #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index a138e3b4f717..097d6bfac0b7 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -62,7 +62,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si { unsigned long ret, limit = current_thread_info()->addr_limit; - addr = untagged_addr(addr); + if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI) && + test_thread_flag(TIF_TAGGED_ADDR)) + addr = untagged_addr(addr); __chk_user_ptr(addr); asm volatile( diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 6a869d9f304f..ef06a303bda0 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -19,6 +19,7 @@ #include <linux/kernel.h> #include <linux/mm.h> #include <linux/stddef.h> +#include <linux/sysctl.h> #include <linux/unistd.h> #include <linux/user.h> #include <linux/delay.h> @@ -38,6 +39,7 @@ #include <trace/events/power.h> #include <linux/percpu.h> #include <linux/thread_info.h> +#include <linux/prctl.h> #include <asm/alternative.h> #include <asm/arch_gicv3.h> @@ -307,11 +309,18 @@ static void tls_thread_flush(void) } } +static void flush_tagged_addr_state(void) +{ + if (IS_ENABLED(CONFIG_ARM64_TAGGED_ADDR_ABI)) + clear_thread_flag(TIF_TAGGED_ADDR); +} + void flush_thread(void) { fpsimd_flush_thread(); tls_thread_flush(); flush_ptrace_hw_breakpoint(current); + flush_tagged_addr_state(); } void release_thread(struct task_struct *dead_task) @@ -541,3 +550,67 @@ void arch_setup_new_exec(void) ptrauth_thread_init_user(current); } + +#ifdef CONFIG_ARM64_TAGGED_ADDR_ABI +/* + * Control the relaxed ABI allowing tagged user addresses into the kernel. + */ +static unsigned int tagged_addr_prctl_allowed = 1; + +long set_tagged_addr_ctrl(unsigned long arg) +{ + if (!tagged_addr_prctl_allowed) + return -EINVAL; + if (is_compat_task()) + return -EINVAL; + if (arg & ~PR_TAGGED_ADDR_ENABLE) + return -EINVAL; + + update_thread_flag(TIF_TAGGED_ADDR, arg & PR_TAGGED_ADDR_ENABLE); + + return 0; +} + +long get_tagged_addr_ctrl(void) +{ + if (!tagged_addr_prctl_allowed) + return -EINVAL; + if (is_compat_task()) + return -EINVAL; + + if (test_thread_flag(TIF_TAGGED_ADDR)) + return PR_TAGGED_ADDR_ENABLE; + + return 0; +} + +/* + * Global sysctl to disable the tagged user addresses support. This control + * only prevents the tagged address ABI enabling via prctl() and does not + * disable it for tasks that already opted in to the relaxed ABI. + */ +static int zero; +static int one = 1; + +static struct ctl_table tagged_addr_sysctl_table[] = { + { + .procname = "tagged_addr", + .mode = 0644, + .data = &tagged_addr_prctl_allowed, + .maxlen = sizeof(int), + .proc_handler = proc_dointvec_minmax, + .extra1 = &zero, + .extra2 = &one, + }, + { } +}; + +static int __init tagged_addr_init(void) +{ + if (!register_sysctl("abi", tagged_addr_sysctl_table)) + return -EINVAL; + return 0; +} + +core_initcall(tagged_addr_init); +#endif /* CONFIG_ARM64_TAGGED_ADDR_ABI */ diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h index 094bb03b9cc2..2e927b3e9d6c 100644 --- a/include/uapi/linux/prctl.h +++ b/include/uapi/linux/prctl.h @@ -229,4 +229,9 @@ struct prctl_mm_map { # define PR_PAC_APDBKEY (1UL << 3) # define PR_PAC_APGAKEY (1UL << 4) +/* Tagged user address controls for arm64 */ +#define PR_SET_TAGGED_ADDR_CTRL 55 +#define PR_GET_TAGGED_ADDR_CTRL 56 +# define PR_TAGGED_ADDR_ENABLE (1UL << 0) + #endif /* _LINUX_PRCTL_H */ diff --git a/kernel/sys.c b/kernel/sys.c index 2969304c29fe..c6c4d5358bd3 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -124,6 +124,12 @@ #ifndef PAC_RESET_KEYS # define PAC_RESET_KEYS(a, b) (-EINVAL) #endif +#ifndef SET_TAGGED_ADDR_CTRL +# define SET_TAGGED_ADDR_CTRL(a) (-EINVAL) +#endif +#ifndef GET_TAGGED_ADDR_CTRL +# define GET_TAGGED_ADDR_CTRL() (-EINVAL) +#endif /* * this is where the system-wide overflow UID and GID are defined, for @@ -2492,6 +2498,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, return -EINVAL; error = PAC_RESET_KEYS(me, arg2); break; + case PR_SET_TAGGED_ADDR_CTRL: + error = SET_TAGGED_ADDR_CTRL(arg2); + break; + case PR_GET_TAGGED_ADDR_CTRL: + error = GET_TAGGED_ADDR_CTRL(); + break; default: error = -EINVAL; break;