On 29/08/2023 16:10, Luca Vizzarro wrote:
When working with PCuABI, a user pointer is a capability carrying metadata such as access bounds and permissions. These are automatically enforced in hardware when a direct access is performed, which triggers a fault if either the bounds or permissions checks failed.
Whenever a futex key is accessed and an access fault occurs, the assumption is that the page containing the futex key must have faulted and the code proceeds to fix it up. Because no distinction is made between a capability fault and a page fault, an explicit capability check needs to be employed to verify which of the two triggered the fault. Therefore unnecessarily avoiding a a user page fault fixup, if the former case happened.
I can't parse that last sentence. The main point we should make is that in the capability fault case, we must bail out as we would otherwise enter an infinite loop: while(1) { get_futex_key() [no capability check] futex_atomic...() [-EFAULT due to capability fault] fault_in_user_writeable() [faults in the user page, has no effect on the capability] }
We should also explain that we make no explicit check in get_futex_key() because it does not actually dereference uaddr, and the futex operations automatically check the capability metadata as you explained in the first paragraph.
Kevin
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
kernel/futex/core.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/kernel/futex/core.c b/kernel/futex/core.c index 4b79b5e2ad3b..8efd02515654 100644 --- a/kernel/futex/core.c +++ b/kernel/futex/core.c @@ -226,8 +226,6 @@ 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.
@@ -413,7 +411,14 @@ int fault_in_user_writeable(u32 __user *uaddr) struct mm_struct *mm = current->mm; int ret;
- /* TODO [PCuABI] - capability checks for uaccess */
- /*
* When working in PCuABI, we may have reached this point because an* atomic load/store failed as a consequence of a capability fault* and not a page fault. To cover this case we should employ an explicit* check, so that we can fail and return early.*/- if (!check_user_ptr_rw(uaddr, sizeof(u32)))
return -EFAULT;mmap_read_lock(mm); ret = fixup_user_fault(mm, user_ptr_addr(uaddr),