Re: [PATCH v2 stable/linux-6.6.y 3/3] 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] Spurious SELinux denial due to checking PROCESS__EXECMEM against mounter's credentials
--

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

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

This commit introduces SELinux backing file enforcement for mmap() and
mprotect() on overlayfs filesystems. It uses the new backing file API and
LSM blobs to enforce access controls against both the user file and the
backing file.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 60092d0b013ce..3f11c5ae8fbf0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
[ ... ]
> @@ -3789,17 +3789,28 @@ 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)
>  {
[ ... ]
> +	if (default_noexec && prot_exec &&
> +	    (!file || IS_PRIVATE(inode) || (!shared && prot_write))) {
> +		int rc;
> +		u32 sid = cred_sid(cred);
>  
> -		rc = avc_has_perm(sid, sid, SECCLASS_PROCESS,
> -				  PROCESS__EXECMEM, NULL);
> +		/*
> +		 * 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);
> +		if (rc)
> +			return rc;

[Severity: High]
When checking PROCESS__EXECMEM, does this inadvertently use the mounter's
credential instead of the current task's credential when called for a
backing file?

Since cred_sid(cred) relies on the passed-in cred argument, if the caller
passes the backing file's credentials, we will check if the mounter has
PROCESS__EXECMEM. Could this lead to spurious SELinux denials for a user
process trying to create a private writable executable mapping (W^X) on
overlayfs, simply because the mounter lacks the execmem permission?

[ ... ]
> @@ -3806,36 +3853,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 passing backing_file->f_cred here trigger the PROCESS__EXECMEM check
against the mounter rather than the current process during an mmap() system
call?

[ ... ]
> @@ -3849,11 +3940,15 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
[ ... ]
> +	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;
>  	}
>  
> -	return file_map_prot_check(vma->vm_file, prot, vma->vm_flags&VM_SHARED);
> +	return 0;
>  }

[Severity: High]
Similarly, does passing file->f_cred to file_map_prot_check() cause the
same issue for mprotect(), checking PROCESS__EXECMEM against the mounter's
credentials?

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