This commit tackles the issue reported at: https://git.morello-project.org/morello/kernel/linux/-/issues/6
Commit also available at: https://git.morello-project.org/Sevenarth/linux/-/commits/morello/futex-v3
v3: - reworded commit bodies - removed a redundant include - fixed whitespace alignment v2: - split code in 3 commits as suggested - added more details in the commit bodies - updated the TODO notation for futex.h - updated the prefix for A64/C64 definitions in futex.h - updated the asm constraint's name to follow naming conventions - updated the robust list entry fetch code to use the pre-existing helper USER_PTR_ALIGN_DOWN - reverted pointer comparisons
Luca Vizzarro (3): arm64: futex: Enable capability-based uaccess futex: Handle capability-based robust list entries futex: Add explicit capability checking TODOs
arch/arm64/include/asm/futex.h | 47 ++++++++++++++++++++++++---------- kernel/futex/core.c | 19 ++++++-------- 2 files changed, 41 insertions(+), 25 deletions(-)
When working with PCuABI we need to ensure that we receive capabilities from user space instead of pointers and perform the appropriate memory accesses.
This commit updates the arm64-specific code for the futex module so that the atomic load and store work with capabilities, hence enabling PCuABI. This commit therefore reverts "arm64: futex: Access user memory via raw 64-bit pointers".
Because the instructions used to perform the atomic operations, ldxr and stlxr, only support capabilities in C64 mode, this commit therefore brings C64/A64 mode switching to the inline assembly.
Whenever a load/store faults, the exception raised will be handled by ex_handler_uaccess_err_zero(), which will correctly set -EFAULT as error and return execution past the loop. Although exception handling is always ran in A64, hence the mode is switched back from C64, this works as expected as the C64/A64 mode is saved per exception level (SPSR_ELx.C64). Thus the selected C64 mode is retained upon return. What is more relevant in this case is that the exception correctly returns to the fixup machinery (label 3), which is when the switch back to A64 happens.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- arch/arm64/include/asm/futex.h | 47 ++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h index 99d73e8a3175..2ef9b5167007 100644 --- a/arch/arm64/include/asm/futex.h +++ b/arch/arm64/include/asm/futex.h @@ -10,6 +10,18 @@
#include <asm/errno.h>
+#ifdef CONFIG_CHERI_PURECAP_UABI +#define __ASM_SWITCH_TO_C64 " bx #4\n" \ + ".arch morello+c64\n" +#define __ASM_SWITCH_TO_A64 " bx #4\n" \ + ".arch morello\n" +#define __ASM_RW_UPTR_CONSTR "+C" +#else +#define __ASM_SWITCH_TO_C64 +#define __ASM_SWITCH_TO_A64 +#define __ASM_RW_UPTR_CONSTR "+r" +#endif + #define FUTEX_MAX_LOOPS 128 /* What's the largest number you can think of? */
#define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg) \ @@ -18,20 +30,24 @@ do { \ \ uaccess_enable_privileged(); \ asm volatile( \ -" prfm pstl1strm, %2\n" \ -"1: ldxr %w1, %2\n" \ + __ASM_SWITCH_TO_C64 \ +" prfm pstl1strm, [%2]\n" \ +"1: ldxr %w1, [%2]\n" \ insn "\n" \ -"2: stlxr %w0, %w3, %2\n" \ +"2: stlxr %w0, %w3, [%2]\n" \ " cbz %w0, 3f\n" \ " sub %w4, %w4, %w0\n" \ " cbnz %w4, 1b\n" \ " mov %w0, %w6\n" \ "3:\n" \ " dmb ish\n" \ + __ASM_SWITCH_TO_A64 \ _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0) \ _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0) \ - : "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp), \ - "+r" (loops) \ + /* TODO [PCuABI]: temporary solution for uaddr. Should be reverted to +Q + * once LLVM supports it for capabilities. */ \ + : "=&r" (ret), "=&r" (oldval), __ASM_RW_UPTR_CONSTR (uaddr), \ + "=&r" (tmp), "+r" (loops) \ : "r" (oparg), "Ir" (-EAGAIN) \ : "memory"); \ uaccess_disable_privileged(); \ @@ -41,8 +57,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *_uaddr) { int oldval = 0, ret, tmp; - /* TODO [PCuABI] - perform the access via the user capability */ - u32 *uaddr = (u32 *)user_ptr_addr(__uaccess_mask_ptr(_uaddr)); + u32 __user *uaddr = __uaccess_mask_ptr(_uaddr);
if (!access_ok(_uaddr, sizeof(u32))) return -EFAULT; @@ -85,20 +100,20 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *_uaddr, int ret = 0; unsigned int loops = FUTEX_MAX_LOOPS; u32 val, tmp; - u32 *uaddr; + u32 __user *uaddr;
if (!access_ok(_uaddr, sizeof(u32))) return -EFAULT;
- /* TODO [PCuABI] - perform the access via the user capability */ - uaddr = (u32 *)user_ptr_addr(__uaccess_mask_ptr(_uaddr)); + uaddr = __uaccess_mask_ptr(_uaddr); uaccess_enable_privileged(); asm volatile("// futex_atomic_cmpxchg_inatomic\n" -" prfm pstl1strm, %2\n" -"1: ldxr %w1, %2\n" + __ASM_SWITCH_TO_C64 +" prfm pstl1strm, [%2]\n" +"1: ldxr %w1, [%2]\n" " sub %w3, %w1, %w5\n" " cbnz %w3, 4f\n" -"2: stlxr %w3, %w6, %2\n" +"2: stlxr %w3, %w6, [%2]\n" " cbz %w3, 3f\n" " sub %w4, %w4, %w3\n" " cbnz %w4, 1b\n" @@ -106,9 +121,13 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *_uaddr, "3:\n" " dmb ish\n" "4:\n" + __ASM_SWITCH_TO_A64 _ASM_EXTABLE_UACCESS_ERR(1b, 4b, %w0) _ASM_EXTABLE_UACCESS_ERR(2b, 4b, %w0) - : "+r" (ret), "=&r" (val), "+Q" (*uaddr), "=&r" (tmp), "+r" (loops) + /* TODO [PCuABI]: temporary solution for uaddr. Should be reverted to +Q once + * LLVM supports it for capabilities. */ + : "+r" (ret), "=&r" (val), __ASM_RW_UPTR_CONSTR (uaddr), "=&r" (tmp), + "+r" (loops) : "r" (oldval), "r" (newval), "Ir" (-EAGAIN) : "memory"); uaccess_disable_privileged();
A robust list is implemented in the user space, but the kernel holds the right to walk it. Given that a robust list may implement the priority inheritance method, the approach chosen to inform the kernel of this feature is to encode the LSB of the uaddr of a list entry to walk from.
In a PCuABI environment the userspace pointer is not a raw address but a capability. So that we can clear the encoded priority-inheritance flag, the helper USER_PTR_ALIGN_DOWN is used without resorting to CHERI-specific code.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- kernel/futex/core.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/kernel/futex/core.c b/kernel/futex/core.c index 759332a26b5a..c85cb1239b54 100644 --- a/kernel/futex/core.c +++ b/kernel/futex/core.c @@ -750,20 +750,13 @@ static inline int fetch_robust_entry(struct robust_list __user **entry, #endif unsigned int *pi) { - unsigned long uentry; + struct robust_list __user *uentry;
- if (get_user(uentry, (unsigned long __user *)head)) + if (get_user_ptr(uentry, head)) return -EFAULT;
- /* - * TODO [PCuABI] - pointer conversion to be checked - * Each entry points to either next one or head of the list - * so this should probably operate on capabilities and use - * get_user_ptr instead, or validate the capability prior to - * get_user - */ - *entry = uaddr_to_user_ptr(uentry & ~1UL); - *pi = uentry & 1; + *entry = USER_PTR_ALIGN_DOWN(uentry, 2); + *pi = user_ptr_addr(uentry) & 1;
return 0; }
Within the futex module, there are some cases in which a user address is handled. When working with PCuABI this means that the capability is discarded, and no checks can be performed by the hardware.
This commit adds TODOs whenever explicit capability checks need to be performed, right before the raw pointer is extracted.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- kernel/futex/core.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/kernel/futex/core.c b/kernel/futex/core.c index c85cb1239b54..9613080ccf0c 100644 --- a/kernel/futex/core.c +++ b/kernel/futex/core.c @@ -226,6 +226,8 @@ int get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key, struct address_space *mapping; int err, ro = 0;
+ /* TODO [PCuABI] - capability checks for uaccess */ + /* * The futex address must be "naturally" aligned. */ @@ -411,6 +413,8 @@ int fault_in_user_writeable(u32 __user *uaddr) struct mm_struct *mm = current->mm; int ret;
+ /* TODO [PCuABI] - capability checks for uaccess */ + mmap_read_lock(mm); ret = fixup_user_fault(mm, user_ptr_addr(uaddr), FAULT_FLAG_WRITE, NULL);
On 12/04/2023 18:11, Luca Vizzarro wrote:
When working with PCuABI we need to ensure that we receive capabilities from user space instead of pointers and perform the appropriate memory accesses.
This commit updates the arm64-specific code for the futex module so that the atomic load and store work with capabilities, hence enabling PCuABI. This commit therefore reverts "arm64: futex: Access user memory via raw 64-bit pointers".
Because the instructions used to perform the atomic operations, ldxr and stlxr, only support capabilities in C64 mode, this commit therefore brings C64/A64 mode switching to the inline assembly.
Whenever a load/store faults, the exception raised will be handled by ex_handler_uaccess_err_zero(), which will correctly set -EFAULT as error and return execution past the loop. Although exception handling is always ran in A64, hence the mode is switched back from C64, this works as expected as the C64/A64 mode is saved per exception level (SPSR_ELx.C64). Thus the selected C64 mode is retained upon return.
These two sentences are built in a strange way, mixing up exception entry and return. How about:
Exceptions are always handled in A64, hence the mode is automatically switched back from C64. The A64/C64 mode is saved in SPSR_ELx.C64 on exception entry, thus C64 is restored upon return.
Happy with the patches otherwise, so if you agree with that wording I can take care of changing the commit message when merging them.
Kevin
What is more relevant in this case is that the exception correctly returns to the fixup machinery (label 3), which is when the switch back to A64 happens.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
arch/arm64/include/asm/futex.h | 47 ++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h index 99d73e8a3175..2ef9b5167007 100644 --- a/arch/arm64/include/asm/futex.h +++ b/arch/arm64/include/asm/futex.h @@ -10,6 +10,18 @@ #include <asm/errno.h> +#ifdef CONFIG_CHERI_PURECAP_UABI +#define __ASM_SWITCH_TO_C64 " bx #4\n" \
".arch morello+c64\n"+#define __ASM_SWITCH_TO_A64 " bx #4\n" \
".arch morello\n"+#define __ASM_RW_UPTR_CONSTR "+C" +#else +#define __ASM_SWITCH_TO_C64 +#define __ASM_SWITCH_TO_A64 +#define __ASM_RW_UPTR_CONSTR "+r" +#endif
#define FUTEX_MAX_LOOPS 128 /* What's the largest number you can think of? */ #define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg) \ @@ -18,20 +30,24 @@ do { \ \ uaccess_enable_privileged(); \ asm volatile( \ -" prfm pstl1strm, %2\n" \ -"1: ldxr %w1, %2\n" \
- __ASM_SWITCH_TO_C64 \
+" prfm pstl1strm, [%2]\n" \ +"1: ldxr %w1, [%2]\n" \ insn "\n" \ -"2: stlxr %w0, %w3, %2\n" \ +"2: stlxr %w0, %w3, [%2]\n" \ " cbz %w0, 3f\n" \ " sub %w4, %w4, %w0\n" \ " cbnz %w4, 1b\n" \ " mov %w0, %w6\n" \ "3:\n" \ " dmb ish\n" \
- __ASM_SWITCH_TO_A64 \ _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0) \ _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0) \
- : "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp), \
"+r" (loops) \
- /* TODO [PCuABI]: temporary solution for uaddr. Should be reverted to +Q
* once LLVM supports it for capabilities. */ \- : "=&r" (ret), "=&r" (oldval), __ASM_RW_UPTR_CONSTR (uaddr), \
"=&r" (tmp), "+r" (loops) \@@ -41,8 +57,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *_uaddr) { int oldval = 0, ret, tmp;
- /* TODO [PCuABI] - perform the access via the user capability */
- u32 *uaddr = (u32 *)user_ptr_addr(__uaccess_mask_ptr(_uaddr));
- u32 __user *uaddr = __uaccess_mask_ptr(_uaddr);
if (!access_ok(_uaddr, sizeof(u32))) return -EFAULT; @@ -85,20 +100,20 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *_uaddr, int ret = 0; unsigned int loops = FUTEX_MAX_LOOPS; u32 val, tmp;
- u32 *uaddr;
- u32 __user *uaddr;
if (!access_ok(_uaddr, sizeof(u32))) return -EFAULT;
- /* TODO [PCuABI] - perform the access via the user capability */
- uaddr = (u32 *)user_ptr_addr(__uaccess_mask_ptr(_uaddr));
- uaddr = __uaccess_mask_ptr(_uaddr); uaccess_enable_privileged(); asm volatile("// futex_atomic_cmpxchg_inatomic\n"
-" prfm pstl1strm, %2\n" -"1: ldxr %w1, %2\n"
- __ASM_SWITCH_TO_C64
+" prfm pstl1strm, [%2]\n" +"1: ldxr %w1, [%2]\n" " sub %w3, %w1, %w5\n" " cbnz %w3, 4f\n" -"2: stlxr %w3, %w6, %2\n" +"2: stlxr %w3, %w6, [%2]\n" " cbz %w3, 3f\n" " sub %w4, %w4, %w3\n" " cbnz %w4, 1b\n" @@ -106,9 +121,13 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *_uaddr, "3:\n" " dmb ish\n" "4:\n"
- __ASM_SWITCH_TO_A64 _ASM_EXTABLE_UACCESS_ERR(1b, 4b, %w0) _ASM_EXTABLE_UACCESS_ERR(2b, 4b, %w0)
- : "+r" (ret), "=&r" (val), "+Q" (*uaddr), "=&r" (tmp), "+r" (loops)
- /* TODO [PCuABI]: temporary solution for uaddr. Should be reverted to +Q once
* LLVM supports it for capabilities. */- : "+r" (ret), "=&r" (val), __ASM_RW_UPTR_CONSTR (uaddr), "=&r" (tmp),
"+r" (loops)
On 13-04-2023 09:04, Kevin Brodsky wrote:
On 12/04/2023 18:11, Luca Vizzarro wrote:
When working with PCuABI we need to ensure that we receive capabilities from user space instead of pointers and perform the appropriate memory accesses.
This commit updates the arm64-specific code for the futex module so that the atomic load and store work with capabilities, hence enabling PCuABI. This commit therefore reverts "arm64: futex: Access user memory via raw 64-bit pointers".
Because the instructions used to perform the atomic operations, ldxr and stlxr, only support capabilities in C64 mode, this commit therefore brings C64/A64 mode switching to the inline assembly.
Whenever a load/store faults, the exception raised will be handled by ex_handler_uaccess_err_zero(), which will correctly set -EFAULT as error and return execution past the loop. Although exception handling is always ran in A64, hence the mode is switched back from C64, this works as expected as the C64/A64 mode is saved per exception level (SPSR_ELx.C64). Thus the selected C64 mode is retained upon return.
These two sentences are built in a strange way, mixing up exception entry and return. How about:
Exceptions are always handled in A64, hence the mode is automatically switched back from C64. The A64/C64 mode is saved in SPSR_ELx.C64 on exception entry, thus C64 is restored upon return.
Happy with the patches otherwise, so if you agree with that wording I can take care of changing the commit message when merging them.
I am a bit late to the party. FWIW, I have just finished reviewing the series and it looks good to me, great work!
Tudor
Kevin
What is more relevant in this case is that the exception correctly returns to the fixup machinery (label 3), which is when the switch back to A64 happens.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
arch/arm64/include/asm/futex.h | 47 ++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h index 99d73e8a3175..2ef9b5167007 100644 --- a/arch/arm64/include/asm/futex.h +++ b/arch/arm64/include/asm/futex.h @@ -10,6 +10,18 @@ #include <asm/errno.h> +#ifdef CONFIG_CHERI_PURECAP_UABI +#define __ASM_SWITCH_TO_C64 " bx #4\n" \
".arch morello+c64\n"+#define __ASM_SWITCH_TO_A64 " bx #4\n" \
".arch morello\n"+#define __ASM_RW_UPTR_CONSTR "+C" +#else +#define __ASM_SWITCH_TO_C64 +#define __ASM_SWITCH_TO_A64 +#define __ASM_RW_UPTR_CONSTR "+r" +#endif
- #define FUTEX_MAX_LOOPS 128 /* What's the largest number you can think of? */
#define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg) \ @@ -18,20 +30,24 @@ do { \ \ uaccess_enable_privileged(); \ asm volatile( \ -" prfm pstl1strm, %2\n" \ -"1: ldxr %w1, %2\n" \
- __ASM_SWITCH_TO_C64 \
+" prfm pstl1strm, [%2]\n" \ +"1: ldxr %w1, [%2]\n" \ insn "\n" \ -"2: stlxr %w0, %w3, %2\n" \ +"2: stlxr %w0, %w3, [%2]\n" \ " cbz %w0, 3f\n" \ " sub %w4, %w4, %w0\n" \ " cbnz %w4, 1b\n" \ " mov %w0, %w6\n" \ "3:\n" \ " dmb ish\n" \
- __ASM_SWITCH_TO_A64 \ _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0) \ _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0) \
- : "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp), \
"+r" (loops) \
- /* TODO [PCuABI]: temporary solution for uaddr. Should be reverted to +Q
* once LLVM supports it for capabilities. */ \- : "=&r" (ret), "=&r" (oldval), __ASM_RW_UPTR_CONSTR (uaddr), \
"=&r" (tmp), "+r" (loops) \@@ -41,8 +57,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *_uaddr) { int oldval = 0, ret, tmp;
- /* TODO [PCuABI] - perform the access via the user capability */
- u32 *uaddr = (u32 *)user_ptr_addr(__uaccess_mask_ptr(_uaddr));
- u32 __user *uaddr = __uaccess_mask_ptr(_uaddr);
if (!access_ok(_uaddr, sizeof(u32))) return -EFAULT; @@ -85,20 +100,20 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *_uaddr, int ret = 0; unsigned int loops = FUTEX_MAX_LOOPS; u32 val, tmp;
- u32 *uaddr;
- u32 __user *uaddr;
if (!access_ok(_uaddr, sizeof(u32))) return -EFAULT;
- /* TODO [PCuABI] - perform the access via the user capability */
- uaddr = (u32 *)user_ptr_addr(__uaccess_mask_ptr(_uaddr));
- uaddr = __uaccess_mask_ptr(_uaddr); uaccess_enable_privileged(); asm volatile("// futex_atomic_cmpxchg_inatomic\n"
-" prfm pstl1strm, %2\n" -"1: ldxr %w1, %2\n"
- __ASM_SWITCH_TO_C64
+" prfm pstl1strm, [%2]\n" +"1: ldxr %w1, [%2]\n" " sub %w3, %w1, %w5\n" " cbnz %w3, 4f\n" -"2: stlxr %w3, %w6, %2\n" +"2: stlxr %w3, %w6, [%2]\n" " cbz %w3, 3f\n" " sub %w4, %w4, %w3\n" " cbnz %w4, 1b\n" @@ -106,9 +121,13 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *_uaddr, "3:\n" " dmb ish\n" "4:\n"
- __ASM_SWITCH_TO_A64 _ASM_EXTABLE_UACCESS_ERR(1b, 4b, %w0) _ASM_EXTABLE_UACCESS_ERR(2b, 4b, %w0)
- : "+r" (ret), "=&r" (val), "+Q" (*uaddr), "=&r" (tmp), "+r" (loops)
- /* TODO [PCuABI]: temporary solution for uaddr. Should be reverted to +Q once
* LLVM supports it for capabilities. */- : "+r" (ret), "=&r" (val), __ASM_RW_UPTR_CONSTR (uaddr), "=&r" (tmp),
"+r" (loops)
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
On 13/04/2023 09:04, Kevin Brodsky wrote:
Happy with the patches otherwise, so if you agree with that wording I can take care of changing the commit message when merging them.
Sure thing, sounds good to me!
Thank you for your review!
Best, Luca
On 13/04/2023 11:28, Luca Vizzarro wrote:
On 13/04/2023 09:04, Kevin Brodsky wrote:
Happy with the patches otherwise, so if you agree with that wording I can take care of changing the commit message when merging them.
Sure thing, sounds good to me!
Thank you for your review!
Applied on next with that wording tweak, thanks! And thanks Tudor for the extra review, always appreciated.
Kevin
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Luca Vizzarro -
Tudor Cretu