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.
It doesn't look like the commit message has much to do with what the patch does? fault_in_safe_writeable() doesn't use GUP itself. It's fine to have the same sentence / paragraph explaining what the series the commit is part of is trying to achieve, typically at the end, but the first paragraph needs to relate to what the commit is specifically doing. That applies to all patches in the series.
Regarding what this specific patch is doing, I think it's worth justifying the check we're adding, as it's a little borderline: we're not actually accessing user memory, just faulting in pages. That said, I think the fact that fault_in_{readable,writeable} end up checking uaddr (by calling uaccess functions) is a good reason to have a check here as well.
Also worth explaining here why we change the prototype of fault_in_safe_writeable() (and why it's ok to do that), as it's really not obvious.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
include/linux/pagemap.h | 2 +- mm/gup.c | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index bbccb4044222..0df3ffd72f9d 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -1094,7 +1094,7 @@ void folio_add_wait_queue(struct folio *folio, wait_queue_entry_t *waiter); */ size_t fault_in_writeable(char __user *uaddr, size_t size); size_t fault_in_subpage_writeable(char __user *uaddr, size_t size); -size_t fault_in_safe_writeable(const char __user *uaddr, size_t size); +size_t fault_in_safe_writeable(char __user *uaddr, size_t size); size_t fault_in_readable(const char __user *uaddr, size_t size); int add_to_page_cache_lru(struct page *page, struct address_space *mapping, diff --git a/mm/gup.c b/mm/gup.c index dc02749eaf9b..6b050b174470 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1841,13 +1841,17 @@ EXPORT_SYMBOL(fault_in_subpage_writeable);
- Returns the number of bytes not faulted in, like copy_to_user() and
- copy_from_user().
*/ -size_t fault_in_safe_writeable(const char __user *uaddr, size_t size) +size_t fault_in_safe_writeable(char __user *uaddr, size_t size) {
- /* TODO [PCuABI] - capability checks for uaccess */
- unsigned long start = user_ptr_addr(uaddr), end;
- unsigned long start, end; struct mm_struct *mm = current->mm; bool unlocked = false;
- if (!check_user_ptr_write(uaddr, size))
return 0;- start = user_ptr_addr(uaddr);
I think we can keep the initialisation where it was - it doesn't hurt to do it before we check uaddr.
Kevin
- if (unlikely(size == 0)) return 0; end = PAGE_ALIGN(start + size);