On 11/03/2024 10:28, Amit Daniel Kachhap wrote:
[...]
@@ -1259,9 +1267,11 @@ unsigned long do_mmap(struct file *file, unsigned long addr, /* Obtain the address to map to. we verify (or select) it and ensure * that it represents a valid section of the address space. */
- addr = get_unmapped_area(file, addr, len, pgoff, flags);
- if (IS_ERR_VALUE(addr))
return addr;
- if (new_reserv) {
addr = get_unmapped_area(file, addr, len, pgoff, flags);if (IS_ERR_VALUE(addr))return addr;- }
I see the rationale in calling get_unmapped_area() only if we need a new reservation, but to avoid duplicating code with what I am suggesting below (vm_mmap() taking unsigned long), I think it would be better to keep calling it unconditionally. In the MAP_FIXED case, get_unmapped_area() would be expected to check that the range is either wholly contained in an existing reservation, or that a new reservation could be created (see related comment on patch 6).
With this approach we shouldn't need to change do_mmap() at all, as all the reservation handling is done in mmap_region() and get_unmapped_area().
if (flags & MAP_FIXED_NOREPLACE) { if (find_vma_intersection(mm, addr, addr + len)) @@ -1383,15 +1393,19 @@ unsigned long do_mmap(struct file *file, unsigned long addr, vm_flags |= VM_NORESERVE; }
- addr = mmap_region(file, addr, len, vm_flags, pgoff, uf);
- if (new_reserv)
user_ptr = addr;- addr = mmap_region(file, user_ptr, len, vm_flags, pgoff, uf, prot, new_reserv); if (!IS_ERR_VALUE(addr) && ((vm_flags & VM_LOCKED) || (flags & (MAP_POPULATE | MAP_NONBLOCK)) == MAP_POPULATE)) *populate = len;
- return addr;
} -user_uintptr_t ksys_mmap_pgoff(user_uintptr_t addr, unsigned long len, +user_uintptr_t ksys_mmap_pgoff(user_uintptr_t user_ptr, unsigned long len,
I think this is a good time to (re)consider the typing of user pointers in all those syscalls and helpers. Does it really make sense to use user_uintptr_t? After all, we know for sure that this is a capability in PCuABI, unlike in ioctl() for instance. Switching to void __user * would remove the need for a lot of casting and give us stronger typing. We already extract the address explicitly by casting, so it is straightforward to make use of user_ptr_addr() instead. I don't immediately see any disadvantage with making that switch (the few extra changes are largely compensated by being able to remove most casts), but I I may be missing something.
unsigned long prot, unsigned long flags, unsigned long fd, unsigned long pgoff){ @@ -1429,7 +1443,7 @@ user_uintptr_t ksys_mmap_pgoff(user_uintptr_t addr, unsigned long len, return PTR_ERR(file); }
- retval = vm_mmap_pgoff(file, addr, len, prot, flags, pgoff);
- retval = vm_mmap_pgoff(file, user_ptr, len, prot, flags, pgoff);
out_fput: if (file) fput(file); @@ -2801,16 +2815,17 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len, unsigned long mmap_region(struct file *file, unsigned long addr, unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
struct list_head *uf)
struct list_head *uf, unsigned long prot, bool new_reserv){ struct mm_struct *mm = current->mm; struct vm_area_struct *vma = NULL;
- struct vm_area_struct *next, *prev, *merge;
- struct vm_area_struct *next, *prev, *merge, *old_vma; pgoff_t pglen = len >> PAGE_SHIFT; unsigned long charged = 0; unsigned long end = addr + len; unsigned long merge_start = addr, merge_end = end; bool writable_file_mapping = false;
- struct reserv_struct reserv_info; pgoff_t vm_pgoff; int error; VMA_ITERATOR(vmi, mm, addr);
@@ -2830,6 +2845,21 @@ unsigned long mmap_region(struct file *file, unsigned long addr, return -ENOMEM; }
- if (!new_reserv) {
With my proposed approach, we need to find out whether we are creating a reservation or not here. Since get_unmapped_area() has already checked that the new mapping is either fully inside a reservation or a new one can be created, we don't need to actually check if that's possible. We just need to find out which situation we are in here by looking up the vmas before *and* after addr, and checking if either reservation contains addr. If it does, then we're not creating a reservation, otherwise we are. (We need to check after addr as well, because it is possible that the reservation the next vma is in has a start address that is below vma->start).
Worth noting that we're not adding much overhead by doing it this way, since we need to find the existing reservation data anyway to copy it into the new vma.
old_vma = vma_find(&vmi, end);if (!old_vma)/** This error scenario may not occur as address with valid* capability should have been verified in the upstream* syscall functions.*/return -ENOMEM;+#ifdef CONFIG_CHERI_PURECAP_UABI
memcpy(&reserv_info, &old_vma->reserv_data, sizeof(reserv_info));
Nit: the struct can be copied with a regular assignment (reserv_info = old_vma->reserv_data).
+#endif
vma_iter_set(&vmi, addr);- }
[...]
@@ -903,6 +905,20 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len, return -ENOMEM; if (flags & MREMAP_FIXED) {
if (reserv_is_supported(mm)) {vma = find_vma(mm, new_addr);if (!vma)
The same logic as in mmap_region() is necessary here, otherwise we are assuming that MREMAP_FIXED targets an existing reservation, but like mmap() that's not necessarily the case (new_addr can null-derived).
/** This error scenario may not occur as address with valid* capability should have been verified in the upstream* syscall functions.*/return -ENOMEM;+#ifdef CONFIG_CHERI_PURECAP_UABI
memcpy(&reserv_info, &vma->reserv_data, sizeof(reserv_info));+#endif
reserv_ptr = &reserv_info;}@@ -948,7 +964,7 @@ static unsigned long mremap_to(unsigned long addr, unsigned long old_len, new_addr = ret; ret = move_vma(vma, addr, old_len, new_len, new_addr, locked, flags, uf,
uf_unmap);
uf_unmap, reserv_ptr);out: return ret; @@ -1169,7 +1185,7 @@ SYSCALL_DEFINE5(__retptr__(mremap), user_uintptr_t, addr, unsigned long, old_len } ret = move_vma(vma, addr, old_len, new_len, new_addr,
&locked, flags, &uf, &uf_unmap);
&locked, flags, &uf, &uf_unmap, NULL);out: if (offset_in_page(ret)) diff --git a/mm/util.c b/mm/util.c index afd40ed9c3c8..077c9a2592a9 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, +user_uintptr_t vm_mmap_pgoff(struct file *file, user_uintptr_t usrptr, unsigned long len, unsigned long prot, unsigned long flag, unsigned long pgoff) { @@ -553,11 +553,7 @@ 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*/ret = do_mmap(file, addr, len, prot, flag, 0, pgoff, &populate,
ret = do_mmap(file, usrptr, len, prot, flag, 0, pgoff, &populate, &uf);@@ -570,7 +566,8 @@ user_uintptr_t vm_mmap_pgoff(struct file *file, user_uintptr_t addr, return ret; } -unsigned long vm_mmap(struct file *file, unsigned long addr, +/* TODO [PCuABI] - Update the users of vm_mmap */ +user_uintptr_t vm_mmap(struct file *file, user_uintptr_t usrptr,
As discussed offline, I still think we're better off with all helpers (including vm_mmap_pgoff()) taking and returning unsigned long, with the exception of ksys_mmap_pgoff() as that's the syscall handler. It already does the capability checking, I think we could make it do all the capability-related operations, including creating a new capability if necessary.
With that approach, we do have the problem of deciding whether we are creating a new reservation or not. I think we can do this in mmap_region() by inspecting existing reservations, see comment above. In this way, we are completely separating the capability handling (ksys_mmap_pgoff()) from the reservation management (mmap_region(), get_unmapped_area()).
Kevin
unsigned long len, unsigned long prot, unsigned long flag, unsigned long offset) { @@ -579,7 +576,7 @@ unsigned long vm_mmap(struct file *file, unsigned long addr, if (unlikely(offset_in_page(offset))) return -EINVAL;
- return vm_mmap_pgoff(file, addr, len, prot, flag, offset >> PAGE_SHIFT);
- return vm_mmap_pgoff(file, usrptr, len, prot, flag, offset >> PAGE_SHIFT);
} EXPORT_SYMBOL(vm_mmap);