* [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN
@ 2021-11-18 16:34 ` Vincent Whitchurch
0 siblings, 0 replies; 12+ messages in thread
From: Vincent Whitchurch @ 2021-11-18 16:34 UTC (permalink / raw)
To: catalin.marinas, will, mark.rutland
Cc: kernel, linux-arm-kernel, linux-kernel, Vincent Whitchurch
The value argument to put_user() must be evaluated before the TTBR0
switch is done. Otherwise, if it is a function and the function sleeps,
the reserved TTBR0 will be restored when the process is switched in
again and the process will end up in an infinite loop of faults.
This problem was seen with the put_user() in schedule_tail(). A similar
fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv:
evaluate put_user() arg before enabling user access").
Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user")
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
arch/arm64/include/asm/uaccess.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 6e2e0b7031ab..96b26fa9d3d0 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -362,10 +362,11 @@ do { \
#define __put_user_error(x, ptr, err) \
do { \
__typeof__(*(ptr)) __user *__p = (ptr); \
+ __typeof__(*(__p)) __val = (x); \
might_fault(); \
if (access_ok(__p, sizeof(*__p))) { \
__p = uaccess_mask_ptr(__p); \
- __raw_put_user((x), __p, (err)); \
+ __raw_put_user(__val, __p, (err)); \
} else { \
(err) = -EFAULT; \
} \
--
2.33.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN @ 2021-11-18 16:34 ` Vincent Whitchurch 0 siblings, 0 replies; 12+ messages in thread From: Vincent Whitchurch @ 2021-11-18 16:34 UTC (permalink / raw) To: catalin.marinas, will, mark.rutland Cc: kernel, linux-arm-kernel, linux-kernel, Vincent Whitchurch The value argument to put_user() must be evaluated before the TTBR0 switch is done. Otherwise, if it is a function and the function sleeps, the reserved TTBR0 will be restored when the process is switched in again and the process will end up in an infinite loop of faults. This problem was seen with the put_user() in schedule_tail(). A similar fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv: evaluate put_user() arg before enabling user access"). Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user") Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- arch/arm64/include/asm/uaccess.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 6e2e0b7031ab..96b26fa9d3d0 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -362,10 +362,11 @@ do { \ #define __put_user_error(x, ptr, err) \ do { \ __typeof__(*(ptr)) __user *__p = (ptr); \ + __typeof__(*(__p)) __val = (x); \ might_fault(); \ if (access_ok(__p, sizeof(*__p))) { \ __p = uaccess_mask_ptr(__p); \ - __raw_put_user((x), __p, (err)); \ + __raw_put_user(__val, __p, (err)); \ } else { \ (err) = -EFAULT; \ } \ -- 2.33.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN 2021-11-18 16:34 ` Vincent Whitchurch @ 2021-11-19 11:33 ` Will Deacon -1 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2021-11-19 11:33 UTC (permalink / raw) To: Vincent Whitchurch Cc: catalin.marinas, mark.rutland, kernel, linux-arm-kernel, linux-kernel On Thu, Nov 18, 2021 at 05:34:17PM +0100, Vincent Whitchurch wrote: > The value argument to put_user() must be evaluated before the TTBR0 > switch is done. Otherwise, if it is a function and the function sleeps, > the reserved TTBR0 will be restored when the process is switched in > again and the process will end up in an infinite loop of faults. > > This problem was seen with the put_user() in schedule_tail(). A similar > fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv: > evaluate put_user() arg before enabling user access"). > > Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user") > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > --- > arch/arm64/include/asm/uaccess.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 6e2e0b7031ab..96b26fa9d3d0 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -362,10 +362,11 @@ do { \ > #define __put_user_error(x, ptr, err) \ > do { \ > __typeof__(*(ptr)) __user *__p = (ptr); \ > + __typeof__(*(__p)) __val = (x); \ > might_fault(); \ > if (access_ok(__p, sizeof(*__p))) { \ > __p = uaccess_mask_ptr(__p); \ > - __raw_put_user((x), __p, (err)); \ > + __raw_put_user(__val, __p, (err)); \ > } else { \ > (err) = -EFAULT; \ > } \ Oh, nice spot! I hope you didn't lose too much time debugging if you actually ran into this... Although it seems a lot less likely to cause a problem, should we do something similar for __get_user_error() and assign to (x) outside of the uaccess-disabled section? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN @ 2021-11-19 11:33 ` Will Deacon 0 siblings, 0 replies; 12+ messages in thread From: Will Deacon @ 2021-11-19 11:33 UTC (permalink / raw) To: Vincent Whitchurch Cc: catalin.marinas, mark.rutland, kernel, linux-arm-kernel, linux-kernel On Thu, Nov 18, 2021 at 05:34:17PM +0100, Vincent Whitchurch wrote: > The value argument to put_user() must be evaluated before the TTBR0 > switch is done. Otherwise, if it is a function and the function sleeps, > the reserved TTBR0 will be restored when the process is switched in > again and the process will end up in an infinite loop of faults. > > This problem was seen with the put_user() in schedule_tail(). A similar > fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv: > evaluate put_user() arg before enabling user access"). > > Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user") > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > --- > arch/arm64/include/asm/uaccess.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 6e2e0b7031ab..96b26fa9d3d0 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -362,10 +362,11 @@ do { \ > #define __put_user_error(x, ptr, err) \ > do { \ > __typeof__(*(ptr)) __user *__p = (ptr); \ > + __typeof__(*(__p)) __val = (x); \ > might_fault(); \ > if (access_ok(__p, sizeof(*__p))) { \ > __p = uaccess_mask_ptr(__p); \ > - __raw_put_user((x), __p, (err)); \ > + __raw_put_user(__val, __p, (err)); \ > } else { \ > (err) = -EFAULT; \ > } \ Oh, nice spot! I hope you didn't lose too much time debugging if you actually ran into this... Although it seems a lot less likely to cause a problem, should we do something similar for __get_user_error() and assign to (x) outside of the uaccess-disabled section? Will ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN 2021-11-19 11:33 ` Will Deacon @ 2021-11-19 13:44 ` Mark Rutland -1 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2021-11-19 13:44 UTC (permalink / raw) To: Will Deacon Cc: Vincent Whitchurch, catalin.marinas, kernel, linux-arm-kernel, linux-kernel On Fri, Nov 19, 2021 at 11:33:06AM +0000, Will Deacon wrote: > On Thu, Nov 18, 2021 at 05:34:17PM +0100, Vincent Whitchurch wrote: > > The value argument to put_user() must be evaluated before the TTBR0 > > switch is done. Otherwise, if it is a function and the function sleeps, > > the reserved TTBR0 will be restored when the process is switched in > > again and the process will end up in an infinite loop of faults. > > > > This problem was seen with the put_user() in schedule_tail(). A similar > > fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv: > > evaluate put_user() arg before enabling user access"). > > > > Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user") > > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > > --- > > arch/arm64/include/asm/uaccess.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > index 6e2e0b7031ab..96b26fa9d3d0 100644 > > --- a/arch/arm64/include/asm/uaccess.h > > +++ b/arch/arm64/include/asm/uaccess.h > > @@ -362,10 +362,11 @@ do { \ > > #define __put_user_error(x, ptr, err) \ > > do { \ > > __typeof__(*(ptr)) __user *__p = (ptr); \ > > + __typeof__(*(__p)) __val = (x); \ > > might_fault(); \ > > if (access_ok(__p, sizeof(*__p))) { \ > > __p = uaccess_mask_ptr(__p); \ > > - __raw_put_user((x), __p, (err)); \ > > + __raw_put_user(__val, __p, (err)); \ > > } else { \ > > (err) = -EFAULT; \ > > } \ > > > Oh, nice spot! I hope you didn't lose too much time debugging if you > actually ran into this... > > Although it seems a lot less likely to cause a problem, should we do > something similar for __get_user_error() and assign to (x) outside of > the uaccess-disabled section? I agree we should follow up with a more general cleanup to avoid any macro evaluation within user-access or tco critical sections. Since that's especially subtle for the get_*() helpers (and I beleive there may be some other latent issues in that area), I reckon we should do that as a follow-up, and shouldn't block this patch on that being done. I'll go audit that and see what I spot. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN @ 2021-11-19 13:44 ` Mark Rutland 0 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2021-11-19 13:44 UTC (permalink / raw) To: Will Deacon Cc: Vincent Whitchurch, catalin.marinas, kernel, linux-arm-kernel, linux-kernel On Fri, Nov 19, 2021 at 11:33:06AM +0000, Will Deacon wrote: > On Thu, Nov 18, 2021 at 05:34:17PM +0100, Vincent Whitchurch wrote: > > The value argument to put_user() must be evaluated before the TTBR0 > > switch is done. Otherwise, if it is a function and the function sleeps, > > the reserved TTBR0 will be restored when the process is switched in > > again and the process will end up in an infinite loop of faults. > > > > This problem was seen with the put_user() in schedule_tail(). A similar > > fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv: > > evaluate put_user() arg before enabling user access"). > > > > Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user") > > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > > --- > > arch/arm64/include/asm/uaccess.h | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > > index 6e2e0b7031ab..96b26fa9d3d0 100644 > > --- a/arch/arm64/include/asm/uaccess.h > > +++ b/arch/arm64/include/asm/uaccess.h > > @@ -362,10 +362,11 @@ do { \ > > #define __put_user_error(x, ptr, err) \ > > do { \ > > __typeof__(*(ptr)) __user *__p = (ptr); \ > > + __typeof__(*(__p)) __val = (x); \ > > might_fault(); \ > > if (access_ok(__p, sizeof(*__p))) { \ > > __p = uaccess_mask_ptr(__p); \ > > - __raw_put_user((x), __p, (err)); \ > > + __raw_put_user(__val, __p, (err)); \ > > } else { \ > > (err) = -EFAULT; \ > > } \ > > > Oh, nice spot! I hope you didn't lose too much time debugging if you > actually ran into this... > > Although it seems a lot less likely to cause a problem, should we do > something similar for __get_user_error() and assign to (x) outside of > the uaccess-disabled section? I agree we should follow up with a more general cleanup to avoid any macro evaluation within user-access or tco critical sections. Since that's especially subtle for the get_*() helpers (and I beleive there may be some other latent issues in that area), I reckon we should do that as a follow-up, and shouldn't block this patch on that being done. I'll go audit that and see what I spot. Thanks, Mark. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN 2021-11-18 16:34 ` Vincent Whitchurch @ 2021-11-19 13:04 ` Mark Rutland -1 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2021-11-19 13:04 UTC (permalink / raw) To: Vincent Whitchurch Cc: catalin.marinas, will, kernel, linux-arm-kernel, linux-kernel Hi Vincent, On Thu, Nov 18, 2021 at 05:34:17PM +0100, Vincent Whitchurch wrote: > The value argument to put_user() must be evaluated before the TTBR0 > switch is done. Otherwise, if it is a function and the function sleeps, > the reserved TTBR0 will be restored when the process is switched in > again and the process will end up in an infinite loop of faults. > This problem was seen with the put_user() in schedule_tail(). A similar > fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv: > evaluate put_user() arg before enabling user access"). > > Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user") > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > --- > arch/arm64/include/asm/uaccess.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 6e2e0b7031ab..96b26fa9d3d0 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -362,10 +362,11 @@ do { \ > #define __put_user_error(x, ptr, err) \ > do { \ > __typeof__(*(ptr)) __user *__p = (ptr); \ > + __typeof__(*(__p)) __val = (x); \ > might_fault(); \ > if (access_ok(__p, sizeof(*__p))) { \ > __p = uaccess_mask_ptr(__p); \ > - __raw_put_user((x), __p, (err)); \ > + __raw_put_user(__val, __p, (err)); \ > } else { \ > (err) = -EFAULT; \ > } \ > -- > 2.33.1 > Thanks for this, and apolgoies for introducing this issue in the first place. The patch looks correct to me. There's a similar problem in __get_kernel_nofault() with TCO, and that will need similar treatement. I think it would be better to use temporaries in __raw_put_user(), along with a comment there, so that the requirement is documented and dealt with in once place. Example diff at the end of this mail; I'm happy for you to pick that for v2, or I can send it out as a patch if your prefer. Thanks, Mark. ---->8---- diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 6e2e0b7031ab..7c0d7a7a9a50 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -351,11 +351,19 @@ do { \ } \ } while (0) +/* + * We must not call into the scheduler between uaccess_ttbr0_enable() and + * uaccess_ttbr0_disable(). As `x` and `ptr` could contain blocking functions, + * we must evaluate these first. + */ #define __raw_put_user(x, ptr, err) \ do { \ - __chk_user_ptr(ptr); \ + __typeof__(*(ptr)) __user *__rpu_ptr = (ptr); \ + __typeof__(*(ptr)) __rpu_val = (x); \ + __chk_user_ptr(__rpu_ptr); \ + \ uaccess_ttbr0_enable(); \ - __raw_put_mem("sttr", x, ptr, err); \ + __raw_put_mem("sttr", __rpu_val, __rpu_ptr, err); \ uaccess_ttbr0_disable(); \ } while (0) @@ -380,14 +388,22 @@ do { \ #define put_user __put_user +/* + * We must not call into the scheduler between __uaccess_enable_tco_async() and + * __uaccess_disable_tco_async(). As `dst` and `src` may contain blocking + * functions, we must evaluate these first. + */ #define __put_kernel_nofault(dst, src, type, err_label) \ do { \ int __pkn_err = 0; \ + __typeof__(dst) __pkn_dst = (dst); \ + __typeof__(src) __pkn_src = (src); \ \ __uaccess_enable_tco_async(); \ - __raw_put_mem("str", *((type *)(src)), \ - (__force type *)(dst), __pkn_err); \ + __raw_put_mem("str", *((type *)(__pkn_src)), \ + (__force type *)(__pkn_dst), __pkn_err); \ __uaccess_disable_tco_async(); \ + \ if (unlikely(__pkn_err)) \ goto err_label; \ } while(0) _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN @ 2021-11-19 13:04 ` Mark Rutland 0 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2021-11-19 13:04 UTC (permalink / raw) To: Vincent Whitchurch Cc: catalin.marinas, will, kernel, linux-arm-kernel, linux-kernel Hi Vincent, On Thu, Nov 18, 2021 at 05:34:17PM +0100, Vincent Whitchurch wrote: > The value argument to put_user() must be evaluated before the TTBR0 > switch is done. Otherwise, if it is a function and the function sleeps, > the reserved TTBR0 will be restored when the process is switched in > again and the process will end up in an infinite loop of faults. > This problem was seen with the put_user() in schedule_tail(). A similar > fix was done for RISC-V in commit 285a76bb2cf51b0c74c634 ("riscv: > evaluate put_user() arg before enabling user access"). > > Fixes: f253d827f33cb5a5990 ("arm64: uaccess: refactor __{get,put}_user") > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > --- > arch/arm64/include/asm/uaccess.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h > index 6e2e0b7031ab..96b26fa9d3d0 100644 > --- a/arch/arm64/include/asm/uaccess.h > +++ b/arch/arm64/include/asm/uaccess.h > @@ -362,10 +362,11 @@ do { \ > #define __put_user_error(x, ptr, err) \ > do { \ > __typeof__(*(ptr)) __user *__p = (ptr); \ > + __typeof__(*(__p)) __val = (x); \ > might_fault(); \ > if (access_ok(__p, sizeof(*__p))) { \ > __p = uaccess_mask_ptr(__p); \ > - __raw_put_user((x), __p, (err)); \ > + __raw_put_user(__val, __p, (err)); \ > } else { \ > (err) = -EFAULT; \ > } \ > -- > 2.33.1 > Thanks for this, and apolgoies for introducing this issue in the first place. The patch looks correct to me. There's a similar problem in __get_kernel_nofault() with TCO, and that will need similar treatement. I think it would be better to use temporaries in __raw_put_user(), along with a comment there, so that the requirement is documented and dealt with in once place. Example diff at the end of this mail; I'm happy for you to pick that for v2, or I can send it out as a patch if your prefer. Thanks, Mark. ---->8---- diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 6e2e0b7031ab..7c0d7a7a9a50 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -351,11 +351,19 @@ do { \ } \ } while (0) +/* + * We must not call into the scheduler between uaccess_ttbr0_enable() and + * uaccess_ttbr0_disable(). As `x` and `ptr` could contain blocking functions, + * we must evaluate these first. + */ #define __raw_put_user(x, ptr, err) \ do { \ - __chk_user_ptr(ptr); \ + __typeof__(*(ptr)) __user *__rpu_ptr = (ptr); \ + __typeof__(*(ptr)) __rpu_val = (x); \ + __chk_user_ptr(__rpu_ptr); \ + \ uaccess_ttbr0_enable(); \ - __raw_put_mem("sttr", x, ptr, err); \ + __raw_put_mem("sttr", __rpu_val, __rpu_ptr, err); \ uaccess_ttbr0_disable(); \ } while (0) @@ -380,14 +388,22 @@ do { \ #define put_user __put_user +/* + * We must not call into the scheduler between __uaccess_enable_tco_async() and + * __uaccess_disable_tco_async(). As `dst` and `src` may contain blocking + * functions, we must evaluate these first. + */ #define __put_kernel_nofault(dst, src, type, err_label) \ do { \ int __pkn_err = 0; \ + __typeof__(dst) __pkn_dst = (dst); \ + __typeof__(src) __pkn_src = (src); \ \ __uaccess_enable_tco_async(); \ - __raw_put_mem("str", *((type *)(src)), \ - (__force type *)(dst), __pkn_err); \ + __raw_put_mem("str", *((type *)(__pkn_src)), \ + (__force type *)(__pkn_dst), __pkn_err); \ __uaccess_disable_tco_async(); \ + \ if (unlikely(__pkn_err)) \ goto err_label; \ } while(0) ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN 2021-11-19 13:04 ` Mark Rutland @ 2021-11-22 10:01 ` Mark Rutland -1 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2021-11-22 10:01 UTC (permalink / raw) To: Vincent Whitchurch Cc: catalin.marinas, will, kernel, linux-arm-kernel, linux-kernel On Fri, Nov 19, 2021 at 01:04:50PM +0000, Mark Rutland wrote: > I think it would be better to use temporaries in __raw_put_user(), along > with a comment there, so that the requirement is documented and dealt > with in once place. Example diff at the end of this mail; I'm happy for > you to pick that for v2, or I can send it out as a patch if your prefer. Looking at this again, it's easier than I thought to fix up the get_*() cases; the only real pain point iss `err` in get_user_error() and put_user_error(), but as those are only used externally in the architectural signal code, we can sort that out as a follow-up. With that in mind, I reckon we should so something like the below. Thanks, Mark. ---->8---- diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 6e2e0b7031ab..3a5ff5e20586 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -281,12 +281,22 @@ do { \ (x) = (__force __typeof__(*(ptr)))__gu_val; \ } while (0) +/* + * We must not call into the scheduler between uaccess_ttbr0_enable() and + * uaccess_ttbr0_disable(). As `x` and `ptr` could contain blocking functions, + * we must evaluate these outside of the critical section. + */ #define __raw_get_user(x, ptr, err) \ do { \ + __typeof__(*(ptr)) __user *__rgu_ptr = (ptr); \ + __typeof__(x) __rgu_val; \ __chk_user_ptr(ptr); \ + \ uaccess_ttbr0_enable(); \ - __raw_get_mem("ldtr", x, ptr, err); \ + __raw_get_mem("ldtr", __rgu_val, __rgu_ptr, err); \ uaccess_ttbr0_disable(); \ + \ + (x) = __rgu_val; \ } while (0) #define __get_user_error(x, ptr, err) \ @@ -310,14 +320,22 @@ do { \ #define get_user __get_user +/* + * We must not call into the scheduler between __uaccess_enable_tco_async() and + * __uaccess_disable_tco_async(). As `dst` and `src` may contain blocking + * functions, we must evaluate these outside of the critical section. + */ #define __get_kernel_nofault(dst, src, type, err_label) \ do { \ + __typeof__(dst) __gkn_dst = (dst); \ + __typeof__(src) __gkn_src = (src); \ int __gkn_err = 0; \ \ __uaccess_enable_tco_async(); \ - __raw_get_mem("ldr", *((type *)(dst)), \ - (__force type *)(src), __gkn_err); \ + __raw_get_mem("ldr", *((type *)(__gkn_dst)), \ + (__force type *)(__gkn_src), __gkn_err); \ __uaccess_disable_tco_async(); \ + \ if (unlikely(__gkn_err)) \ goto err_label; \ } while (0) @@ -351,11 +369,19 @@ do { \ } \ } while (0) +/* + * We must not call into the scheduler between uaccess_ttbr0_enable() and + * uaccess_ttbr0_disable(). As `x` and `ptr` could contain blocking functions, + * we must evaluate these outside of the critical section. + */ #define __raw_put_user(x, ptr, err) \ do { \ - __chk_user_ptr(ptr); \ + __typeof__(*(ptr)) __user *__rpu_ptr = (ptr); \ + __typeof__(*(ptr)) __rpu_val = (x); \ + __chk_user_ptr(__rpu_ptr); \ + \ uaccess_ttbr0_enable(); \ - __raw_put_mem("sttr", x, ptr, err); \ + __raw_put_mem("sttr", __rpu_val, __rpu_ptr, err); \ uaccess_ttbr0_disable(); \ } while (0) @@ -380,14 +406,22 @@ do { \ #define put_user __put_user +/* + * We must not call into the scheduler between __uaccess_enable_tco_async() and + * __uaccess_disable_tco_async(). As `dst` and `src` may contain blocking + * functions, we must evaluate these outside of the critical section. + */ #define __put_kernel_nofault(dst, src, type, err_label) \ do { \ + __typeof__(dst) __pkn_dst = (dst); \ + __typeof__(src) __pkn_src = (src); \ int __pkn_err = 0; \ \ __uaccess_enable_tco_async(); \ - __raw_put_mem("str", *((type *)(src)), \ - (__force type *)(dst), __pkn_err); \ + __raw_put_mem("str", *((type *)(__pkn_src)), \ + (__force type *)(__pkn_dst), __pkn_err); \ __uaccess_disable_tco_async(); \ + \ if (unlikely(__pkn_err)) \ goto err_label; \ } while(0) -- 2.11.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN @ 2021-11-22 10:01 ` Mark Rutland 0 siblings, 0 replies; 12+ messages in thread From: Mark Rutland @ 2021-11-22 10:01 UTC (permalink / raw) To: Vincent Whitchurch Cc: catalin.marinas, will, kernel, linux-arm-kernel, linux-kernel On Fri, Nov 19, 2021 at 01:04:50PM +0000, Mark Rutland wrote: > I think it would be better to use temporaries in __raw_put_user(), along > with a comment there, so that the requirement is documented and dealt > with in once place. Example diff at the end of this mail; I'm happy for > you to pick that for v2, or I can send it out as a patch if your prefer. Looking at this again, it's easier than I thought to fix up the get_*() cases; the only real pain point iss `err` in get_user_error() and put_user_error(), but as those are only used externally in the architectural signal code, we can sort that out as a follow-up. With that in mind, I reckon we should so something like the below. Thanks, Mark. ---->8---- diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 6e2e0b7031ab..3a5ff5e20586 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -281,12 +281,22 @@ do { \ (x) = (__force __typeof__(*(ptr)))__gu_val; \ } while (0) +/* + * We must not call into the scheduler between uaccess_ttbr0_enable() and + * uaccess_ttbr0_disable(). As `x` and `ptr` could contain blocking functions, + * we must evaluate these outside of the critical section. + */ #define __raw_get_user(x, ptr, err) \ do { \ + __typeof__(*(ptr)) __user *__rgu_ptr = (ptr); \ + __typeof__(x) __rgu_val; \ __chk_user_ptr(ptr); \ + \ uaccess_ttbr0_enable(); \ - __raw_get_mem("ldtr", x, ptr, err); \ + __raw_get_mem("ldtr", __rgu_val, __rgu_ptr, err); \ uaccess_ttbr0_disable(); \ + \ + (x) = __rgu_val; \ } while (0) #define __get_user_error(x, ptr, err) \ @@ -310,14 +320,22 @@ do { \ #define get_user __get_user +/* + * We must not call into the scheduler between __uaccess_enable_tco_async() and + * __uaccess_disable_tco_async(). As `dst` and `src` may contain blocking + * functions, we must evaluate these outside of the critical section. + */ #define __get_kernel_nofault(dst, src, type, err_label) \ do { \ + __typeof__(dst) __gkn_dst = (dst); \ + __typeof__(src) __gkn_src = (src); \ int __gkn_err = 0; \ \ __uaccess_enable_tco_async(); \ - __raw_get_mem("ldr", *((type *)(dst)), \ - (__force type *)(src), __gkn_err); \ + __raw_get_mem("ldr", *((type *)(__gkn_dst)), \ + (__force type *)(__gkn_src), __gkn_err); \ __uaccess_disable_tco_async(); \ + \ if (unlikely(__gkn_err)) \ goto err_label; \ } while (0) @@ -351,11 +369,19 @@ do { \ } \ } while (0) +/* + * We must not call into the scheduler between uaccess_ttbr0_enable() and + * uaccess_ttbr0_disable(). As `x` and `ptr` could contain blocking functions, + * we must evaluate these outside of the critical section. + */ #define __raw_put_user(x, ptr, err) \ do { \ - __chk_user_ptr(ptr); \ + __typeof__(*(ptr)) __user *__rpu_ptr = (ptr); \ + __typeof__(*(ptr)) __rpu_val = (x); \ + __chk_user_ptr(__rpu_ptr); \ + \ uaccess_ttbr0_enable(); \ - __raw_put_mem("sttr", x, ptr, err); \ + __raw_put_mem("sttr", __rpu_val, __rpu_ptr, err); \ uaccess_ttbr0_disable(); \ } while (0) @@ -380,14 +406,22 @@ do { \ #define put_user __put_user +/* + * We must not call into the scheduler between __uaccess_enable_tco_async() and + * __uaccess_disable_tco_async(). As `dst` and `src` may contain blocking + * functions, we must evaluate these outside of the critical section. + */ #define __put_kernel_nofault(dst, src, type, err_label) \ do { \ + __typeof__(dst) __pkn_dst = (dst); \ + __typeof__(src) __pkn_src = (src); \ int __pkn_err = 0; \ \ __uaccess_enable_tco_async(); \ - __raw_put_mem("str", *((type *)(src)), \ - (__force type *)(dst), __pkn_err); \ + __raw_put_mem("str", *((type *)(__pkn_src)), \ + (__force type *)(__pkn_dst), __pkn_err); \ __uaccess_disable_tco_async(); \ + \ if (unlikely(__pkn_err)) \ goto err_label; \ } while(0) -- 2.11.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN 2021-11-19 13:04 ` Mark Rutland @ 2021-11-22 10:09 ` Vincent Whitchurch -1 siblings, 0 replies; 12+ messages in thread From: Vincent Whitchurch @ 2021-11-22 10:09 UTC (permalink / raw) To: Mark Rutland Cc: catalin.marinas@arm.com, will@kernel.org, kernel, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org On Fri, Nov 19, 2021 at 02:04:50PM +0100, Mark Rutland wrote: > I think it would be better to use temporaries in __raw_put_user(), along > with a comment there, so that the requirement is documented and dealt > with in once place. Example diff at the end of this mail; I'm happy for > you to pick that for v2, or I can send it out as a patch if your prefer. Please feel free to send it out as a patch. Thank you. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN @ 2021-11-22 10:09 ` Vincent Whitchurch 0 siblings, 0 replies; 12+ messages in thread From: Vincent Whitchurch @ 2021-11-22 10:09 UTC (permalink / raw) To: Mark Rutland Cc: catalin.marinas@arm.com, will@kernel.org, kernel, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org On Fri, Nov 19, 2021 at 02:04:50PM +0100, Mark Rutland wrote: > I think it would be better to use temporaries in __raw_put_user(), along > with a comment there, so that the requirement is documented and dealt > with in once place. Example diff at the end of this mail; I'm happy for > you to pick that for v2, or I can send it out as a patch if your prefer. Please feel free to send it out as a patch. Thank you. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-11-22 11:28 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-18 16:34 [PATCH] arm64: uaccess: fix put_user() with TTBR0 PAN Vincent Whitchurch 2021-11-18 16:34 ` Vincent Whitchurch 2021-11-19 11:33 ` Will Deacon 2021-11-19 11:33 ` Will Deacon 2021-11-19 13:44 ` Mark Rutland 2021-11-19 13:44 ` Mark Rutland 2021-11-19 13:04 ` Mark Rutland 2021-11-19 13:04 ` Mark Rutland 2021-11-22 10:01 ` Mark Rutland 2021-11-22 10:01 ` Mark Rutland 2021-11-22 10:09 ` Vincent Whitchurch 2021-11-22 10:09 ` Vincent Whitchurch
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.