Re: [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 2 potential issue(s) to consider:

New issues:
- [High] Incorrect SELinux PROCESS__EXECMEM capability check against the mounter's credentials for backing files.

Pre-existing issues:
- [High] Use of `!file` to classify anonymous VMAs bypasses the SELinux PROCESS__EXECSTACK security check for /dev/zero mappings.
--

commit fc1612e3086f71b6727cf2b1788b9dcb2edf19fb
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).

> 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]
Does this code inadvertently check the mounter's credentials for the
PROCESS__EXECMEM capability when handling backing files?

In __file_map_prot_check(), the EXECMEM permission is verified using the
subject ID derived from the 'cred' argument. When this is called for backing
files from selinux_file_mprotect() or selinux_mmap_backing_file(), the
backing_file->f_cred (the mounter's credentials) is passed instead of the
calling process's credentials.

Because EXECMEM restricts process memory creation, shouldn't this check
apply to the current user process rather than the mounter's domain? This
might cause legitimate memory mappings to be falsely rejected if the mounter
lacks this permission.

> +		if (rc)
> +			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))) {

[Severity: High]
This isn't a bug introduced by this patch, but does checking '!file'
accurately identify anonymous VMAs for the PROCESS__EXECSTACK check?

According to the MM subsystem guidelines, evaluating 'vma->vm_file == NULL'
is incorrect for classifying a VMA as anonymous, because private mappings of
/dev/zero have a valid vm_file but remain anonymous mappings without vm_ops.

If an attacker uses a private mapping of /dev/zero as a stack, '!file'
evaluates to false, bypassing the PROCESS__EXECSTACK mitigation and falling
through to the FILE__EXECMOD check instead. Could this be corrected to use
vma_is_anonymous(vma) as the discriminator?

>  			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) {

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