Re: [PATCH stable/linux-5.10.y 7/7] 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] SELinux inappropriately evaluates process-specific execution permissions (PROCESS__EXECMEM and FILE__EXECMOD) against the mounter's credentials for overlayfs backing files.

Pre-existing issues:
- [High] SELinux uses `!file` as a proxy to classify anonymous vs file-backed VMAs, causing incorrect MAC checks for anonymous `/dev/zero` mappings.
--

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

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

This patch implements backing file enforcement for mmap() and mprotect()
operations on overlayfs filesystems. It uses the new LSM hook and backing
file API to ensure correct access controls.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e1bbdef0bcd39..76cc0cbd1fd2b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c

[ ... ]

> @@ -3762,36 +3807,86 @@ static int selinux_mmap_file(struct file *file, unsigned long reqprot,
>  	if (checkreqprot_get(&selinux_state))
>  		prot = reqprot;
>  
> -	return file_map_prot_check(file, prot,
> -				   (flags & MAP_TYPE) == MAP_SHARED);
> +	return file_map_prot_check(cred, file, prot, shared);
> +}
> +
> +/**
> + * selinux_mmap_backing_file - Check mmap permissions on a backing file
> + * @vma: memory region
> + * @backing_file: stacked filesystem backing file
> + * @user_file: user visible file
> + *
> + * This is called after selinux_mmap_file() on stacked filesystems, and it
> + * is this function's responsibility to verify access to @backing_file and
> + * setup the SELinux state for possible later use in the mprotect() code path.
> + *
> + * By the time this function is called, mmap() access to @user_file has already
> + * been authorized and @vma->vm_file has been set to point to @backing_file.
> + *
> + * Return zero on success, negative values otherwise.
> + */
> +static int selinux_mmap_backing_file(struct vm_area_struct *vma,
> +				     struct file *backing_file,
> +				     struct file *user_file __always_unused)
> +{
> +	unsigned long prot = 0;
> +
> +	/* translate vma->vm_flags perms into PROT perms */
> +	if (vma->vm_flags & VM_READ)
> +		prot |= PROT_READ;
> +	if (vma->vm_flags & VM_WRITE)
> +		prot |= PROT_WRITE;
> +	if (vma->vm_flags & VM_EXEC)
> +		prot |= PROT_EXEC;
> +
> +	return selinux_mmap_file_common(backing_file->f_cred, backing_file,
> +					prot, prot, vma->vm_flags & VM_SHARED);
>  }

[Severity: High]
When selinux_mmap_backing_file() calls selinux_mmap_file_common(), it passes
backing_file->f_cred. This cred propagates down to __file_map_prot_check(),
where it is used to evaluate PROCESS__EXECMEM.

Because this evaluates PROCESS__EXECMEM against the mounter's credentials
rather than the current task mapping the memory, could this result in
spurious EACCES denials if the mounter's policy lacks the EXECMEM privilege?

>  static int selinux_file_mprotect(struct vm_area_struct *vma,
>  				 unsigned long reqprot,
>  				 unsigned long prot)
>  {
> +	int rc;
>  	const struct cred *cred = current_cred();
>  	u32 sid = cred_sid(cred);
> +	const struct file *file = vma->vm_file;
> +	bool backing_file;
> +	bool shared = vma->vm_flags & VM_SHARED;
> +
> +	/* check if we need to trigger the "backing files are awful" mode */
> +	backing_file = file && (file->f_mode & FMODE_BACKING);
>  
>  	if (checkreqprot_get(&selinux_state))
>  		prot = reqprot;
>  
>  	if (default_noexec &&
>  	    (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
> -		int rc = 0;
>  		if (vma->vm_start >= vma->vm_mm->start_brk &&
>  		    vma->vm_end <= vma->vm_mm->brk) {
>  			rc = avc_has_perm(&selinux_state,
>  					  sid, sid, SECCLASS_PROCESS,
>  					  PROCESS__EXECHEAP, NULL);
> -		} else if (!vma->vm_file &&
> +			if (rc)
> +				return rc;
> +		} else if (!file &&
>  			   ((vma->vm_start <= vma->vm_mm->start_stack &&
>  			     vma->vm_end >= vma->vm_mm->start_stack) ||
>  			    vma_is_stack_for_current(vma))) {

[Severity: High]
This is a pre-existing issue, but using !file (which evaluates to
vma->vm_file == NULL) as a proxy for an anonymous VMA misses some anonymous
mappings.

Private mappings of /dev/zero are anonymous but have a non-NULL vma->vm_file.
Can this bypass the PROCESS__EXECSTACK permission check for /dev/zero
executable stacks by incorrectly falling through to the FILE__EXECMOD check?

Should this check use vma_is_anonymous(vma) rather than !file to ensure
all anonymous mappings are correctly evaluated?

>  			rc = avc_has_perm(&selinux_state,
>  					  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
> @@ -3799,13 +3894,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);

[Severity: High]
Similar to the PROCESS__EXECMEM check in the mmap path, this evaluates
FILE__EXECMOD using the backing file's credentials (file->f_cred).

Is it intended to check EXECMOD against the mounter's credentials here,
or should this process-specific execution permission be evaluated against
the current process modifying the memory?

> +				if (rc)
> +					return rc;
> +			}
>  		}
> +	}
> +
> +	rc = __file_map_prot_check(cred, file, prot, shared, backing_file);

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