Re: [PATCH v3 stable/linux-6.12.y 3/3] 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] The patch erroneously applies the process-centric `PROCESS__EXECMEM` and `FILE__EXECMOD` permission checks to the backing file's mounter credentials instead of the executing process.

Pre-existing issues:
- [High] The code incorrectly uses `vma->vm_file` as a proxy for file-backed VMA classification, which violates VMA handling rules and triggers incorrect SELinux `FILE__EXECMOD` checks on anonymous `/dev/zero` mappings.
--

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

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

This commit updates the SELinux security model for overlayfs by applying
access controls for mmap() and mprotect(). It introduces a new backing
file LSM hook and uses the backing file API to enforce mprotect() access
controls properly for stacked filesystems.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8e31d3b60fc62..1b89c8d5fa2fc 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
[ ... ]
> @@ -3821,36 +3868,80 @@ static int selinux_mmap_addr(unsigned long addr)
[ ... ]
> +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, vma->vm_flags & VM_SHARED);

[Severity: High]
Does this apply the process-centric PROCESS__EXECMEM permission check
to the mounter's credentials instead of the executing process?

selinux_mmap_file_common() eventually calls __file_map_prot_check(), which
evaluates the PROCESS__EXECMEM permission against the credentials provided.
When passed the mounter's credentials here (backing_file->f_cred), it
seems to evaluate the mounter domain instead of the current task. This
could spuriously deny mapping W+X memory on overlayfs files, or allow an
unauthorized process to do so if the mounter has execmem.

>  }
>  
>  static int selinux_file_mprotect(struct vm_area_struct *vma,
>  				 unsigned long reqprot __always_unused,
>  				 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 (default_noexec &&
>  	    (prot & PROT_EXEC) && !(vma->vm_flags & VM_EXEC)) {
> -		int rc = 0;
>  		/*
>  		 * We don't use the vma_is_initial_heap() helper as it has
>  		 * a history of problems and is currently broken on systems
> @@ -3864,11 +3955,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 is a pre-existing issue, but does using !file correctly identify
anonymous VMAs?

According to the VMA guidelines, a VMA can have a valid vm_file but still
be anonymous, such as private mappings of /dev/zero. The canonical test for
an anonymous VMA is vma_is_anonymous(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) {

[Severity: High]
This is a pre-existing issue, but checking for a non-null file and anon_vma
as a proxy for file-backed VMA classification seems incorrect.

Private mappings of /dev/zero have a vm_file and set anon_vma after a COW
fault, which would cause this code to erroneously check FILE__EXECMOD on
them. Should this use !vma_is_anonymous(vma) instead to safely handle these
cases?

>  			/*
>  			 * We are making executable a file mapping that has
>  			 * had some COW done. Since pages might have been
> @@ -3876,13 +3971,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]
Does this apply the process-centric FILE__EXECMOD permission check to the
mounter's credentials? Checking file->f_cred evaluates if the mounter can
execute modified file memory, rather than checking the executing process.

> +				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);

[Severity: High]
Does this have the same issue of passing the mounter's credentials down to
the PROCESS__EXECMEM check?

By passing file->f_cred into file_map_prot_check(), the mounter is
evaluated for process-centric permissions instead of the current task.

> +		if (rc)
> +			return rc;
>  	}
>  
> -	return file_map_prot_check(vma->vm_file, prot, vma->vm_flags&VM_SHARED);
> +	return 0;
>  }

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