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

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

[yao zhang1]

From: Shu Han <ebpqwerty472123@gmail.com>

The remap_file_pages syscall handler calls do_mmap() directly, which
doesn't contain the LSM security check. And if the process has called
personality(READ_IMPLIES_EXEC) before and remap_file_pages() is called for
RW pages, this will actually result in remapping the pages to RWX,
bypassing a W^X policy enforced by SELinux.

So we should check prot by security_mmap_file LSM hook in the
remap_file_pages syscall handler before do_mmap() is called. Otherwise, it
potentially permits an attacker to bypass a W^X policy enforced by
SELinux.

The bypass is similar to CVE-2016-10044, which bypass the same thing via
AIO and can be found in [1].

The PoC:

$ cat > test.c

int main(void) {
	size_t pagesz = sysconf(_SC_PAGE_SIZE);
	int mfd = syscall(SYS_memfd_create, "test", 0);
	const char *buf = mmap(NULL, 4 * pagesz, PROT_READ | PROT_WRITE,
		MAP_SHARED, mfd, 0);
	unsigned int old = syscall(SYS_personality, 0xffffffff);
	syscall(SYS_personality, READ_IMPLIES_EXEC | old);
	syscall(SYS_remap_file_pages, buf, pagesz, 0, 2, 0);
	syscall(SYS_personality, old);
	// show the RWX page exists even if W^X policy is enforced
	int fd = open("/proc/self/maps", O_RDONLY);
	unsigned char buf2[1024];
	while (1) {
		int ret = read(fd, buf2, 1024);
		if (ret <= 0) break;
		write(1, buf2, ret);
	}
	close(fd);
}

$ gcc test.c -o test
$ ./test | grep rwx
7f1836c34000-7f1836c35000 rwxs 00002000 00:01 2050 /memfd:test (deleted)

Link: https://project-zero.issues.chromium.org/issues/42452389 [1]
Cc: stable@vger.kernel.org
Signed-off-by: Shu Han <ebpqwerty472123@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
[PM: subject line tweaks]
Signed-off-by: Paul Moore <paul@paul-moore.com>
(cherry picked from commit ea7e2d5e49c05e5db1922387b09ca74aa40f46e2)
Signed-off-by: yao zhang1 <yao.zhang1@amlogic.com>
---
 mm/mmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/mmap.c b/mm/mmap.c
index 273e06dad3c6..7f357de8f4d6 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;
 	ret = do_mmap(vma->vm_file, start, size,
 			prot, flags, pgoff, &populate, NULL);
+out_fput:
 	fput(file);
 out:
 	mmap_write_unlock(mm);
-- 
2.46.0

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

[yao zhang1]

From: Shu Han <ebpqwerty472123@gmail.com>

commit ea7e2d5e49c05e5db1922387b09ca74aa40f46e2 upstream.

The remap_file_pages syscall handler calls do_mmap() directly, which
doesn't contain the LSM security check. And if the process has called
personality(READ_IMPLIES_EXEC) before and remap_file_pages() is called for
RW pages, this will actually result in remapping the pages to RWX,
bypassing a W^X policy enforced by SELinux.

So we should check prot by security_mmap_file LSM hook in the
remap_file_pages syscall handler before do_mmap() is called. Otherwise, it
potentially permits an attacker to bypass a W^X policy enforced by
SELinux.

The bypass is similar to CVE-2016-10044, which bypass the same thing via
AIO and can be found in [1].

The PoC:

$ cat > test.c

int main(void) {
	size_t pagesz = sysconf(_SC_PAGE_SIZE);
	int mfd = syscall(SYS_memfd_create, "test", 0);
	const char *buf = mmap(NULL, 4 * pagesz, PROT_READ | PROT_WRITE,
		MAP_SHARED, mfd, 0);
	unsigned int old = syscall(SYS_personality, 0xffffffff);
	syscall(SYS_personality, READ_IMPLIES_EXEC | old);
	syscall(SYS_remap_file_pages, buf, pagesz, 0, 2, 0);
	syscall(SYS_personality, old);
	// show the RWX page exists even if W^X policy is enforced
	int fd = open("/proc/self/maps", O_RDONLY);
	unsigned char buf2[1024];
	while (1) {
		int ret = read(fd, buf2, 1024);
		if (ret <= 0) break;
		write(1, buf2, ret);
	}
	close(fd);
}

$ gcc test.c -o test
$ ./test | grep rwx
7f1836c34000-7f1836c35000 rwxs 00002000 00:01 2050 /memfd:test (deleted)

Link: https://project-zero.issues.chromium.org/issues/42452389 [1]
Cc: stable@vger.kernel.org
Signed-off-by: Shu Han <ebpqwerty472123@gmail.com>
Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com>
[PM: subject line tweaks]
Signed-off-by: Paul Moore <paul@paul-moore.com>
(cherry picked from commit ea7e2d5e49c05e5db1922387b09ca74aa40f46e2)
Signed-off-by: yao zhang1 <yao.zhang1@amlogic.com>
---
 mm/mmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mm/mmap.c b/mm/mmap.c
index 273e06dad3c6..7f357de8f4d6 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;
 	ret = do_mmap(vma->vm_file, start, size,
 			prot, flags, pgoff, &populate, NULL);
+out_fput:
 	fput(file);
 out:
 	mmap_write_unlock(mm);
-- 
2.46.0

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

[sashiko-bot]

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

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

[sashiko-bot]

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