| Message ID | 4327b260fb17c4776a1e3c844f388e4948cfb747.1559580831.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: untag user pointers passed to the kernel | expand |
On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > This patch is a part of a series that extends arm64 kernel ABI to allow to > pass tagged user pointers (with the top byte set to something else other > than 0x00) as syscall arguments. > > copy_from_user (and a few other similar functions) are used to copy data > from user memory into the kernel memory or vice versa. Since a user can > provided a tagged pointer to one of the syscalls that use copy_from_user, > we need to correctly handle such pointers. > > Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr, > before performing access validity checks. > > Note, that this patch only temporarily untags the pointers to perform the > checks, but then passes them as is into the kernel internals. > > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com> Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > --- > arch/arm64/include/asm/uaccess.h | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index e5d5f31c6d36..9164ecb5feca 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > return ret; > } > > -#define access_ok(addr, size) __range_ok(addr, size) > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > #define user_addr_max get_fs > > #define _ASM_EXTABLE(from, to) \ > @@ -226,7 +226,8 @@ static inline void uaccess_enable_not_uao(void) > > /* > * Sanitise a uaccess pointer such that it becomes NULL if above the > - * current addr_limit. > + * current addr_limit. In case the pointer is tagged (has the top byte set), > + * untag the pointer before checking. > */ > #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr) > static inline void __user *__uaccess_mask_ptr(const void __user *ptr) > @@ -234,10 +235,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) > void __user *safe_ptr; > > asm volatile( > - " bics xzr, %1, %2\n" > + " bics xzr, %3, %2\n" > " csel %0, %1, xzr, eq\n" > : "=&r" (safe_ptr) > - : "r" (ptr), "r" (current_thread_info()->addr_limit) > + : "r" (ptr), "r" (current_thread_info()->addr_limit), > + "r" (untagged_addr(ptr)) > : "cc"); > > csdb(); > -- > 2.22.0.rc1.311.g5d7573a151-goog >
On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index e5d5f31c6d36..9164ecb5feca 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > return ret; > } > > -#define access_ok(addr, size) __range_ok(addr, size) > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) I'm going to propose an opt-in method here (RFC for now). We can't have a check in untagged_addr() since this is already used throughout the kernel for both user and kernel addresses (khwasan) but we can add one in __range_ok(). The same prctl() option will be used for controlling the precise/imprecise mode of MTE later on. We can use a TIF_ flag here assuming that this will be called early on and any cloned thread will inherit this. Anyway, it's easier to paste some diff than explain but Vincenzo can fold them into his ABI patches that should really go together with these. I added a couple of MTE definitions for prctl() as an example, not used currently: ------------------8<--------------------------------------------- diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index fcd0e691b1ea..2d4cb7e4edab 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -307,6 +307,10 @@ extern void __init minsigstksz_setup(void); /* PR_PAC_RESET_KEYS prctl */ #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) +/* PR_UNTAGGED_UADDR prctl */ +int untagged_uaddr_set_mode(unsigned long arg); +#define SET_UNTAGGED_UADDR_MODE(arg) untagged_uaddr_set_mode(arg) + /* * For CONFIG_GCC_PLUGIN_STACKLEAK * diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h index c285d1ce7186..89ce77773c49 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -101,6 +101,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_UNTAGGED_UADDR 26 #define _TIF_SIGPENDING (1 << TIF_SIGPENDING) #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) @@ -116,6 +117,7 @@ void arch_release_task_struct(struct task_struct *tsk); #define _TIF_FSCHECK (1 << TIF_FSCHECK) #define _TIF_32BIT (1 << TIF_32BIT) #define _TIF_SVE (1 << TIF_SVE) +#define _TIF_UNTAGGED_UADDR (1 << TIF_UNTAGGED_UADDR) #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 9164ecb5feca..54f5bbaebbc4 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si { unsigned long ret, limit = current_thread_info()->addr_limit; + if (test_thread_flag(TIF_UNTAGGED_UADDR)) + addr = untagged_addr(addr); + __chk_user_ptr(addr); asm volatile( // A + B <= C + 1 for all A,B,C, in four easy steps: @@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si return ret; } -#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) +#define access_ok(addr, size) __range_ok(addr, size) #define user_addr_max get_fs #define _ASM_EXTABLE(from, to) \ diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 3767fb21a5b8..fd191c5b92aa 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -552,3 +552,18 @@ void arch_setup_new_exec(void) ptrauth_thread_init_user(current); } + +/* + * Enable the relaxed ABI allowing tagged user addresses into the kernel. + */ +int untagged_uaddr_set_mode(unsigned long arg) +{ + if (is_compat_task()) + return -ENOTSUPP; + if (arg) + return -EINVAL; + + set_thread_flag(TIF_UNTAGGED_UADDR); + + return 0; +} diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h index 094bb03b9cc2..4afd5e2980ee 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) +/* Untagged user addresses for arm64 */ +#define PR_UNTAGGED_UADDR 55 +# define PR_MTE_IMPRECISE_CHECK 0 +# define PR_MTE_PRECISE_CHECK 1 + #endif /* _LINUX_PRCTL_H */ diff --git a/kernel/sys.c b/kernel/sys.c index 2969304c29fe..b1f67a8cffc4 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -124,6 +124,9 @@ #ifndef PAC_RESET_KEYS # define PAC_RESET_KEYS(a, b) (-EINVAL) #endif +#ifndef SET_UNTAGGED_UADDR_MODE +# define SET_UNTAGGED_UADDR_MODE (-EINVAL) +#endif /* * this is where the system-wide overflow UID and GID are defined, for @@ -2492,6 +2495,11 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, return -EINVAL; error = PAC_RESET_KEYS(me, arg2); break; + case PR_UNTAGGED_UADDR: + if (arg3 || arg4 || arg5) + return -EINVAL; + error = SET_UNTAGGED_UADDR_MODE(arg2); + break; default: error = -EINVAL; break; ------------------8<--------------------------------------------- The tag_ptr() function in the test library would become: static void *tag_ptr(void *ptr) { static int tbi_enabled = 0; unsigned long tag = 0; if (!tbi_enabled) { if (prctl(PR_UNTAGGED_UADDR, 0, 0, 0, 0) == 0) tbi_enabled = 1; } if (!ptr) return ptr; if (tbi_enabled) tag = rand() & 0xff; return (void *)((unsigned long)ptr | (tag << TAG_SHIFT)); }
On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > index e5d5f31c6d36..9164ecb5feca 100644 > > --- a/arch/arm64/include/asm/uaccess.h > > +++ b/arch/arm64/include/asm/uaccess.h > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > > return ret; > > } > > > > -#define access_ok(addr, size) __range_ok(addr, size) > > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > > I'm going to propose an opt-in method here (RFC for now). We can't have > a check in untagged_addr() since this is already used throughout the > kernel for both user and kernel addresses (khwasan) but we can add one > in __range_ok(). The same prctl() option will be used for controlling > the precise/imprecise mode of MTE later on. We can use a TIF_ flag here > assuming that this will be called early on and any cloned thread will > inherit this. > > Anyway, it's easier to paste some diff than explain but Vincenzo can > fold them into his ABI patches that should really go together with > these. I added a couple of MTE definitions for prctl() as an example, > not used currently: > > ------------------8<--------------------------------------------- > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index fcd0e691b1ea..2d4cb7e4edab 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -307,6 +307,10 @@ extern void __init minsigstksz_setup(void); > /* PR_PAC_RESET_KEYS prctl */ > #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg) > > +/* PR_UNTAGGED_UADDR prctl */ > +int untagged_uaddr_set_mode(unsigned long arg); > +#define SET_UNTAGGED_UADDR_MODE(arg) untagged_uaddr_set_mode(arg) > + > /* > * For CONFIG_GCC_PLUGIN_STACKLEAK > * > diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > index c285d1ce7186..89ce77773c49 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -101,6 +101,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_UNTAGGED_UADDR 26 > > #define _TIF_SIGPENDING (1 << TIF_SIGPENDING) > #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) > @@ -116,6 +117,7 @@ void arch_release_task_struct(struct task_struct *tsk); > #define _TIF_FSCHECK (1 << TIF_FSCHECK) > #define _TIF_32BIT (1 << TIF_32BIT) > #define _TIF_SVE (1 << TIF_SVE) > +#define _TIF_UNTAGGED_UADDR (1 << TIF_UNTAGGED_UADDR) > > #define _TIF_WORK_MASK (_TIF_NEED_RESCHED | _TIF_SIGPENDING | \ > _TIF_NOTIFY_RESUME | _TIF_FOREIGN_FPSTATE | \ > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 9164ecb5feca..54f5bbaebbc4 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > { > unsigned long ret, limit = current_thread_info()->addr_limit; > > + if (test_thread_flag(TIF_UNTAGGED_UADDR)) > + addr = untagged_addr(addr); > + > __chk_user_ptr(addr); > asm volatile( > // A + B <= C + 1 for all A,B,C, in four easy steps: > @@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > return ret; > } > > -#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > +#define access_ok(addr, size) __range_ok(addr, size) > #define user_addr_max get_fs > > #define _ASM_EXTABLE(from, to) \ > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 3767fb21a5b8..fd191c5b92aa 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -552,3 +552,18 @@ void arch_setup_new_exec(void) > > ptrauth_thread_init_user(current); > } > + > +/* > + * Enable the relaxed ABI allowing tagged user addresses into the kernel. > + */ > +int untagged_uaddr_set_mode(unsigned long arg) > +{ > + if (is_compat_task()) > + return -ENOTSUPP; > + if (arg) > + return -EINVAL; > + > + set_thread_flag(TIF_UNTAGGED_UADDR); > + > + return 0; > +} I think this should be paired with a flag clearing in copy_thread(), yes? (i.e. each binary needs to opt in)
On Mon, Jun 10, 2019 at 11:07:03AM -0700, Kees Cook wrote: > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > > index e5d5f31c6d36..9164ecb5feca 100644 > > > --- a/arch/arm64/include/asm/uaccess.h > > > +++ b/arch/arm64/include/asm/uaccess.h > > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > > > return ret; > > > } > > > > > > -#define access_ok(addr, size) __range_ok(addr, size) > > > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) [...] > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > index 3767fb21a5b8..fd191c5b92aa 100644 > > --- a/arch/arm64/kernel/process.c > > +++ b/arch/arm64/kernel/process.c > > @@ -552,3 +552,18 @@ void arch_setup_new_exec(void) > > > > ptrauth_thread_init_user(current); > > } > > + > > +/* > > + * Enable the relaxed ABI allowing tagged user addresses into the kernel. > > + */ > > +int untagged_uaddr_set_mode(unsigned long arg) > > +{ > > + if (is_compat_task()) > > + return -ENOTSUPP; > > + if (arg) > > + return -EINVAL; > > + > > + set_thread_flag(TIF_UNTAGGED_UADDR); > > + > > + return 0; > > +} > > I think this should be paired with a flag clearing in copy_thread(), > yes? (i.e. each binary needs to opt in) It indeed needs clearing though not in copy_thread() as that's used on clone/fork but rather in flush_thread(), called on the execve() path. And a note to myself: I think PR_UNTAGGED_ADDR (not UADDR) looks better in a uapi header, the user doesn't differentiate between uaddr and kaddr.
On Mon, Jun 10, 2019 at 07:53:30PM +0100, Catalin Marinas wrote: > On Mon, Jun 10, 2019 at 11:07:03AM -0700, Kees Cook wrote: > > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > > > index 3767fb21a5b8..fd191c5b92aa 100644 > > > --- a/arch/arm64/kernel/process.c > > > +++ b/arch/arm64/kernel/process.c > > > @@ -552,3 +552,18 @@ void arch_setup_new_exec(void) > > > > > > ptrauth_thread_init_user(current); > > > } > > > + > > > +/* > > > + * Enable the relaxed ABI allowing tagged user addresses into the kernel. > > > + */ > > > +int untagged_uaddr_set_mode(unsigned long arg) > > > +{ > > > + if (is_compat_task()) > > > + return -ENOTSUPP; > > > + if (arg) > > > + return -EINVAL; > > > + > > > + set_thread_flag(TIF_UNTAGGED_UADDR); > > > + > > > + return 0; > > > +} > > > > I think this should be paired with a flag clearing in copy_thread(), > > yes? (i.e. each binary needs to opt in) > > It indeed needs clearing though not in copy_thread() as that's used on > clone/fork but rather in flush_thread(), called on the execve() path. Ah! Yes, thanks. > And a note to myself: I think PR_UNTAGGED_ADDR (not UADDR) looks better > in a uapi header, the user doesn't differentiate between uaddr and > kaddr. Good point. I would agree. :)
On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > index e5d5f31c6d36..9164ecb5feca 100644 > > --- a/arch/arm64/include/asm/uaccess.h > > +++ b/arch/arm64/include/asm/uaccess.h > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > > return ret; > > } > > > > -#define access_ok(addr, size) __range_ok(addr, size) > > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > > I'm going to propose an opt-in method here (RFC for now). We can't have > a check in untagged_addr() since this is already used throughout the > kernel for both user and kernel addresses (khwasan) but we can add one > in __range_ok(). The same prctl() option will be used for controlling > the precise/imprecise mode of MTE later on. We can use a TIF_ flag here > assuming that this will be called early on and any cloned thread will > inherit this. Updated patch, inlining it below. Once we agreed on the approach, I think Andrey can insert in in this series, probably after patch 2. The differences from the one I posted yesterday: - renamed PR_* macros together with get/set variants and the possibility to disable the relaxed ABI - sysctl option - /proc/sys/abi/tagged_addr to disable the ABI globally (just the prctl() opt-in, tasks already using it won't be affected) And, of course, it needs more testing. ---------8<---------------- From 7c624777a4e545522dec1b34e60f0229cb2bd59f Mon Sep 17 00:00:00 2001 From: Catalin Marinas <catalin.marinas@arm.com> Date: Tue, 11 Jun 2019 13:03:38 +0100 Subject: [PATCH] arm64: Introduce prctl() options to control the tagged user addresses ABI 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 | 5 ++- arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++ include/uapi/linux/prctl.h | 5 +++ kernel/sys.c | 16 +++++++ 6 files changed, 99 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 c285d1ce7186..7263d4c973ce 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -101,6 +101,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 #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 9164ecb5feca..995b9ea11a89 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si { unsigned long ret, limit = current_thread_info()->addr_limit; + if (test_thread_flag(TIF_TAGGED_ADDR)) + addr = untagged_addr(addr); + __chk_user_ptr(addr); asm volatile( // A + B <= C + 1 for all A,B,C, in four easy steps: @@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si return ret; } -#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) +#define access_ok(addr, size) __range_ok(addr, size) #define user_addr_max get_fs #define _ASM_EXTABLE(from, to) \ 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;
Hi Catalin, ... > ---------8<---------------- > From 7c624777a4e545522dec1b34e60f0229cb2bd59f Mon Sep 17 00:00:00 2001 > From: Catalin Marinas <catalin.marinas@arm.com> > Date: Tue, 11 Jun 2019 13:03:38 +0100 > Subject: [PATCH] arm64: Introduce prctl() options to control the tagged user > addresses ABI > > 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 | 5 ++- > arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++ > include/uapi/linux/prctl.h | 5 +++ > kernel/sys.c | 16 +++++++ > 6 files changed, 99 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 c285d1ce7186..7263d4c973ce 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -101,6 +101,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 > Can you please put a comment here? > #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 9164ecb5feca..995b9ea11a89 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > { > unsigned long ret, limit = current_thread_info()->addr_limit; > > + if (test_thread_flag(TIF_TAGGED_ADDR)) > + addr = untagged_addr(addr); > + > __chk_user_ptr(addr); > asm volatile( > // A + B <= C + 1 for all A,B,C, in four easy steps: > @@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > return ret; > } > > -#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > +#define access_ok(addr, size) __range_ok(addr, size) > #define user_addr_max get_fs >> #define _ASM_EXTABLE(from, to) \ > 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); Nit: in line we the other functions in thread_flush we could have something like "tagged_addr_thread_flush", maybe inlined. > } > > 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); process.c seems already a bit "overcrowded". Probably we could move all the tagged_addr features in a separate file. What do you think? It would make easier the implementation of mte as well going forward. > 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; Why do we need two prctl here? We could have only one and use arg2 as set/get and arg3 as a parameter. What do you think? > default: > error = -EINVAL; > break; >
On Tue, Jun 11, 2019 at 4:57 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > > index e5d5f31c6d36..9164ecb5feca 100644 > > > --- a/arch/arm64/include/asm/uaccess.h > > > +++ b/arch/arm64/include/asm/uaccess.h > > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > > > return ret; > > > } > > > > > > -#define access_ok(addr, size) __range_ok(addr, size) > > > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > > > > I'm going to propose an opt-in method here (RFC for now). We can't have > > a check in untagged_addr() since this is already used throughout the > > kernel for both user and kernel addresses (khwasan) but we can add one > > in __range_ok(). The same prctl() option will be used for controlling > > the precise/imprecise mode of MTE later on. We can use a TIF_ flag here > > assuming that this will be called early on and any cloned thread will > > inherit this. > > Updated patch, inlining it below. Once we agreed on the approach, I > think Andrey can insert in in this series, probably after patch 2. The > differences from the one I posted yesterday: > > - renamed PR_* macros together with get/set variants and the possibility > to disable the relaxed ABI > > - sysctl option - /proc/sys/abi/tagged_addr to disable the ABI globally > (just the prctl() opt-in, tasks already using it won't be affected) > > And, of course, it needs more testing. Sure, I'll add it to the series. Should I drop access_ok() change from my patch, since yours just reverts it? Thanks! > > ---------8<---------------- > From 7c624777a4e545522dec1b34e60f0229cb2bd59f Mon Sep 17 00:00:00 2001 > From: Catalin Marinas <catalin.marinas@arm.com> > Date: Tue, 11 Jun 2019 13:03:38 +0100 > Subject: [PATCH] arm64: Introduce prctl() options to control the tagged user > addresses ABI > > 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 | 5 ++- > arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++ > include/uapi/linux/prctl.h | 5 +++ > kernel/sys.c | 16 +++++++ > 6 files changed, 99 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 c285d1ce7186..7263d4c973ce 100644 > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -101,6 +101,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 > > #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 9164ecb5feca..995b9ea11a89 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -73,6 +73,9 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > { > unsigned long ret, limit = current_thread_info()->addr_limit; > > + if (test_thread_flag(TIF_TAGGED_ADDR)) > + addr = untagged_addr(addr); > + > __chk_user_ptr(addr); > asm volatile( > // A + B <= C + 1 for all A,B,C, in four easy steps: > @@ -94,7 +97,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > return ret; > } > > -#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > +#define access_ok(addr, size) __range_ok(addr, size) > #define user_addr_max get_fs > > #define _ASM_EXTABLE(from, to) \ > 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;
On Tue, Jun 11, 2019 at 07:09:46PM +0200, Andrey Konovalov wrote: > On Tue, Jun 11, 2019 at 4:57 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > > > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > > > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > > > index e5d5f31c6d36..9164ecb5feca 100644 > > > > --- a/arch/arm64/include/asm/uaccess.h > > > > +++ b/arch/arm64/include/asm/uaccess.h > > > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > > > > return ret; > > > > } > > > > > > > > -#define access_ok(addr, size) __range_ok(addr, size) > > > > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > > > > > > I'm going to propose an opt-in method here (RFC for now). We can't have > > > a check in untagged_addr() since this is already used throughout the > > > kernel for both user and kernel addresses (khwasan) but we can add one > > > in __range_ok(). The same prctl() option will be used for controlling > > > the precise/imprecise mode of MTE later on. We can use a TIF_ flag here > > > assuming that this will be called early on and any cloned thread will > > > inherit this. > > > > Updated patch, inlining it below. Once we agreed on the approach, I > > think Andrey can insert in in this series, probably after patch 2. The > > differences from the one I posted yesterday: > > > > - renamed PR_* macros together with get/set variants and the possibility > > to disable the relaxed ABI > > > > - sysctl option - /proc/sys/abi/tagged_addr to disable the ABI globally > > (just the prctl() opt-in, tasks already using it won't be affected) > > > > And, of course, it needs more testing. > > Sure, I'll add it to the series. > > Should I drop access_ok() change from my patch, since yours just reverts it? Not necessary, your patch just relaxes the ABI for all apps, mine tightens it. You could instead move the untagging to __range_ok() and rebase my patch accordingly.
Hi Vincenzo, On Tue, Jun 11, 2019 at 06:09:10PM +0100, Vincenzo Frascino wrote: > > 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); > > Nit: in line we the other functions in thread_flush we could have something like > "tagged_addr_thread_flush", maybe inlined. The other functions do a lot more than clearing a TIF flag, so they deserved their own place. We could do this when adding MTE support. I think we also need to check what other TIF flags we may inadvertently pass on execve(), maybe have a mask clearing. > > 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; > > Why do we need two prctl here? We could have only one and use arg2 as set/get > and arg3 as a parameter. What do you think? This follows the other PR_* options, e.g. PR_SET_VL/GET_VL, PR_*_FP_MODE. We will use other bits in arg2, for example to set the precise vs imprecise MTE trapping.
On Tue, Jun 11, 2019 at 7:39 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Jun 11, 2019 at 07:09:46PM +0200, Andrey Konovalov wrote: > > On Tue, Jun 11, 2019 at 4:57 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > > > On Mon, Jun 10, 2019 at 06:53:27PM +0100, Catalin Marinas wrote: > > > > On Mon, Jun 03, 2019 at 06:55:04PM +0200, Andrey Konovalov wrote: > > > > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > > > > index e5d5f31c6d36..9164ecb5feca 100644 > > > > > --- a/arch/arm64/include/asm/uaccess.h > > > > > +++ b/arch/arm64/include/asm/uaccess.h > > > > > @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si > > > > > return ret; > > > > > } > > > > > > > > > > -#define access_ok(addr, size) __range_ok(addr, size) > > > > > +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) > > > > > > > > I'm going to propose an opt-in method here (RFC for now). We can't have > > > > a check in untagged_addr() since this is already used throughout the > > > > kernel for both user and kernel addresses (khwasan) but we can add one > > > > in __range_ok(). The same prctl() option will be used for controlling > > > > the precise/imprecise mode of MTE later on. We can use a TIF_ flag here > > > > assuming that this will be called early on and any cloned thread will > > > > inherit this. > > > > > > Updated patch, inlining it below. Once we agreed on the approach, I > > > think Andrey can insert in in this series, probably after patch 2. The > > > differences from the one I posted yesterday: > > > > > > - renamed PR_* macros together with get/set variants and the possibility > > > to disable the relaxed ABI > > > > > > - sysctl option - /proc/sys/abi/tagged_addr to disable the ABI globally > > > (just the prctl() opt-in, tasks already using it won't be affected) > > > > > > And, of course, it needs more testing. > > > > Sure, I'll add it to the series. > > > > Should I drop access_ok() change from my patch, since yours just reverts it? > > Not necessary, your patch just relaxes the ABI for all apps, mine > tightens it. You could instead move the untagging to __range_ok() and > rebase my patch accordingly. OK, will do. I'll also add a comment next to TIF_TAGGED_ADDR as Vincenzo asked. > > -- > Catalin
On Wed, Jun 12, 2019 at 01:03:10PM +0200, Andrey Konovalov wrote: > On Tue, Jun 11, 2019 at 7:39 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Tue, Jun 11, 2019 at 07:09:46PM +0200, Andrey Konovalov wrote: > > > Should I drop access_ok() change from my patch, since yours just reverts it? > > > > Not necessary, your patch just relaxes the ABI for all apps, mine > > tightens it. You could instead move the untagging to __range_ok() and > > rebase my patch accordingly. > > OK, will do. I'll also add a comment next to TIF_TAGGED_ADDR as Vincenzo asked. Thanks.
Hi Catalin, On 12/06/2019 10:32, Catalin Marinas wrote: > Hi Vincenzo, > > On Tue, Jun 11, 2019 at 06:09:10PM +0100, Vincenzo Frascino wrote: >>> 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); >> >> Nit: in line we the other functions in thread_flush we could have something like >> "tagged_addr_thread_flush", maybe inlined. > > The other functions do a lot more than clearing a TIF flag, so they > deserved their own place. We could do this when adding MTE support. I > think we also need to check what other TIF flags we may inadvertently > pass on execve(), maybe have a mask clearing. > Agreed. All the comments I provided are meant to simplify the addition of MTE support. >>> 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; >> >> Why do we need two prctl here? We could have only one and use arg2 as set/get >> and arg3 as a parameter. What do you think? > > This follows the other PR_* options, e.g. PR_SET_VL/GET_VL, > PR_*_FP_MODE. We will use other bits in arg2, for example to set the > precise vs imprecise MTE trapping. > Indeed. I was not questioning the pre-existing interface definition, but trying more to reduce the changes to the ABI to the minimum since: - prctl does not mandate how to use the arg[2-5] - prctl interface is flexible enough for the problem to be solved with only one PR_ command. I agree on reusing the interface for MTE for the purposes you specified.
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index e5d5f31c6d36..9164ecb5feca 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -94,7 +94,7 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si return ret; } -#define access_ok(addr, size) __range_ok(addr, size) +#define access_ok(addr, size) __range_ok(untagged_addr(addr), size) #define user_addr_max get_fs #define _ASM_EXTABLE(from, to) \ @@ -226,7 +226,8 @@ static inline void uaccess_enable_not_uao(void) /* * Sanitise a uaccess pointer such that it becomes NULL if above the - * current addr_limit. + * current addr_limit. In case the pointer is tagged (has the top byte set), + * untag the pointer before checking. */ #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr) static inline void __user *__uaccess_mask_ptr(const void __user *ptr) @@ -234,10 +235,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) void __user *safe_ptr; asm volatile( - " bics xzr, %1, %2\n" + " bics xzr, %3, %2\n" " csel %0, %1, xzr, eq\n" : "=&r" (safe_ptr) - : "r" (ptr), "r" (current_thread_info()->addr_limit) + : "r" (ptr), "r" (current_thread_info()->addr_limit), + "r" (untagged_addr(ptr)) : "cc"); csdb();