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 5a07ec5e983287a566c335be52d6b9bab2cb5e44
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(). This prevents processes with the READ_IMPLIES_EXEC
personality set from remapping writable file pages as executable and
bypassing SELinux W^X restrictions.

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]
Could calling security_mmap_file() here while holding mm->mmap_lock for
write introduce a lock ordering inversion against inode->i_rwsem?

In SYSCALL_DEFINE5(remap_file_pages) in mm/mmap.c, mm->mmap_lock is acquired
exclusively before reaching 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);
}

When remap_file_pages() is called on a file mapping where the VMA is
executable, or where the VMA is readable and the task has READ_IMPLIES_EXEC
set in current->personality, security_mmap_file() in security/security.c
computes prot_adj with PROT_EXEC set and calls ima_file_mmap():

security_mmap_file()
  -> ima_file_mmap()
    -> process_measurement()

In security/integrity/ima/ima_main.c, when an IMA policy applies to the
regular file, process_measurement() acquires inode_lock(inode)
(inode->i_rwsem) as well as iint->mutex:

security/integrity/ima/ima_main.c:process_measurement() {
	...
	inode_lock(inode);

	if (action) {
		iint = integrity_inode_get(inode);
		if (!iint)
			rc = -ENOMEM;
	}
}

However, the lock ordering documented in mm/rmap.c requires inode->i_rwsem to
nest outside mm->mmap_lock:

 * Lock ordering in mm:
 *
 * inode->i_rwsem	(while writing or truncating, not reading or faulting)
 *   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() before acquiring
mmap_write_lock_killable(mm). Can calling security_mmap_file() under
mmap_write_lock() cause a deadlock against concurrent buffered writes or
truncates on the same file that take inode_lock(inode) before faulting pages
under mm->mmap_lock?

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