[PATCH] selinux: fix incorrect execmem checks on overlayfs

[PATCH] selinux: fix incorrect execmem checks on overlayfs

[Ondrej Mosnacek]

The commit fixing the overlayfs mmap() and mprotect() access checks
failed to skip the execmem check in __file_map_prot_check() for the case
where the "mounter check" is being performed. This check should be
performed only against the credentials of the task that is calling
mmap()/mprotect(), since it doesn't pertain to the file itself, but
rather just gates the ability of the calling task to get an executable
memory mapping in general.

The purpose of the "mounter check" is to guard against using an
overlayfs mount to gain file access that would otherwise be denied to
the mounter. For execmem this is not relevant, as there is no further
file access granted based on it (notice that the file's context is not
used as the target in the check), so checking it also against the
mounter credentials would be incorrect.

Fix this by passing a boolean to [__]file_map_prot_check() and
selinux_mmap_file_common() that indicates if we are doing the "mounter
check" and skiping the execmem check in that case. Since this boolean
also indicates if we use current_cred() or the mounter cred as the
subject, also remove the "cred" argument from these functions and
determine it based on the boolean and the file struct.

Fixes: 82544d36b172 ("selinux: fix overlayfs mmap() and mprotect() access checks")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1a713d96206f5..87790f6f0d4de 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3969,13 +3969,14 @@ static int selinux_file_ioctl_compat(struct file *file, unsigned int cmd,
 
 static int default_noexec __ro_after_init;
 
-static int __file_map_prot_check(const struct cred *cred,
-				 const struct file *file, unsigned long prot,
-				 bool shared, bool bf_user_file)
+static int __file_map_prot_check(const struct file *file, unsigned long prot,
+				 bool shared, bool mounter_check,
+				 bool bf_user_file)
 {
 	struct inode *inode = NULL;
 	bool prot_exec = prot & PROT_EXEC;
 	bool prot_write = prot & PROT_WRITE;
+	const struct cred *cred = mounter_check ? file->f_cred : current_cred();
 
 	if (file) {
 		if (bf_user_file)
@@ -3984,7 +3985,7 @@ static int __file_map_prot_check(const struct cred *cred,
 			inode = file_inode(file);
 	}
 
-	if (default_noexec && prot_exec &&
+	if (!mounter_check && default_noexec && prot_exec &&
 	    (!file || IS_PRIVATE(inode) || (!shared && prot_write))) {
 		int rc;
 		u32 sid = cred_sid(cred);
@@ -4013,11 +4014,11 @@ static int __file_map_prot_check(const struct cred *cred,
 	return 0;
 }
 
-static inline int file_map_prot_check(const struct cred *cred,
-				      const struct file *file,
-				      unsigned long prot, bool shared)
+static inline int file_map_prot_check(const struct file *file,
+				      unsigned long prot, bool shared,
+				      bool mounter_check)
 {
-	return __file_map_prot_check(cred, file, prot, shared, false);
+	return __file_map_prot_check(file, prot, shared, mounter_check, false);
 }
 
 static int selinux_mmap_addr(unsigned long addr)
@@ -4033,9 +4034,11 @@ static int selinux_mmap_addr(unsigned long addr)
 	return rc;
 }
 
-static int selinux_mmap_file_common(const struct cred *cred, struct file *file,
-				    unsigned long prot, bool shared)
+static int selinux_mmap_file_common(struct file *file, unsigned long prot,
+				    bool shared, bool mounter_check)
 {
+	const struct cred *cred = mounter_check ? file->f_cred : current_cred();
+
 	if (file) {
 		int rc;
 		struct common_audit_data ad;
@@ -4047,15 +4050,16 @@ static int selinux_mmap_file_common(const struct cred *cred, struct file *file,
 			return rc;
 	}
 
-	return file_map_prot_check(cred, file, prot, shared);
+	return file_map_prot_check(file, prot, shared, mounter_check);
 }
 
 static int selinux_mmap_file(struct file *file,
 			     unsigned long reqprot __always_unused,
 			     unsigned long prot, unsigned long flags)
 {
-	return selinux_mmap_file_common(current_cred(), file, prot,
-					(flags & MAP_TYPE) == MAP_SHARED);
+	return selinux_mmap_file_common(file, prot,
+					(flags & MAP_TYPE) == MAP_SHARED,
+					false);
 }
 
 /**
@@ -4087,8 +4091,9 @@ static int selinux_mmap_backing_file(struct vm_area_struct *vma,
 	if (vma->vm_flags & VM_EXEC)
 		prot |= PROT_EXEC;
 
-	return selinux_mmap_file_common(backing_file->f_cred, backing_file,
-					prot, vma->vm_flags & VM_SHARED);
+	return selinux_mmap_file_common(backing_file, prot,
+					vma->vm_flags & VM_SHARED,
+					true);
 }
 
 static int selinux_file_mprotect(struct vm_area_struct *vma,
@@ -4149,11 +4154,11 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
 		}
 	}
 
-	rc = __file_map_prot_check(cred, file, prot, shared, backing_file);
+	rc = __file_map_prot_check(file, prot, shared, false, backing_file);
 	if (rc)
 		return rc;
 	if (backing_file) {
-		rc = file_map_prot_check(file->f_cred, file, prot, shared);
+		rc = file_map_prot_check(file, prot, shared, true);
 		if (rc)
 			return rc;
 	}
-- 
2.55.0

Re: [PATCH] selinux: fix incorrect execmem checks on overlayfs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] SELinux uses `vma->vm_file` instead of `vma_is_anonymous()` to classify anonymous VMAs, leading to incorrect permission checks.
--

commit 2446671db8e6a83fbf4c3451f231d685bec26e4e
Author: Ondrej Mosnacek <omosnace@redhat.com>

selinux: fix incorrect execmem checks on overlayfs

This commit fixes overlayfs mmap() and mprotect() access checks by skipping
the execmem check in __file_map_prot_check() during a "mounter check". It
introduces a boolean to indicate this state, ensuring the check is only
performed against the calling task's credentials.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0f704380a8c81..fca310b7270f3 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
[ ... ]
> @@ -3988,7 +3989,7 @@ static int __file_map_prot_check(const struct cred *cred,
>  			inode = file_inode(file);
>  	}
>  
> -	if (default_noexec && prot_exec &&
> +	if (!mounter_check && default_noexec && prot_exec &&
>  	    (!file || IS_PRIVATE(inode) || (!shared && prot_write))) {

[Severity: High]
This isn't a bug introduced by this patch, but is it correct to rely on
!file to identify anonymous mappings here?

Using a file pointer to check for anonymous VMAs instead of
vma_is_anonymous() can misclassify mappings. A private mapping of /dev/zero
has vma->vm_file != NULL but is functionally an anonymous mapping. This could
allow an anonymous mapping to bypass the PROCESS__EXECMEM check.

This same pre-existing pattern appears in selinux_file_mprotect(), which uses
!file to check for anonymous stack mappings:

security/selinux/hooks.c:selinux_file_mprotect() {
    ...
    } else if (!file && (vma_is_initial_stack(vma) ||
                vma_is_stack_for_current(vma))) {
    ...
}

It also applies the FILE__EXECMOD check to the backing file if vma->anon_vma
is set, which would incorrectly check the backing device for /dev/zero:

security/selinux/hooks.c:selinux_file_mprotect() {
    ...
    } else if (file && vma->anon_vma) {
        /* ... */
        rc = __file_has_perm(cred, file, FILE__EXECMOD,
                             backing_file);
    ...
}

Could this allow SELinux policies to be bypassed for driver mappings that call
vma_set_anonymous() by incorrectly treating them as file-backed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703114733.1243995-1-omosnace@redhat.com?part=1