Re: [PATCH 5.15.y] mm: call the security_mmap_file() LSM hook in remap_file_pages()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: lock inversion in remap_file_pages() calling security_mmap_file()
--

commit 78749fa17008059030caa915f2db592fbe2d9fa2
Author: Shu Han <ebpqwerty472123@gmail.com>

mm: call the security_mmap_file() LSM hook in remap_file_pages()

This commit adds a call to security_mmap_file() in remap_file_pages() before
invoking do_mmap() so that LSM policies such as SELinux W^X checks cannot be
bypassed when READ_IMPLIES_EXEC is set in the process personality.

Link: https://project-zero.issues.chromium.org/issues/42452389 [1]

> diff --git a/mm/mmap.c b/mm/mmap.c
> index 273e06dad3c6f..7f357de8f4d68 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -3047,8 +3047,12 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
>  		flags |= MAP_LOCKED;
>  
>  	file = get_file(vma->vm_file);
> +	ret = security_mmap_file(vma->vm_file, prot, flags);
> +	if (ret)
> +		goto out_fput;

[Severity: High]
Does calling security_mmap_file() here while holding mm->mmap_lock introduce a
lock inversion with inode->i_rwsem?

In mm/mmap.c, SYSCALL_DEFINE5(remap_file_pages) acquires mm->mmap_lock for
write before looking up the VMA and holds it across this call:

mm/mmap.c:SYSCALL_DEFINE5(remap_file_pages) {
	if (mmap_write_lock_killable(mm))
		return -EINTR;
	...
	file = get_file(vma->vm_file);
	ret = security_mmap_file(vma->vm_file, prot, flags);
}

However, security_mmap_file() in security/security.c computes the adjusted
protection flags (which includes PROT_EXEC when VM_EXEC is present or when
VM_READ is present with READ_IMPLIES_EXEC set in current->personality) and
calls ima_file_mmap() -> process_measurement() in
security/integrity/ima/ima_main.c:

security/security.c:security_mmap_file()
  -> security/integrity/ima/ima_main.c:ima_file_mmap()
    -> security/integrity/ima/ima_main.c:process_measurement() {
	...
	inode_lock(inode);
    }

When an IMA measurement or appraisal policy applies to the file,
process_measurement() acquires inode->i_rwsem (as well as iint->mutex) while
mm->mmap_lock is already held.

As documented in the lock ordering comment in mm/rmap.c, inode->i_rwsem ranks
outside mm->mmap_lock (for example, generic_file_write_iter() ->
generic_perform_write() in mm/filemap.c holds inode_lock(inode) while faulting
in user pages under mm->mmap_lock). Other callers such as vm_mmap_pgoff() in
mm/util.c and do_shmat() in ipc/shm.c invoke security_mmap_file() outside
mm->mmap_lock for this reason (see commit 8b3ec6814c83).

Can a concurrent buffered write or truncate on the same file deadlock against
remap_file_pages() when IMA is enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028080719.1821-1-yao.zhang1@amlogic.com?part=1