* [PATCH v2 0/2] uaccess: Add unsafe accessors for arm64 @ 2018-12-03 13:55 Julien Thierry 2018-12-03 13:55 ` [RFC PATCH v2 1/2] uaccess: Check no rescheduling function is called in unsafe region Julien Thierry 2018-12-03 13:55 ` [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors Julien Thierry 0 siblings, 2 replies; 11+ messages in thread From: Julien Thierry @ 2018-12-03 13:55 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: mingo, peterz, catalin.marinas, will.deacon, james.morse, hpa, Julien Thierry Hi, First version of this series[1] was briefly in linux-next but had to be reverted due to a bug where schedule would end up being called while user_access was active[2]. After clarifications[3], rescheduling while in a user_access region is not allowed. * Patch 1 clarifies this restriction in the API and attempts to check against violations of the restriction. * Patch 2 implements the unsafe accessors for arm64 Changes since v1: - Add a way to detect code calling schedule within a user_access region - Make sure put_user/get_user arguments are evaluated before disabling PAN [1] https://www.spinics.net/lists/arm-kernel/msg674925.html [2] https://patchwork.kernel.org/patch/10634783/ [3] https://lkml.org/lkml/2018/11/28/50 Cheers, Julien --> Julien Thierry (2): uaccess: Check no rescheduling function is called in unsafe region arm64: uaccess: Implement unsafe accessors arch/arm64/include/asm/sysreg.h | 2 + arch/arm64/include/asm/uaccess.h | 89 +++++++++++++++++++++++++++++++--------- include/linux/kernel.h | 6 ++- include/linux/uaccess.h | 11 +++++ kernel/sched/core.c | 19 +++++++++ 5 files changed, 105 insertions(+), 22 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH v2 1/2] uaccess: Check no rescheduling function is called in unsafe region 2018-12-03 13:55 [PATCH v2 0/2] uaccess: Add unsafe accessors for arm64 Julien Thierry @ 2018-12-03 13:55 ` Julien Thierry 2019-01-14 12:03 ` Valentin Schneider 2018-12-03 13:55 ` [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors Julien Thierry 1 sibling, 1 reply; 11+ messages in thread From: Julien Thierry @ 2018-12-03 13:55 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: mingo, peterz, catalin.marinas, will.deacon, james.morse, hpa, Julien Thierry While running a user_access regions, it is not supported to reschedule. Add an overridable primitive to indicate whether a user_access region is active and check that this is not the case when calling rescheduling functions. Also, add a comment clarifying the behaviour of user_access regions. Signed-off-by: Julien Thierry <julien.thierry@arm.com> --- include/linux/kernel.h | 6 ++++-- include/linux/uaccess.h | 11 +++++++++++ kernel/sched/core.c | 19 +++++++++++++++++++ 3 files changed, 34 insertions(+), 2 deletions(-) I'm not sure these are the best locations to check this but I was hoping this patch could start the discussion. Should I move the check? Should I add a config option to conditionally build those checks? diff --git a/include/linux/kernel.h b/include/linux/kernel.h index d6aac75..fe0e984 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -237,11 +237,13 @@ struct pt_regs; struct user; +extern void __might_resched(const char *file, int line); #ifdef CONFIG_PREEMPT_VOLUNTARY extern int _cond_resched(void); -# define might_resched() _cond_resched() +# define might_resched() \ + do { __might_resched(__FILE__, __LINE__); _cond_resched(); } while (0) #else -# define might_resched() do { } while (0) +# define might_resched() __might_resched(__FILE__, __LINE__) #endif #ifdef CONFIG_DEBUG_ATOMIC_SLEEP diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index efe79c1..50adb84 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -266,6 +266,13 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to, #define probe_kernel_address(addr, retval) \ probe_kernel_read(&retval, addr, sizeof(retval)) +/* + * user_access_begin() and user_access_end() define a region where + * unsafe user accessors can be used. + * During execution of this region, no sleeping functions should be called. + * Exceptions and interrupt shall exit the user_access region and re-enter it + * when returning to the interrupted context. + */ #ifndef user_access_begin #define user_access_begin() do { } while (0) #define user_access_end() do { } while (0) @@ -273,6 +280,10 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to, #define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0) #endif +#ifndef unsafe_user_region_active +#define unsafe_user_region_active() false +#endif + #ifdef CONFIG_HARDENED_USERCOPY void usercopy_warn(const char *name, const char *detail, bool to_user, unsigned long offset, unsigned long len); diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 6fedf3a..03f53c8 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -3289,6 +3289,13 @@ static inline void schedule_debug(struct task_struct *prev) __schedule_bug(prev); preempt_count_set(PREEMPT_DISABLED); } + + if (unlikely(unsafe_user_region_active())) { + printk(KERN_ERR "BUG: scheduling while user_access enabled: %s/%d/0x%08x\n", + prev->comm, prev->pid, preempt_count()); + dump_stack(); + } + rcu_sleep_check(); profile_hit(SCHED_PROFILING, __builtin_return_address(0)); @@ -6151,6 +6158,18 @@ void ___might_sleep(const char *file, int line, int preempt_offset) EXPORT_SYMBOL(___might_sleep); #endif +void __might_resched(const char *file, int line) +{ + if (!unsafe_user_region_active()) + return; + + printk(KERN_ERR + "BUG: rescheduling function called from user access context at %s:%d\n", + file, line); + dump_stack(); +} +EXPORT_SYMBOL(__might_resched); + #ifdef CONFIG_MAGIC_SYSRQ void normalize_rt_tasks(void) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v2 1/2] uaccess: Check no rescheduling function is called in unsafe region 2018-12-03 13:55 ` [RFC PATCH v2 1/2] uaccess: Check no rescheduling function is called in unsafe region Julien Thierry @ 2019-01-14 12:03 ` Valentin Schneider 2019-01-15 11:48 ` Julien Thierry 0 siblings, 1 reply; 11+ messages in thread From: Valentin Schneider @ 2019-01-14 12:03 UTC (permalink / raw) To: Julien Thierry, linux-kernel, linux-arm-kernel Cc: mingo, peterz, catalin.marinas, will.deacon, james.morse, hpa Hi, On 03/12/2018 13:55, Julien Thierry wrote: > While running a user_access regions, it is not supported to reschedule. > Add an overridable primitive to indicate whether a user_access region is > active and check that this is not the case when calling rescheduling > functions. > > Also, add a comment clarifying the behaviour of user_access regions. > > Signed-off-by: Julien Thierry <julien.thierry@arm.com> > --- > include/linux/kernel.h | 6 ++++-- > include/linux/uaccess.h | 11 +++++++++++ > kernel/sched/core.c | 19 +++++++++++++++++++ > 3 files changed, 34 insertions(+), 2 deletions(-) > > I'm not sure these are the best locations to check this but I was hoping > this patch could start the discussion. > > Should I move the check? Should I add a config option to conditionally > build those checks? > I was going to say it's already under DEBUG_ATOMIC_SLEEP, but that's only true for the __might_sleep() bit actually. I think it'd make sense to blanket that under a config, but using DEBUG_ATOMIC_SLEEP for that is a bit too much. What about a DEBUG_UACCESS_SLEEP? > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index d6aac75..fe0e984 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -237,11 +237,13 @@ > struct pt_regs; > struct user; > > +extern void __might_resched(const char *file, int line); > #ifdef CONFIG_PREEMPT_VOLUNTARY > extern int _cond_resched(void); > -# define might_resched() _cond_resched() > +# define might_resched() \ > + do { __might_resched(__FILE__, __LINE__); _cond_resched(); } while (0) > #else > -# define might_resched() do { } while (0) > +# define might_resched() __might_resched(__FILE__, __LINE__)> #endif > > #ifdef CONFIG_DEBUG_ATOMIC_SLEEP > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h > index efe79c1..50adb84 100644 > --- a/include/linux/uaccess.h > +++ b/include/linux/uaccess.h > @@ -266,6 +266,13 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to, > #define probe_kernel_address(addr, retval) \ > probe_kernel_read(&retval, addr, sizeof(retval)) > > +/* > + * user_access_begin() and user_access_end() define a region where > + * unsafe user accessors can be used. > + * During execution of this region, no sleeping functions should be called. > + * Exceptions and interrupt shall exit the user_access region and re-enter it > + * when returning to the interrupted context. > + */ I would first have the bit about exceptions, then mention sleeping and add something along the lines of "[...] no sleeping functions should be called - we rely on exception handling to take care of the user_access status for us, but that doesn't happen when directly calling schedule()." My wording's not the best but I just want something to point out *why* sleeping ain't okay. > #ifndef user_access_begin > #define user_access_begin() do { } while (0) > #define user_access_end() do { } while (0) > @@ -273,6 +280,10 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to, > #define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0) > #endif > > +#ifndef unsafe_user_region_active > +#define unsafe_user_region_active() false > +#endif > + > #ifdef CONFIG_HARDENED_USERCOPY > void usercopy_warn(const char *name, const char *detail, bool to_user, > unsigned long offset, unsigned long len); > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 6fedf3a..03f53c8 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -3289,6 +3289,13 @@ static inline void schedule_debug(struct task_struct *prev) > __schedule_bug(prev); > preempt_count_set(PREEMPT_DISABLED); > } > + > + if (unlikely(unsafe_user_region_active())) { > + printk(KERN_ERR "BUG: scheduling while user_access enabled: %s/%d/0x%08x\n", > + prev->comm, prev->pid, preempt_count()); > + dump_stack(); > + } > + > rcu_sleep_check(); > > profile_hit(SCHED_PROFILING, __builtin_return_address(0)); > @@ -6151,6 +6158,18 @@ void ___might_sleep(const char *file, int line, int preempt_offset) > EXPORT_SYMBOL(___might_sleep); > #endif > > +void __might_resched(const char *file, int line) > +{ > + if (!unsafe_user_region_active()) > + return; > + > + printk(KERN_ERR > + "BUG: rescheduling function called from user access context at %s:%d\n", > + file, line); > + dump_stack(); > +} So this check is "careful, things might go bad" and the schedule_debug() one is "things went bad". IIUC we'll always get this warning when we hit the schedule_debug() one. I was going to suggest only keeping one of them, but I think both hold value. > +EXPORT_SYMBOL(__might_resched); > + > #ifdef CONFIG_MAGIC_SYSRQ > void normalize_rt_tasks(void) > { > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH v2 1/2] uaccess: Check no rescheduling function is called in unsafe region 2019-01-14 12:03 ` Valentin Schneider @ 2019-01-15 11:48 ` Julien Thierry 0 siblings, 0 replies; 11+ messages in thread From: Julien Thierry @ 2019-01-15 11:48 UTC (permalink / raw) To: Valentin Schneider, linux-kernel, linux-arm-kernel Cc: mingo, peterz, catalin.marinas, will.deacon, james.morse, hpa Hi, On 14/01/2019 12:03, Valentin Schneider wrote: > Hi, > > On 03/12/2018 13:55, Julien Thierry wrote: >> While running a user_access regions, it is not supported to reschedule. >> Add an overridable primitive to indicate whether a user_access region is >> active and check that this is not the case when calling rescheduling >> functions. >> >> Also, add a comment clarifying the behaviour of user_access regions. >> >> Signed-off-by: Julien Thierry <julien.thierry@arm.com> >> --- >> include/linux/kernel.h | 6 ++++-- >> include/linux/uaccess.h | 11 +++++++++++ >> kernel/sched/core.c | 19 +++++++++++++++++++ >> 3 files changed, 34 insertions(+), 2 deletions(-) >> >> I'm not sure these are the best locations to check this but I was hoping >> this patch could start the discussion. >> >> Should I move the check? Should I add a config option to conditionally >> build those checks? >> > > I was going to say it's already under DEBUG_ATOMIC_SLEEP, but that's only > true for the __might_sleep() bit actually. > > I think it'd make sense to blanket that under a config, but using > DEBUG_ATOMIC_SLEEP for that is a bit too much. What about a > DEBUG_UACCESS_SLEEP? > Yes, I was wondering whether to add something like that, I'll add a DEBUG_UACCESS_SLEEP to my next version. >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h >> index d6aac75..fe0e984 100644 >> --- a/include/linux/kernel.h >> +++ b/include/linux/kernel.h >> @@ -237,11 +237,13 @@ >> struct pt_regs; >> struct user; >> >> +extern void __might_resched(const char *file, int line); >> #ifdef CONFIG_PREEMPT_VOLUNTARY >> extern int _cond_resched(void); >> -# define might_resched() _cond_resched() >> +# define might_resched() \ >> + do { __might_resched(__FILE__, __LINE__); _cond_resched(); } while (0) >> #else >> -# define might_resched() do { } while (0) >> +# define might_resched() __might_resched(__FILE__, __LINE__)> #endif >> >> #ifdef CONFIG_DEBUG_ATOMIC_SLEEP >> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h >> index efe79c1..50adb84 100644 >> --- a/include/linux/uaccess.h >> +++ b/include/linux/uaccess.h >> @@ -266,6 +266,13 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to, >> #define probe_kernel_address(addr, retval) \ >> probe_kernel_read(&retval, addr, sizeof(retval)) >> >> +/* >> + * user_access_begin() and user_access_end() define a region where >> + * unsafe user accessors can be used. >> + * During execution of this region, no sleeping functions should be called. >> + * Exceptions and interrupt shall exit the user_access region and re-enter it >> + * when returning to the interrupted context. >> + */ > > I would first have the bit about exceptions, then mention sleeping and add > something along the lines of > > "[...] no sleeping functions should be called - we rely on exception > handling to take care of the user_access status for us, but that doesn't > happen when directly calling schedule()." > > My wording's not the best but I just want something to point out *why* > sleeping ain't okay. > I think the wording is alright, I'll include your suggestion for the next version. >> #ifndef user_access_begin >> #define user_access_begin() do { } while (0) >> #define user_access_end() do { } while (0) >> @@ -273,6 +280,10 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to, >> #define unsafe_put_user(x, ptr, err) do { if (unlikely(__put_user(x, ptr))) goto err; } while (0) >> #endif >> >> +#ifndef unsafe_user_region_active >> +#define unsafe_user_region_active() false >> +#endif >> + >> #ifdef CONFIG_HARDENED_USERCOPY >> void usercopy_warn(const char *name, const char *detail, bool to_user, >> unsigned long offset, unsigned long len); >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index 6fedf3a..03f53c8 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -3289,6 +3289,13 @@ static inline void schedule_debug(struct task_struct *prev) >> __schedule_bug(prev); >> preempt_count_set(PREEMPT_DISABLED); >> } >> + >> + if (unlikely(unsafe_user_region_active())) { >> + printk(KERN_ERR "BUG: scheduling while user_access enabled: %s/%d/0x%08x\n", >> + prev->comm, prev->pid, preempt_count()); >> + dump_stack(); >> + } >> + >> rcu_sleep_check(); >> >> profile_hit(SCHED_PROFILING, __builtin_return_address(0)); >> @@ -6151,6 +6158,18 @@ void ___might_sleep(const char *file, int line, int preempt_offset) >> EXPORT_SYMBOL(___might_sleep); >> #endif >> >> +void __might_resched(const char *file, int line) >> +{ >> + if (!unsafe_user_region_active()) >> + return; >> + >> + printk(KERN_ERR >> + "BUG: rescheduling function called from user access context at %s:%d\n", >> + file, line); >> + dump_stack(); >> +} > > So this check is "careful, things might go bad" and the schedule_debug() > one is "things went bad". IIUC we'll always get this warning when we hit > the schedule_debug() one. I was going to suggest only keeping one of them, > but I think both hold value. > Yes, I can't really convince myself to remove either, unless there is a magic place that covers both cases. Thanks for the suggestions. Cheers, -- Julien Thierry ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors 2018-12-03 13:55 [PATCH v2 0/2] uaccess: Add unsafe accessors for arm64 Julien Thierry 2018-12-03 13:55 ` [RFC PATCH v2 1/2] uaccess: Check no rescheduling function is called in unsafe region Julien Thierry @ 2018-12-03 13:55 ` Julien Thierry 2018-12-06 18:25 ` Catalin Marinas 2018-12-21 14:57 ` James Morse 1 sibling, 2 replies; 11+ messages in thread From: Julien Thierry @ 2018-12-03 13:55 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: mingo, peterz, catalin.marinas, will.deacon, james.morse, hpa, Julien Thierry Current implementation of get/put_user_unsafe default to get/put_user which toggle PAN before each access, despite having been told by the caller that multiple accesses to user memory were about to happen. Provide implementations for user_access_begin/end to turn PAN off/on and implement unsafe accessors that assume PAN was already turned off. Signed-off-by: Julien Thierry <julien.thierry@arm.com> --- arch/arm64/include/asm/sysreg.h | 2 + arch/arm64/include/asm/uaccess.h | 89 +++++++++++++++++++++++++++++++--------- 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 842fb95..4e6477b 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -108,6 +108,8 @@ #define SYS_DC_CSW sys_insn(1, 0, 7, 10, 2) #define SYS_DC_CISW sys_insn(1, 0, 7, 14, 2) +#define SYS_PSTATE_PAN sys_reg(3, 0, 4, 2, 3) + #define SYS_OSDTRRX_EL1 sys_reg(2, 0, 0, 0, 2) #define SYS_MDCCINT_EL1 sys_reg(2, 0, 0, 2, 0) #define SYS_MDSCR_EL1 sys_reg(2, 0, 0, 2, 2) diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 07c3408..cabfcae 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -233,6 +233,23 @@ static inline void uaccess_enable_not_uao(void) __uaccess_enable(ARM64_ALT_PAN_NOT_UAO); } +#define unsafe_user_region_active uaccess_region_active +static inline bool uaccess_region_active(void) +{ + if (system_uses_ttbr0_pan()) { + u64 ttbr; + + ttbr = read_sysreg(ttbr1_el1); + return ttbr & TTBR_ASID_MASK; + } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO)) { + return (read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN) ? + false : + !read_sysreg_s(SYS_PSTATE_PAN); + } + + return false; +} + /* * Sanitise a uaccess pointer such that it becomes NULL if above the * current addr_limit. @@ -276,11 +293,9 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) : "+r" (err), "=&r" (x) \ : "r" (addr), "i" (-EFAULT)) -#define __get_user_err(x, ptr, err) \ +#define __get_user_err_unsafe(x, ptr, err) \ do { \ unsigned long __gu_val; \ - __chk_user_ptr(ptr); \ - uaccess_enable_not_uao(); \ switch (sizeof(*(ptr))) { \ case 1: \ __get_user_asm("ldrb", "ldtrb", "%w", __gu_val, (ptr), \ @@ -301,17 +316,26 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) default: \ BUILD_BUG(); \ } \ - uaccess_disable_not_uao(); \ (x) = (__force __typeof__(*(ptr)))__gu_val; \ } while (0) -#define __get_user_check(x, ptr, err) \ +#define __get_user_err_check(x, ptr, err) \ +do { \ + __typeof__(x) __gu_dest; \ + __chk_user_ptr(ptr); \ + uaccess_enable_not_uao(); \ + __get_user_err_unsafe((__gu_dest), (ptr), (err)); \ + uaccess_disable_not_uao(); \ + (x) = __gu_dest; \ +} while (0) + +#define __get_user_err(x, ptr, err, accessor) \ ({ \ __typeof__(*(ptr)) __user *__p = (ptr); \ might_fault(); \ if (access_ok(VERIFY_READ, __p, sizeof(*__p))) { \ __p = uaccess_mask_ptr(__p); \ - __get_user_err((x), __p, (err)); \ + accessor((x), __p, (err)); \ } else { \ (x) = 0; (err) = -EFAULT; \ } \ @@ -319,14 +343,14 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) #define __get_user_error(x, ptr, err) \ ({ \ - __get_user_check((x), (ptr), (err)); \ + __get_user_err((x), (ptr), (err), __get_user_err_check); \ (void)0; \ }) #define __get_user(x, ptr) \ ({ \ int __gu_err = 0; \ - __get_user_check((x), (ptr), __gu_err); \ + __get_user_err((x), (ptr), __gu_err, __get_user_err_check); \ __gu_err; \ }) @@ -346,41 +370,46 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) : "+r" (err) \ : "r" (x), "r" (addr), "i" (-EFAULT)) -#define __put_user_err(x, ptr, err) \ +#define __put_user_err_unsafe(x, ptr, err) \ do { \ - __typeof__(*(ptr)) __pu_val = (x); \ - __chk_user_ptr(ptr); \ - uaccess_enable_not_uao(); \ switch (sizeof(*(ptr))) { \ case 1: \ - __put_user_asm("strb", "sttrb", "%w", __pu_val, (ptr), \ + __put_user_asm("strb", "sttrb", "%w", (x), (ptr), \ (err), ARM64_HAS_UAO); \ break; \ case 2: \ - __put_user_asm("strh", "sttrh", "%w", __pu_val, (ptr), \ + __put_user_asm("strh", "sttrh", "%w", (x), (ptr), \ (err), ARM64_HAS_UAO); \ break; \ case 4: \ - __put_user_asm("str", "sttr", "%w", __pu_val, (ptr), \ + __put_user_asm("str", "sttr", "%w", (x), (ptr), \ (err), ARM64_HAS_UAO); \ break; \ case 8: \ - __put_user_asm("str", "sttr", "%x", __pu_val, (ptr), \ + __put_user_asm("str", "sttr", "%x", (x), (ptr), \ (err), ARM64_HAS_UAO); \ break; \ default: \ BUILD_BUG(); \ } \ +} while (0) + +#define __put_user_err_check(x, ptr, err) \ +do { \ + __typeof__(*(ptr)) __pu_val = (x); \ + __chk_user_ptr(ptr); \ + uaccess_enable_not_uao(); \ + __put_user_err_unsafe(__pu_val, (ptr), (err)); \ uaccess_disable_not_uao(); \ } while (0) -#define __put_user_check(x, ptr, err) \ +#define __put_user_err(x, ptr, err, accessor) \ ({ \ __typeof__(*(ptr)) __user *__p = (ptr); \ might_fault(); \ if (access_ok(VERIFY_WRITE, __p, sizeof(*__p))) { \ __p = uaccess_mask_ptr(__p); \ - __put_user_err((x), __p, (err)); \ + accessor((x), __p, (err)); \ } else { \ (err) = -EFAULT; \ } \ @@ -388,19 +417,39 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr) #define __put_user_error(x, ptr, err) \ ({ \ - __put_user_check((x), (ptr), (err)); \ + __put_user_err((x), (ptr), (err), __put_user_err_check); \ (void)0; \ }) #define __put_user(x, ptr) \ ({ \ int __pu_err = 0; \ - __put_user_check((x), (ptr), __pu_err); \ + __put_user_err((x), (ptr), __pu_err, __put_user_err_check); \ __pu_err; \ }) #define put_user __put_user + +#define user_access_begin() uaccess_enable_not_uao() +#define user_access_end() uaccess_disable_not_uao() + +#define unsafe_get_user(x, ptr, err) \ +do { \ + int __gu_err = 0; \ + __get_user_err((x), (ptr), __gu_err, __get_user_err_unsafe); \ + if (__gu_err != 0) \ + goto err; \ +} while (0) + +#define unsafe_put_user(x, ptr, err) \ +do { \ + int __pu_err = 0; \ + __put_user_err((x), (ptr), __pu_err, __put_user_err_unsafe); \ + if (__pu_err != 0) \ + goto err; \ +} while (0) + extern unsigned long __must_check __arch_copy_from_user(void *to, const void __user *from, unsigned long n); #define raw_copy_from_user(to, from, n) \ ({ \ -- 1.9.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors 2018-12-03 13:55 ` [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors Julien Thierry @ 2018-12-06 18:25 ` Catalin Marinas 2018-12-07 8:38 ` Julien Thierry 2018-12-21 14:57 ` James Morse 1 sibling, 1 reply; 11+ messages in thread From: Catalin Marinas @ 2018-12-06 18:25 UTC (permalink / raw) To: Julien Thierry Cc: linux-kernel, linux-arm-kernel, peterz, will.deacon, mingo, james.morse, hpa On Mon, Dec 03, 2018 at 01:55:18PM +0000, Julien Thierry wrote: > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 07c3408..cabfcae 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -233,6 +233,23 @@ static inline void uaccess_enable_not_uao(void) > __uaccess_enable(ARM64_ALT_PAN_NOT_UAO); > } > > +#define unsafe_user_region_active uaccess_region_active > +static inline bool uaccess_region_active(void) > +{ > + if (system_uses_ttbr0_pan()) { > + u64 ttbr; > + > + ttbr = read_sysreg(ttbr1_el1); > + return ttbr & TTBR_ASID_MASK; Nitpick: could write this in 1-2 lines. > + } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO)) { > + return (read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN) ? > + false : > + !read_sysreg_s(SYS_PSTATE_PAN); > + } ARM64_ALT_PAN_NOT_UAO implies ARM64_HAS_PAN which implies SCTLR_EL1.SPAN is 0 at run-time. Is this to cope with the case of being called prior to cpu_enable_pan()? -- Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors 2018-12-06 18:25 ` Catalin Marinas @ 2018-12-07 8:38 ` Julien Thierry 2018-12-10 14:59 ` Catalin Marinas 0 siblings, 1 reply; 11+ messages in thread From: Julien Thierry @ 2018-12-07 8:38 UTC (permalink / raw) To: Catalin Marinas Cc: linux-kernel, linux-arm-kernel, peterz, will.deacon, mingo, james.morse, hpa On 12/06/2018 06:25 PM, Catalin Marinas wrote: > On Mon, Dec 03, 2018 at 01:55:18PM +0000, Julien Thierry wrote: >> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h >> index 07c3408..cabfcae 100644 >> --- a/arch/arm64/include/asm/uaccess.h >> +++ b/arch/arm64/include/asm/uaccess.h >> @@ -233,6 +233,23 @@ static inline void uaccess_enable_not_uao(void) >> __uaccess_enable(ARM64_ALT_PAN_NOT_UAO); >> } >> >> +#define unsafe_user_region_active uaccess_region_active >> +static inline bool uaccess_region_active(void) >> +{ >> + if (system_uses_ttbr0_pan()) { >> + u64 ttbr; >> + >> + ttbr = read_sysreg(ttbr1_el1); >> + return ttbr & TTBR_ASID_MASK; > > Nitpick: could write this in 1-2 lines. > True, I can do that in 1 line. >> + } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO)) { >> + return (read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN) ? >> + false : >> + !read_sysreg_s(SYS_PSTATE_PAN); >> + } > > ARM64_ALT_PAN_NOT_UAO implies ARM64_HAS_PAN which implies SCTLR_EL1.SPAN > is 0 at run-time. Is this to cope with the case of being called prior to > cpu_enable_pan()? > Yes, the issue I can into is that for cpufeatures, .cpu_enable() callbacks are called inside stop_machine() which obviously might_sleep and so attempts to check whether user_access is on. But for features that get enabled before PAN, the PAN bit will be set. Thanks, Julien ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors 2018-12-07 8:38 ` Julien Thierry @ 2018-12-10 14:59 ` Catalin Marinas 2018-12-12 17:40 ` Suzuki K Poulose 2018-12-21 14:57 ` James Morse 0 siblings, 2 replies; 11+ messages in thread From: Catalin Marinas @ 2018-12-10 14:59 UTC (permalink / raw) To: Julien Thierry Cc: peterz, will.deacon, linux-kernel, mingo, james.morse, hpa, linux-arm-kernel, Suzuki K Poulose On Fri, Dec 07, 2018 at 08:38:11AM +0000, Julien Thierry wrote: > > > On 12/06/2018 06:25 PM, Catalin Marinas wrote: > > On Mon, Dec 03, 2018 at 01:55:18PM +0000, Julien Thierry wrote: > > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > > index 07c3408..cabfcae 100644 > > > --- a/arch/arm64/include/asm/uaccess.h > > > +++ b/arch/arm64/include/asm/uaccess.h > > > @@ -233,6 +233,23 @@ static inline void uaccess_enable_not_uao(void) > > > __uaccess_enable(ARM64_ALT_PAN_NOT_UAO); > > > } > > > +#define unsafe_user_region_active uaccess_region_active > > > +static inline bool uaccess_region_active(void) > > > +{ > > > + if (system_uses_ttbr0_pan()) { > > > + u64 ttbr; > > > + > > > + ttbr = read_sysreg(ttbr1_el1); > > > + return ttbr & TTBR_ASID_MASK; > > > > Nitpick: could write this in 1-2 lines. > > True, I can do that in 1 line. > > > > + } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO)) { > > > + return (read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN) ? > > > + false : > > > + !read_sysreg_s(SYS_PSTATE_PAN); > > > + } > > > > ARM64_ALT_PAN_NOT_UAO implies ARM64_HAS_PAN which implies SCTLR_EL1.SPAN > > is 0 at run-time. Is this to cope with the case of being called prior to > > cpu_enable_pan()? > > > > Yes, the issue I can into is that for cpufeatures, .cpu_enable() callbacks > are called inside stop_machine() which obviously might_sleep and so attempts > to check whether user_access is on. But for features that get enabled before > PAN, the PAN bit will be set. OK, so the PSTATE.PAN bit only makes sense when SCTLR_EL1.SPAN is 0, IOW the PAN hardware feature has been enabled. Maybe you could write it (together with some comment): } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO) && !(read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN)) { /* only if PAN is present and enabled */ return !read_sysreg_s(SYS_PSTATE_PAN) } On the cpufeature.c side of things, it seems that we enable the static_branch before calling the cpu_enable. I wonder whether changing the order here would help with avoid the SCTLR_EL1 read (not sure what else it would break; cc'ing Suzuki). -- Catalin ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors 2018-12-10 14:59 ` Catalin Marinas @ 2018-12-12 17:40 ` Suzuki K Poulose 2018-12-21 14:57 ` James Morse 1 sibling, 0 replies; 11+ messages in thread From: Suzuki K Poulose @ 2018-12-12 17:40 UTC (permalink / raw) To: Catalin Marinas, Julien Thierry Cc: peterz, will.deacon, linux-kernel, mingo, james.morse, hpa, linux-arm-kernel On 12/10/2018 02:59 PM, Catalin Marinas wrote: > On Fri, Dec 07, 2018 at 08:38:11AM +0000, Julien Thierry wrote: >> >> >> On 12/06/2018 06:25 PM, Catalin Marinas wrote: >>> On Mon, Dec 03, 2018 at 01:55:18PM +0000, Julien Thierry wrote: >>>> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h >>>> index 07c3408..cabfcae 100644 >>>> --- a/arch/arm64/include/asm/uaccess.h >>>> +++ b/arch/arm64/include/asm/uaccess.h >>>> @@ -233,6 +233,23 @@ static inline void uaccess_enable_not_uao(void) >>>> __uaccess_enable(ARM64_ALT_PAN_NOT_UAO); >>>> } >>>> +#define unsafe_user_region_active uaccess_region_active >>>> +static inline bool uaccess_region_active(void) >>>> +{ >>>> + if (system_uses_ttbr0_pan()) { >>>> + u64 ttbr; >>>> + >>>> + ttbr = read_sysreg(ttbr1_el1); >>>> + return ttbr & TTBR_ASID_MASK; >>> >>> Nitpick: could write this in 1-2 lines. >> >> True, I can do that in 1 line. >> >>>> + } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO)) { >>>> + return (read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN) ? >>>> + false : >>>> + !read_sysreg_s(SYS_PSTATE_PAN); >>>> + } >>> >>> ARM64_ALT_PAN_NOT_UAO implies ARM64_HAS_PAN which implies SCTLR_EL1.SPAN >>> is 0 at run-time. Is this to cope with the case of being called prior to >>> cpu_enable_pan()? >>> >> >> Yes, the issue I can into is that for cpufeatures, .cpu_enable() callbacks >> are called inside stop_machine() which obviously might_sleep and so attempts >> to check whether user_access is on. But for features that get enabled before >> PAN, the PAN bit will be set. > > OK, so the PSTATE.PAN bit only makes sense when SCTLR_EL1.SPAN is 0, IOW > the PAN hardware feature has been enabled. Maybe you could write it > (together with some comment): > > } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO) && > !(read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN)) { > /* only if PAN is present and enabled */ > return !read_sysreg_s(SYS_PSTATE_PAN) > } > > On the cpufeature.c side of things, it seems that we enable the > static_branch before calling the cpu_enable. I wonder whether changing > the order here would help with avoid the SCTLR_EL1 read (not sure what > else it would break; cc'ing Suzuki). > I doubt if we would gain anything by moving it around. cpus_have_const_cap() would fall back to test_bit() until we mark that the static_branches have been updated explicitly, which happens after we have issued the stop_machine(). So, even if we move the static branch per capability, we don't gain much. Is that what you were looking for ? Cheers Suzuki ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors 2018-12-10 14:59 ` Catalin Marinas 2018-12-12 17:40 ` Suzuki K Poulose @ 2018-12-21 14:57 ` James Morse 1 sibling, 0 replies; 11+ messages in thread From: James Morse @ 2018-12-21 14:57 UTC (permalink / raw) To: Catalin Marinas, Julien Thierry Cc: peterz, will.deacon, linux-kernel, mingo, hpa, linux-arm-kernel, Suzuki K Poulose Hi guys, On 10/12/2018 14:59, Catalin Marinas wrote: > On Fri, Dec 07, 2018 at 08:38:11AM +0000, Julien Thierry wrote: >> On 12/06/2018 06:25 PM, Catalin Marinas wrote: >>> On Mon, Dec 03, 2018 at 01:55:18PM +0000, Julien Thierry wrote: >>>> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h >>>> index 07c3408..cabfcae 100644 >>>> --- a/arch/arm64/include/asm/uaccess.h >>>> +++ b/arch/arm64/include/asm/uaccess.h >>>> @@ -233,6 +233,23 @@ static inline void uaccess_enable_not_uao(void) >>>> +#define unsafe_user_region_active uaccess_region_active >>>> +static inline bool uaccess_region_active(void) >>>> +{ >>>> + if (system_uses_ttbr0_pan()) { >>>> + } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO)) { >>>> + return (read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN) ? >>>> + false : >>>> + !read_sysreg_s(SYS_PSTATE_PAN); >>>> + } >>> >>> ARM64_ALT_PAN_NOT_UAO implies ARM64_HAS_PAN which implies SCTLR_EL1.SPAN >>> is 0 at run-time. Is this to cope with the case of being called prior to >>> cpu_enable_pan()? >>> >> >> Yes, the issue I can into is that for cpufeatures, .cpu_enable() callbacks >> are called inside stop_machine() which obviously might_sleep and so attempts >> to check whether user_access is on. But for features that get enabled before >> PAN, the PAN bit will be set. > > OK, so the PSTATE.PAN bit only makes sense when SCTLR_EL1.SPAN is 0, IOW > the PAN hardware feature has been enabled. Maybe you could write it > (together with some comment): > > } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO) && > !(read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN)) { > /* only if PAN is present and enabled */ > return !read_sysreg_s(SYS_PSTATE_PAN) > } > > On the cpufeature.c side of things, it seems that we enable the > static_branch before calling the cpu_enable. I wonder whether changing > the order here would help with avoid the SCTLR_EL1 read (not sure what > else it would break; cc'ing Suzuki). Avoiding the system-register read would be good. Can we check alternatives_applied? It gets set later, and is obviously connected to the PAN alternatives being patched in to the uaccess routines. Thanks, James ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors 2018-12-03 13:55 ` [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors Julien Thierry 2018-12-06 18:25 ` Catalin Marinas @ 2018-12-21 14:57 ` James Morse 1 sibling, 0 replies; 11+ messages in thread From: James Morse @ 2018-12-21 14:57 UTC (permalink / raw) To: Julien Thierry Cc: linux-kernel, linux-arm-kernel, mingo, peterz, catalin.marinas, will.deacon, hpa Hi Julien, On 03/12/2018 13:55, Julien Thierry wrote: > Current implementation of get/put_user_unsafe default to get/put_user > which toggle PAN before each access, despite having been told by the caller > that multiple accesses to user memory were about to happen. > > Provide implementations for user_access_begin/end to turn PAN off/on and > implement unsafe accessors that assume PAN was already turned off. > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 842fb95..4e6477b 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -108,6 +108,8 @@ > #define SYS_DC_CSW sys_insn(1, 0, 7, 10, 2) > #define SYS_DC_CISW sys_insn(1, 0, 7, 14, 2) > > +#define SYS_PSTATE_PAN sys_reg(3, 0, 4, 2, 3) Nit: Could we keep this list in encoding order please. (it makes conflicts easier to resolve in the future) > #define SYS_OSDTRRX_EL1 sys_reg(2, 0, 0, 0, 2) > #define SYS_MDCCINT_EL1 sys_reg(2, 0, 0, 2, 0) > #define SYS_MDSCR_EL1 sys_reg(2, 0, 0, 2, 2) > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 07c3408..cabfcae 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -233,6 +233,23 @@ static inline void uaccess_enable_not_uao(void) > __uaccess_enable(ARM64_ALT_PAN_NOT_UAO); > } > > +#define unsafe_user_region_active uaccess_region_active > +static inline bool uaccess_region_active(void) > +{ > + if (system_uses_ttbr0_pan()) { > + u64 ttbr; > + > + ttbr = read_sysreg(ttbr1_el1); > + return ttbr & TTBR_ASID_MASK; > + } else if (cpus_have_const_cap(ARM64_ALT_PAN_NOT_UAO)) { > + return (read_sysreg(sctlr_el1) & SCTLR_EL1_SPAN) ? > + false : > + !read_sysreg_s(SYS_PSTATE_PAN); > + } > + > + return false; > +} (Reading the SCTLR bit is a bit of a heavy-hammer, as suggested elsewhere on the thread, can we use alternatives_applied here?) It may be worth splitting this into three patches, so the 'unsafe' bits can be merged without the debug option. Either way, for the unsafe parts: Reviewed-by: James Morse <james.morse@arm.com> Thanks! James ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-01-15 11:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-03 13:55 [PATCH v2 0/2] uaccess: Add unsafe accessors for arm64 Julien Thierry 2018-12-03 13:55 ` [RFC PATCH v2 1/2] uaccess: Check no rescheduling function is called in unsafe region Julien Thierry 2019-01-14 12:03 ` Valentin Schneider 2019-01-15 11:48 ` Julien Thierry 2018-12-03 13:55 ` [PATCH v2 2/2] arm64: uaccess: Implement unsafe accessors Julien Thierry 2018-12-06 18:25 ` Catalin Marinas 2018-12-07 8:38 ` Julien Thierry 2018-12-10 14:59 ` Catalin Marinas 2018-12-12 17:40 ` Suzuki K Poulose 2018-12-21 14:57 ` James Morse 2018-12-21 14:57 ` James Morse
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).