| Message ID | 20211118163417.21617-1-vincent.whitchurch@axis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: uaccess: fix put_user() with TTBR0 PAN | expand |
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
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)
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.
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)
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.
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; \ } \
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(-)