On 15/04/2024 15:21, Amit Daniel Kachhap wrote:
Use the recently introduced PCuABI reservation interfaces to add different parameter constraints for mmap/munmap syscall. The capability returned by mmap syscall is now bounded and is same as the reservation range. The in-kernel memory mapping vm_mmap() function do not check the constraints on parameters. These reservation checks added do not affect the compat64 code path.
Signed-off-by: Amit Daniel Kachhap amitdaniel.kachhap@arm.com
include/linux/mm.h | 4 ++++ mm/internal.h | 2 +- mm/mmap.c | 56 ++++++++++++++++++++++++++++++++++++++++++---- mm/util.c | 9 +------- 4 files changed, 58 insertions(+), 13 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h index 6d62e91676cb..137dbd27db55 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3411,6 +3411,10 @@ struct vm_unmapped_area_info { extern unsigned long vm_unmapped_area(struct vm_unmapped_area_info *info); +int check_pcuabi_params(user_uintptr_t user_ptr, unsigned long len,
unsigned long flags, bool enforce_cap_validity,bool enforce_range_mapped, bool reserv_lock);/* truncate.c */ extern void truncate_inode_pages(struct address_space *, loff_t); extern void truncate_inode_pages_range(struct address_space *, diff --git a/mm/internal.h b/mm/internal.h index 58df037c3824..3a88f1e2ffee 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -861,7 +861,7 @@ extern u64 hwpoison_filter_flags_value; extern u64 hwpoison_filter_memcg; extern u32 hwpoison_filter_enable; -extern user_uintptr_t __must_check vm_mmap_pgoff(struct file *, user_uintptr_t, +extern unsigned long __must_check vm_mmap_pgoff(struct file *, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long); diff --git a/mm/mmap.c b/mm/mmap.c index 84e26bb7b203..cb069b76d761 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1392,12 +1392,40 @@ unsigned long do_mmap(struct file *file, unsigned long addr, return addr; } -user_uintptr_t ksys_mmap_pgoff(user_uintptr_t addr, unsigned long len, +int check_pcuabi_params(user_uintptr_t user_ptr, unsigned long len,
unsigned long flags, bool enforce_cap_validity,bool enforce_range_mapped, bool reserv_lock)
The combination of enforce_cap_validity + enforce_range_mapped is pretty hard to read, especially as the function is directly called with true/false as parameters.
In practice, I think only enforce_cap_validity == false && enforce_range_mapped == true is useful. That corresponds to the standard mmap() case, as well as the new_addr case in mremap(), and shmat() (which is equivalent to mmap()). The old_addr case in mremap() corresponds in fact to the same checks as standard syscalls like munmap(), so it shouldn't be handled here.
+{
- ptraddr_t addr = (ptraddr_t)user_ptr;
- int ret = -EINVAL;
- VMA_ITERATOR(vmi, current->mm, addr);
- if (!reserv_is_supported(current->mm))
return 0;- if (!check_user_ptr_owning(user_ptr, addr, len)) {
if (enforce_cap_validity || !user_ptr_is_same((const void __user *)user_ptr,(const void __user *)(user_uintptr_t)addr))return ret;return 0;- }
- if (!reserv_vmi_cap_within_reserv(&vmi, user_ptr, reserv_lock))
return -ERESERVATION;- if (!(flags & MREMAP_FIXED || flags & MAP_FIXED))
We cannot do this. MREMAP_FIXED is the same value as MAP_PRIVATE. The caller should check its own flags, we could pass a boolean instead.
return ret;- if (enforce_range_mapped && !reserv_vmi_range_mapped(&vmi, addr, len, reserv_lock))
return -ENOMEM;- return 0;
+}
+user_uintptr_t ksys_mmap_pgoff(user_uintptr_t user_ptr, unsigned long len, unsigned long prot, unsigned long flags, unsigned long fd, unsigned long pgoff) { struct file *file = NULL;
- user_uintptr_t retval;
- user_uintptr_t retval = -EINVAL;
- ptraddr_t addr = (ptraddr_t)user_ptr;
- bool new_reserv = true;
if (!(flags & MAP_ANONYMOUS)) { audit_mmap_fd(fd, flags); @@ -1430,7 +1458,21 @@ user_uintptr_t ksys_mmap_pgoff(user_uintptr_t addr, unsigned long len, return PTR_ERR(file); }
- retval = check_pcuabi_params(user_ptr, len, flags, false, true, false);
- if (retval)
goto out_fput;- if (user_ptr_is_valid((const void __user *)user_ptr))
new_reserv = true;
new_reserv = false surely?
- retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
- if (!IS_ERR_VALUE(retval) && reserv_is_supported(current->mm)) {
if (new_reserv)retval = make_user_ptr_owning(retval, len,user_ptr_owning_perms_from_prot(prot,(flags & MAP_SHARED) ? false : true));elseretval = (user_uintptr_t)user_ptr_set_addr((void __user *)user_ptr, retval);
There's nothing to do in this case, the return value is exactly user_ptr as per the spec (in other words this operation should be a no-op).
Kevin
- }
out_fput: if (file) fput(file); @@ -3097,9 +3139,15 @@ int vm_munmap(unsigned long start, size_t len) } EXPORT_SYMBOL(vm_munmap); -SYSCALL_DEFINE2(munmap, user_uintptr_t, addr, size_t, len) +SYSCALL_DEFINE2(munmap, user_uintptr_t, user_ptr, size_t, len) {
- addr = untagged_addr(addr);
- ptraddr_t addr = untagged_addr((ptraddr_t)user_ptr);
- VMA_ITERATOR(vmi, current->mm, addr);
- if (reserv_is_supported(current->mm) && !check_user_ptr_owning(user_ptr, addr, len))
return -EINVAL;- if (!reserv_vmi_cap_within_reserv(&vmi, user_ptr, false))
return -ERESERVATION;} diff --git a/mm/util.c b/mm/util.c index afd40ed9c3c8..bd69a417c6a9 100644 --- a/mm/util.c +++ b/mm/util.c @@ -540,7 +540,7 @@ int account_locked_vm(struct mm_struct *mm, unsigned long pages, bool inc) } EXPORT_SYMBOL_GPL(account_locked_vm); -user_uintptr_t vm_mmap_pgoff(struct file *file, user_uintptr_t addr, +unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr, unsigned long len, unsigned long prot, unsigned long flag, unsigned long pgoff) { @@ -553,19 +553,12 @@ user_uintptr_t vm_mmap_pgoff(struct file *file, user_uintptr_t addr, if (!ret) { if (mmap_write_lock_killable(mm)) return -EINTR;
/** TODO [PCuABI] - might need propagating uintcap further down* to do_mmap to properly handle capabilities*//* TODO [PCuABI] - derive proper capability */if (!IS_ERR_VALUE(ret))ret = (user_uintptr_t)uaddr_to_user_ptr_safe((ptraddr_t)ret);}