On 05/06/2023 17:46, Luca Vizzarro wrote:
Whenever a GUP call is made, a user address in the form of a 64-bit integer is used to lookup its corresponding page. When working in PCuABI, this means that the metadata of the user capability gets discarded, hence any access made by the GUP is not checked in hardware.
This commit introduces explicit capability checks whenever a call to the current mm through the GUP functions is made.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
kernel/futex/core.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/kernel/futex/core.c b/kernel/futex/core.c index 9613080ccf0c..f4ede8836a5a 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.
@@ -239,6 +237,12 @@ int get_futex_key(u32 __user *uaddr, bool fshared, union futex_key *key, if (unlikely(!access_ok(uaddr, sizeof(u32)))) return -EFAULT;
- if (rw == FUTEX_READ && !check_user_ptr_read(uaddr, sizeof(u32)))
Nit: similar checks are marked with unlikely() and these are equally improbable, so we could use unlikely() here too.
Thinking some more, I am not convinced we need such checks at all. This function does not access any memory in itself, even in the shared case: it uses GUP to obtain the underlying page's metadata, not its contents. Given your work on making futex operations capability-based, all the actual memory accesses via uaddr should already be checked at the point of use (our preference). I might be missing something though, quite a bit of subtlety here!
return -EFAULT;- if (rw == FUTEX_WRITE && !check_user_ptr_rw(uaddr, sizeof(u32)))
return -EFAULT;- if (unlikely(should_fail_futex(fshared))) return -EFAULT;
@@ -413,7 +417,14 @@ int fault_in_user_writeable(u32 __user *uaddr) struct mm_struct *mm = current->mm; int ret;
- /* TODO [PCuABI] - capability checks for uaccess */
- /*
* We may have reached this point because the atomic load/store failed* as a consequence of the capability's bad permissions and/or bounds,
We should start by mentioning PCuABI, otherwise it's unclear why we're talking about capabilities. Also to generalise slightly, I would say "as a consequence of a capability fault" (which may happen for other reasons, i.e. invalid tag or sealed capability).
Kevin
* 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),