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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The PROCESS__EXECMEM permission is incorrectly evaluated against the mounter's credentials instead of the executing task when mapping a backing file.
- [Low] The patch references undefined struct members, functions, and LSM hooks, causing a build failure.
- [Low] Calling `backing_file_user_path()` with a `const struct file *` discards the `const` qualifier.

Pre-existing issues:
- [High] The code incorrectly uses `!file` to determine if a VMA is an anonymous stack mapping, creating a potential SELinux bypass.
--

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

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

This patch updates SELinux to enforce backing file access controls for
mmap() and mprotect() operations on overlayfs. It introduces checks using
the newly created security_mmap_backing_file() LSM hook and backing file
APIs.

> 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

[ ... ]

> @@ -1724,49 +1724,72 @@ static inline int file_path_has_perm(const struct cred *cred,
> +static int __file_has_perm(const struct cred *cred, const struct file *file,
> +			   u32 av, bool bf_user_file)
> +
>  {
> -	struct file_security_struct *fsec = selinux_file(file);
> -	struct inode *inode = file_inode(file);
>  	struct common_audit_data ad;
> -	u32 sid = cred_sid(cred);
> +	struct inode *inode;
> +	u32 ssid = cred_sid(cred);
> +	u32 tsid_fd;
>  	int rc;
>  
> -	ad.type = LSM_AUDIT_DATA_FILE;
> -	ad.u.file = file;
> +	if (bf_user_file) {
> +		struct backing_file_security_struct *bfsec;
> +		const struct path *path;
> +
> +		if (WARN_ON(!(file->f_mode & FMODE_BACKING)))
> +			return -EIO;
> +
> +		bfsec = selinux_backing_file(file);
> +		path = backing_file_user_path(file);

[Severity: Low]
Does this discard the const qualifier from the file pointer?
__file_has_perm() accepts a const struct file pointer, but
backing_file_user_path() expects a mutable pointer. This might trigger a
compiler warning.

[ ... ]

> -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 evaluate the PROCESS__EXECMEM permission against the mounter's
credentials instead of the executing task?

When mapping a backing file, selinux_mmap_backing_file() and
selinux_file_mprotect() pass the mounter's credentials (file->f_cred) to this
function. Because PROCESS__EXECMEM is a subject-centric privilege that
governs the executing task's memory, evaluating it against the mounter's SID
could incorrectly require the mounter to possess execmem privileges, causing
legitimate executable mappings to fail.

[ ... ]

>  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 this incorrectly use !file to determine
if a VMA is an anonymous stack mapping?

Using !file as a proxy for an anonymous VMA can be bypassed. If a stack
mapping is backed by a file (e.g., a private mapping of /dev/zero), the !file
check evaluates to false. This skips the PROCESS__EXECSTACK check and falls
through to the FILE__EXECMOD check, potentially allowing a bypass. Should this
be updated to use vma_is_anonymous(vma) instead?

[ ... ]

> @@ -6960,6 +7071,7 @@ static void selinux_bpf_token_free(struct bpf_token *token)
>  struct lsm_blob_sizes selinux_blob_sizes __ro_after_init = {
>  	.lbs_cred = sizeof(struct task_security_struct),
>  	.lbs_file = sizeof(struct file_security_struct),
> +	.lbs_backing_file = sizeof(struct backing_file_security_struct),
>  	.lbs_inode = sizeof(struct inode_security_struct),
>  	.lbs_ipc = sizeof(struct ipc_security_struct),

[Severity: Low]
Does this cause a build failure? It looks like lbs_backing_file (along with
the backing_file_alloc and mmap_backing_file LSM hooks registered below) does
not exist in the core LSM framework at this commit.

[ ... ]

> diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
> index c88cae81ee4c5..dc42282a2c052 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h

[ ... ]

> @@ -159,6 +163,13 @@ static inline struct file_security_struct *selinux_file(const struct file *file)
>  	return file->f_security + selinux_blob_sizes.lbs_file;
>  }
>  
> +static inline struct backing_file_security_struct *
> +selinux_backing_file(const struct file *backing_file)
> +{
> +	void *blob = backing_file_security(backing_file);
> +	return blob + selinux_blob_sizes.lbs_backing_file;
> +}
> +

[Severity: Low]
Is backing_file_security() defined anywhere? Similar to the LSM hook and blob
size additions, this appears to be missing from the tree and will likely
prevent compilation.

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