| Message ID | a7a2933bea5fe57e504891b7eec7e9432e5e1c1a.1560339705.git.andreyknvl@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 12/06/2019 12:43, 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(). > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > MTE-specific settings like imprecise vs precise exceptions. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> Reviewed-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > arch/arm64/include/asm/processor.h | 6 +++ > arch/arm64/include/asm/thread_info.h | 1 + > arch/arm64/include/asm/uaccess.h | 3 +- > arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++ > include/uapi/linux/prctl.h | 5 +++ > kernel/sys.c | 16 +++++++ > 6 files changed, 97 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index fcd0e691b1ea..fee457456aa8 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void); > /* PR_PAC_RESET_KEYS prctl */ > #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) > > +/* PR_TAGGED_ADDR 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() > + > /* > * For CONFIG_GCC_PLUGIN_STACKLEAK > * > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > index f1d032be628a..354a31d2b737 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -99,6 +99,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 df729afca0ba..995b9ea11a89 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -73,7 +73,8 @@ 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 (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 3767fb21a5b8..69d0be1fc708 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -30,6 +30,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> > @@ -323,6 +324,7 @@ void flush_thread(void) > fpsimd_flush_thread(); > tls_thread_flush(); > flush_ptrace_hw_breakpoint(current); > + clear_thread_flag(TIF_TAGGED_ADDR); > } > > void release_thread(struct task_struct *dead_task) > @@ -552,3 +554,68 @@ void arch_setup_new_exec(void) > > ptrauth_thread_init_user(current); > } > + > +/* > + * 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; > + > + if (arg & PR_TAGGED_ADDR_ENABLE) > + set_thread_flag(TIF_TAGGED_ADDR); > + else > + clear_thread_flag(TIF_TAGGED_ADDR); > + > + 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); > 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..ec48396b4943 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,16 @@ 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: > + 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: > error = -EINVAL; > break; > -- Regards, Vincenzo
On Wed, Jun 12, 2019 at 01:43:20PM +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(). > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > MTE-specific settings like imprecise vs precise exceptions. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> You need your signed-off-by here since you are contributing it. And thanks for adding the comment to the TIF definition. -- Catalin
On Wed, Jun 12, 2019 at 01:43:20PM +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(). > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > MTE-specific settings like imprecise vs precise exceptions. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > --- > arch/arm64/include/asm/processor.h | 6 +++ > arch/arm64/include/asm/thread_info.h | 1 + > arch/arm64/include/asm/uaccess.h | 3 +- > arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++ > include/uapi/linux/prctl.h | 5 +++ > kernel/sys.c | 16 +++++++ > 6 files changed, 97 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index fcd0e691b1ea..fee457456aa8 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void); > /* PR_PAC_RESET_KEYS prctl */ > #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) > > +/* PR_TAGGED_ADDR prctl */ (A couple of comments I missed in my last reply:) Name mismatch? > +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() > + [...] > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 3767fb21a5b8..69d0be1fc708 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -30,6 +30,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> > @@ -323,6 +324,7 @@ void flush_thread(void) > fpsimd_flush_thread(); > tls_thread_flush(); > flush_ptrace_hw_breakpoint(current); > + clear_thread_flag(TIF_TAGGED_ADDR); > } > > void release_thread(struct task_struct *dead_task) > @@ -552,3 +554,68 @@ void arch_setup_new_exec(void) > > ptrauth_thread_init_user(current); > } > + > +/* > + * 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; So, tagging can actually be locked on by having a process enable it and then some possibly unrelated process clearing tagged_addr_prctl_allowed. That feels a bit weird. Do we want to allow a process that has tagging on to be able to turn it off at all? Possibly things like CRIU might want to do that. > + if (is_compat_task()) > + return -EINVAL; > + if (arg & ~PR_TAGGED_ADDR_ENABLE) > + return -EINVAL; How do we expect this argument to be extended in the future? I'm wondering whether this is really a bitmask or an enum, or a mixture of the two. Maybe it doesn't matter. > + > + if (arg & PR_TAGGED_ADDR_ENABLE) > + set_thread_flag(TIF_TAGGED_ADDR); > + else > + clear_thread_flag(TIF_TAGGED_ADDR); I think update_thread_flag() could be used here. [...] Cheers ---Dave
Hi Dave, On Thu, Jun 13, 2019 at 12:02:35PM +0100, Dave P Martin wrote: > On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote: > > +/* > > + * 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; > > !!! > > And these can't even be const without a cast. Yuk. > > (Not your fault though, but it would be nice to have a proc_dobool() to > avoid this.) I had the same reaction. Maybe for another patch sanitising this pattern across the kernel. > > --- 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) > > + > > Do we expect this prctl to be applicable to other arches, or is it > strictly arm64-specific? I kept it generic, at least the tagged address part. The MTE bits later on would be arm64-specific. > > @@ -2492,6 +2498,16 @@ 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: > > + if (arg3 || arg4 || arg5) > > <bikeshed> > > How do you anticipate these arguments being used in the future? I don't expect them to be used at all. But since I'm not sure, I'd force them as zero for now rather than ignored. The GET is supposed to return the SET arg2, hence I'd rather not used the other arguments. > For the SVE prctls I took the view that "get" could only ever mean one > thing, and "put" already had a flags argument with spare bits for future > expansion anyway, so forcing the extra arguments to zero would be > unnecessary. > > Opinions seem to differ on whether requiring surplus arguments to be 0 > is beneficial for hygiene, but the glibc prototype for prctl() is > > int prctl (int __option, ...); > > so it seemed annoying to have to pass extra arguments to it just for the > sake of it. IMHO this also makes the code at the call site less > readable, since it's not immediately apparent that all those 0s are > meaningless. It's fine by me to ignore the other arguments. I just followed the pattern of some existing prctl options. I don't have a strong opinion either way. > > + 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(); > > Having a "get" prctl is probably a good idea, but is there a clear > usecase for it? Not sure, maybe some other library (e.g. a JIT compiler) would like to check whether tagged addresses have been enabled during application start and decide to generate tagged pointers for itself. It seemed pretty harmless, unless we add more complex things to the prctl() that cannot be returned in one request). -- Catalin
On Thu, Jun 13, 2019 at 12:16:59PM +0100, Dave P Martin wrote: > On Wed, Jun 12, 2019 at 01:43:20PM +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(). > > > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > > MTE-specific settings like imprecise vs precise exceptions. > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > --- > > arch/arm64/include/asm/processor.h | 6 +++ > > arch/arm64/include/asm/thread_info.h | 1 + > > arch/arm64/include/asm/uaccess.h | 3 +- > > arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++ > > include/uapi/linux/prctl.h | 5 +++ > > kernel/sys.c | 16 +++++++ > > 6 files changed, 97 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > > index fcd0e691b1ea..fee457456aa8 100644 > > --- a/arch/arm64/include/asm/processor.h > > +++ b/arch/arm64/include/asm/processor.h > > @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void); > > /* PR_PAC_RESET_KEYS prctl */ > > #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) > > > > +/* PR_TAGGED_ADDR prctl */ > > (A couple of comments I missed in my last reply:) > > Name mismatch? Yeah, it went through several names but it seems that I didn't update all places. > > +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() > > + > > [...] > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > index 3767fb21a5b8..69d0be1fc708 100644 > > --- a/arch/arm64/kernel/process.c > > +++ b/arch/arm64/kernel/process.c > > @@ -30,6 +30,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> > > @@ -323,6 +324,7 @@ void flush_thread(void) > > fpsimd_flush_thread(); > > tls_thread_flush(); > > flush_ptrace_hw_breakpoint(current); > > + clear_thread_flag(TIF_TAGGED_ADDR); > > } > > > > void release_thread(struct task_struct *dead_task) > > @@ -552,3 +554,68 @@ void arch_setup_new_exec(void) > > > > ptrauth_thread_init_user(current); > > } > > + > > +/* > > + * 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; > > So, tagging can actually be locked on by having a process enable it and > then some possibly unrelated process clearing tagged_addr_prctl_allowed. > That feels a bit weird. The problem is that if you disable the ABI globally, lots of applications would crash. This sysctl is meant as a way to disable the opt-in to the TBI ABI. Another option would be a kernel command line option (I'm not keen on a Kconfig option). > Do we want to allow a process that has tagging on to be able to turn > it off at all? Possibly things like CRIU might want to do that. I left it in for symmetry but I don't expect it to be used. A potential use-case is doing it per subsequent threads in an application. > > + if (is_compat_task()) > > + return -EINVAL; > > + if (arg & ~PR_TAGGED_ADDR_ENABLE) > > + return -EINVAL; > > How do we expect this argument to be extended in the future? Yes, for MTE. That's why I wouldn't allow random bits here. > I'm wondering whether this is really a bitmask or an enum, or a mixture > of the two. Maybe it doesn't matter. User may want to set PR_TAGGED_ADDR_ENABLE | PR_MTE_PRECISE in a single call. > > + if (arg & PR_TAGGED_ADDR_ENABLE) > > + set_thread_flag(TIF_TAGGED_ADDR); > > + else > > + clear_thread_flag(TIF_TAGGED_ADDR); > > I think update_thread_flag() could be used here. Yes. I forgot you added this. -- Catalin
On 13/06/2019 16:35, Catalin Marinas wrote: > On Thu, Jun 13, 2019 at 12:16:59PM +0100, Dave P Martin wrote: >> On Wed, Jun 12, 2019 at 01:43:20PM +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(). >>> >>> The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle >>> MTE-specific settings like imprecise vs precise exceptions. >>> >>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> >>> --- >>> arch/arm64/include/asm/processor.h | 6 +++ >>> arch/arm64/include/asm/thread_info.h | 1 + >>> arch/arm64/include/asm/uaccess.h | 3 +- >>> arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++ >>> include/uapi/linux/prctl.h | 5 +++ >>> kernel/sys.c | 16 +++++++ >>> 6 files changed, 97 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h >>> index fcd0e691b1ea..fee457456aa8 100644 >>> --- a/arch/arm64/include/asm/processor.h >>> +++ b/arch/arm64/include/asm/processor.h >>> @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void); >>> /* PR_PAC_RESET_KEYS prctl */ >>> #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) >>> >>> +/* PR_TAGGED_ADDR prctl */ >> >> (A couple of comments I missed in my last reply:) >> >> Name mismatch? > > Yeah, it went through several names but it seems that I didn't update > all places. > >>> +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() >>> + >> >> [...] >> >>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>> index 3767fb21a5b8..69d0be1fc708 100644 >>> --- a/arch/arm64/kernel/process.c >>> +++ b/arch/arm64/kernel/process.c >>> @@ -30,6 +30,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> >>> @@ -323,6 +324,7 @@ void flush_thread(void) >>> fpsimd_flush_thread(); >>> tls_thread_flush(); >>> flush_ptrace_hw_breakpoint(current); >>> + clear_thread_flag(TIF_TAGGED_ADDR); >>> } >>> >>> void release_thread(struct task_struct *dead_task) >>> @@ -552,3 +554,68 @@ void arch_setup_new_exec(void) >>> >>> ptrauth_thread_init_user(current); >>> } >>> + >>> +/* >>> + * 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; >> >> So, tagging can actually be locked on by having a process enable it and >> then some possibly unrelated process clearing tagged_addr_prctl_allowed. >> That feels a bit weird. > > The problem is that if you disable the ABI globally, lots of > applications would crash. This sysctl is meant as a way to disable the > opt-in to the TBI ABI. Another option would be a kernel command line > option (I'm not keen on a Kconfig option). > Why you are not keen on a Kconfig option? >> Do we want to allow a process that has tagging on to be able to turn >> it off at all? Possibly things like CRIU might want to do that. > > I left it in for symmetry but I don't expect it to be used. A potential > use-case is doing it per subsequent threads in an application. > >>> + if (is_compat_task()) >>> + return -EINVAL; >>> + if (arg & ~PR_TAGGED_ADDR_ENABLE) >>> + return -EINVAL; >> >> How do we expect this argument to be extended in the future? > > Yes, for MTE. That's why I wouldn't allow random bits here. > >> I'm wondering whether this is really a bitmask or an enum, or a mixture >> of the two. Maybe it doesn't matter. > > User may want to set PR_TAGGED_ADDR_ENABLE | PR_MTE_PRECISE in a single > call. > >>> + if (arg & PR_TAGGED_ADDR_ENABLE) >>> + set_thread_flag(TIF_TAGGED_ADDR); >>> + else >>> + clear_thread_flag(TIF_TAGGED_ADDR); >> >> I think update_thread_flag() could be used here. > > Yes. I forgot you added this. > -- Regards, Vincenzo
On Thu, Jun 13, 2019 at 04:45:54PM +0100, Vincenzo Frascino wrote: > On 13/06/2019 16:35, Catalin Marinas wrote: > > On Thu, Jun 13, 2019 at 12:16:59PM +0100, Dave P Martin wrote: > >> On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote: > >>> + > >>> +/* > >>> + * 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; > >> > >> So, tagging can actually be locked on by having a process enable it and > >> then some possibly unrelated process clearing tagged_addr_prctl_allowed. > >> That feels a bit weird. > > > > The problem is that if you disable the ABI globally, lots of > > applications would crash. This sysctl is meant as a way to disable the > > opt-in to the TBI ABI. Another option would be a kernel command line > > option (I'm not keen on a Kconfig option). > > Why you are not keen on a Kconfig option? Because I don't want to rebuild the kernel/reboot just to be able to test how user space handles the ABI opt-in. I'm ok with a Kconfig option to disable this globally in addition to a run-time option (if actually needed, I'm not sure). -- Catalin
On Thu, Jun 13, 2019 at 04:26:32PM +0100, Catalin Marinas wrote: > On Thu, Jun 13, 2019 at 12:02:35PM +0100, Dave P Martin wrote: > > On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote: > > > +static int zero; > > > +static int one = 1; > > > > !!! > > > > And these can't even be const without a cast. Yuk. > > > > (Not your fault though, but it would be nice to have a proc_dobool() to > > avoid this.) > > I had the same reaction. Maybe for another patch sanitising this pattern > across the kernel. That's actually already happening (via -mm tree last I looked). tl;dr: it ends up using a cast hidden in a macro. It's in linux-next already along with a checkpatch.pl addition to yell about doing what's being done here. ;) https://lore.kernel.org/lkml/20190430180111.10688-1-mcroce@redhat.com/#r -- Kees Cook
On Wed, Jun 12, 2019 at 01:43:20PM +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(). > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > MTE-specific settings like imprecise vs precise exceptions. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> A question for the user-space folk: if an application opts in to this ABI, would you want the sigcontext.fault_address and/or siginfo.si_addr to contain the tag? We currently clear it early in the arm64 entry.S but we could find a way to pass it down if needed. -- Catalin
On 17/06/2019 14:56, Catalin Marinas wrote: > On Wed, Jun 12, 2019 at 01:43:20PM +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(). >> >> The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle >> MTE-specific settings like imprecise vs precise exceptions. >> >> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > A question for the user-space folk: if an application opts in to this > ABI, would you want the sigcontext.fault_address and/or siginfo.si_addr > to contain the tag? We currently clear it early in the arm64 entry.S but > we could find a way to pass it down if needed. to me it makes sense to keep the tag in si_addr / fault_address. but i don't know in detail how those fields are used currently. keeping the tag is certainly useful for MTE to debug wrong tag failures unless there is a separate mechanism for that.
On Mon, Jun 17, 2019 at 09:57:36AM -0700, Evgenii Stepanov wrote: > On Mon, Jun 17, 2019 at 6:56 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Wed, Jun 12, 2019 at 01:43:20PM +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(). > > > > > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > > > MTE-specific settings like imprecise vs precise exceptions. > > > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > > A question for the user-space folk: if an application opts in to this > > ABI, would you want the sigcontext.fault_address and/or siginfo.si_addr > > to contain the tag? We currently clear it early in the arm64 entry.S but > > we could find a way to pass it down if needed. > > For HWASan this would not be useful because we instrument memory > accesses with explicit checks anyway. For MTE, on the other hand, it > would be very convenient to know the fault address tag without > disassembling the code. I could as this differently: does anything break if, once the user opts in to TBI, fault_address and/or si_addr have non-zero top byte? Alternatively, we could present the original FAR_EL1 register as a separate field as we do with ESR_EL1, independently of whether the user opted in to TBI or not. -- Catalin
On Mon, Jun 17, 2019 at 10:18 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Jun 17, 2019 at 09:57:36AM -0700, Evgenii Stepanov wrote: > > On Mon, Jun 17, 2019 at 6:56 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > On Wed, Jun 12, 2019 at 01:43:20PM +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(). > > > > > > > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > > > > MTE-specific settings like imprecise vs precise exceptions. > > > > > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > > > > A question for the user-space folk: if an application opts in to this > > > ABI, would you want the sigcontext.fault_address and/or siginfo.si_addr > > > to contain the tag? We currently clear it early in the arm64 entry.S but > > > we could find a way to pass it down if needed. > > > > For HWASan this would not be useful because we instrument memory > > accesses with explicit checks anyway. For MTE, on the other hand, it > > would be very convenient to know the fault address tag without > > disassembling the code. > > I could as this differently: does anything break if, once the user > opts in to TBI, fault_address and/or si_addr have non-zero top byte? I think it would be fine. > Alternatively, we could present the original FAR_EL1 register as a > separate field as we do with ESR_EL1, independently of whether the user > opted in to TBI or not. > > -- > Catalin
On Thu, Jun 13, 2019 at 10:13:54PM -0700, Kees Cook wrote: > On Thu, Jun 13, 2019 at 04:26:32PM +0100, Catalin Marinas wrote: > > On Thu, Jun 13, 2019 at 12:02:35PM +0100, Dave P Martin wrote: > > > On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote: > > > > +static int zero; > > > > +static int one = 1; > > > > > > !!! > > > > > > And these can't even be const without a cast. Yuk. > > > > > > (Not your fault though, but it would be nice to have a proc_dobool() to > > > avoid this.) > > > > I had the same reaction. Maybe for another patch sanitising this pattern > > across the kernel. > > That's actually already happening (via -mm tree last I looked). tl;dr: > it ends up using a cast hidden in a macro. It's in linux-next already > along with a checkpatch.pl addition to yell about doing what's being > done here. ;) > > https://lore.kernel.org/lkml/20190430180111.10688-1-mcroce@redhat.com/#r Hmmm, that is marginally less bad. Ideally we'd have a union in there, not just a bunch of void *. I may look at that someday... Cheers ---Dave
On Wed, Jun 12, 2019 at 1:43 PM Andrey Konovalov <andreyknvl@google.com> 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(). > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > MTE-specific settings like imprecise vs precise exceptions. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> Catalin, would you like to do the requested changes to this patch yourself and send it to me or should I do that?
On Wed, Jun 19, 2019 at 04:45:02PM +0200, Andrey Konovalov wrote: > On Wed, Jun 12, 2019 at 1:43 PM Andrey Konovalov <andreyknvl@google.com> 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(). > > > > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle > > MTE-specific settings like imprecise vs precise exceptions. > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > Catalin, would you like to do the requested changes to this patch > yourself and send it to me or should I do that? I'll send you an updated version this week. -- Catalin
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index fcd0e691b1ea..fee457456aa8 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void); /* PR_PAC_RESET_KEYS prctl */ #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) +/* PR_TAGGED_ADDR 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() + /* * For CONFIG_GCC_PLUGIN_STACKLEAK * diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h index f1d032be628a..354a31d2b737 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -99,6 +99,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 df729afca0ba..995b9ea11a89 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -73,7 +73,8 @@ 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 (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 3767fb21a5b8..69d0be1fc708 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -30,6 +30,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> @@ -323,6 +324,7 @@ void flush_thread(void) fpsimd_flush_thread(); tls_thread_flush(); flush_ptrace_hw_breakpoint(current); + clear_thread_flag(TIF_TAGGED_ADDR); } void release_thread(struct task_struct *dead_task) @@ -552,3 +554,68 @@ void arch_setup_new_exec(void) ptrauth_thread_init_user(current); } + +/* + * 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; + + if (arg & PR_TAGGED_ADDR_ENABLE) + set_thread_flag(TIF_TAGGED_ADDR); + else + clear_thread_flag(TIF_TAGGED_ADDR); + + 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); 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..ec48396b4943 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,16 @@ 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: + 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: error = -EINVAL; break;