Re: [PATCH v2 stable/linux-6.18.y 2/2] selinux: fix overlayfs mmap() and mprotect() access checks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PROCESS__EXECMEM permission is incorrectly checked against the mounter's credentials instead of the executing process.
--

commit ff9b809022a43e85182f65f4fb0c497928b26b8d
Author: Paul Moore <paul@paul-moore.com>

selinux: fix overlayfs mmap() and mprotect() access checks

The existing SELinux security model for overlayfs is to allow access if
the current task is able to access the top level file (the "user" file)
and the mounter's credentials are sufficient to access the lower
level file (the "backing" file). This patch makes use of the newly created
security_mmap_backing_file() LSM hook and the backing file API to provide
backing file enforcement for mmap() and mprotect().

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3da3017ad2ca0..f96ee8f372e3b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c

[ ... ]

> @@ -3916,42 +3950,55 @@ 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(struct file *file, unsigned long prot, int shared)
> +static int __file_map_prot_check(const struct cred *cred,
> +				 const struct file *file, unsigned long prot,
> +				 bool shared, bool bf_user_file)
>  {
> -	const struct cred *cred = current_cred();
> -	u32 sid = cred_sid(cred);
> -	int rc = 0;
> +	struct inode *inode = NULL;
> +	bool prot_exec = prot & PROT_EXEC;
> +	bool prot_write = prot & PROT_WRITE;
> +
> +	if (file) {
> +		if (bf_user_file)
> +			inode = d_inode(backing_file_user_path(file)->dentry);
> +		else
> +			inode = file_inode(file);
> +	}
> +
> +	if (default_noexec && prot_exec &&
> +	    (!file || IS_PRIVATE(inode) || (!shared && prot_write))) {
> +		int rc;
> +		u32 sid = cred_sid(cred);
>  
> -	if (default_noexec &&
> -	    (prot & PROT_EXEC) && (!file || IS_PRIVATE(file_inode(file)) ||
> -				   (!shared && (prot & PROT_WRITE)))) {
>  		/*
> -		 * We are making executable an anonymous mapping or a
> -		 * private file mapping that will also be writable.
> -		 * This has an additional check.
> +		 * We are making executable an anonymous mapping or a private
> +		 * file mapping that will also be writable.
>  		 */
> -		rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
> -				  PROCESS__EXECMEM, NULL);
> +		rc = avc_has_perm(sid, sid, SECCLASS_PROCESS, PROCESS__EXECMEM,
> +				  NULL);

[Severity: High]
Will this check PROCESS__EXECMEM against the mounter's credentials instead of
the executing process?

When called for a backing file in selinux_mmap_backing_file() or
selinux_file_mprotect(), the cred passed in is file->f_cred (the mounter's
credentials). Since PROCESS__EXECMEM is a property of the executing process,
could checking it against the mounter cause spurious SELinux denials?

For example, could this break legitimate unprivileged processes (like JIT
compilers) attempting to create W+X private mappings on overlayfs, simply
because the mount process lacks execmem privileges?

> +		if (rc)
> -			goto error;
> +			return rc;
>  	}

[ ... ]

> @@ -4010,11 +4101,15 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
>  		    vma->vm_end <= vma->vm_mm->brk) {
>  			rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
>  					  PROCESS__EXECHEAP, NULL);
> -		} else if (!vma->vm_file && (vma_is_initial_stack(vma) ||
> +			if (rc)
> +				return rc;
> +		} else if (!file && (vma_is_initial_stack(vma) ||
>  			    vma_is_stack_for_current(vma))) {
>  			rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
>  					  PROCESS__EXECSTACK, NULL);
> -		} else if (vma->vm_file && vma->anon_vma) {
> +			if (rc)
> +				return rc;
> +		} else if (file && vma->anon_vma) {
>  			/*
>  			 * We are making executable a file mapping that has
>  			 * had some COW done. Since pages might have been
> @@ -4022,13 +4117,29 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
>  			 * modified content.  This typically should only
>  			 * occur for text relocations.
>  			 */
> -			rc = file_has_perm(cred, vma->vm_file, FILE__EXECMOD);
> +			rc = __file_has_perm(cred, file, FILE__EXECMOD,
> +					     backing_file);
> +			if (rc)
> +				return rc;
> +			if (backing_file) {
> +				rc = file_has_perm(file->f_cred, file,
> +						   FILE__EXECMOD);
> +				if (rc)
> +					return rc;
> +			}
>  		}
> +	}
> +
> +	rc = __file_map_prot_check(cred, file, prot, shared, backing_file);
> +	if (rc)
> +		return rc;
> +	if (backing_file) {
> +		rc = file_map_prot_check(file->f_cred, file, prot, shared);
>  		if (rc)
>  			return rc;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626075035.143419-1-caixinchen1@huawei.com?part=2