Re: [PATCH] selinux: fix incorrect execmem checks on overlayfs

On Jul  3, 2026 Ondrej Mosnacek <[email protected]> wrote:
> 
> The commit fixing the overlayfs mmap() and mprotect() access checks
> failed to skip the execmem check in __file_map_prot_check() for the case
> where the "mounter check" is being performed. This check should be
> performed only against the credentials of the task that is calling
> mmap()/mprotect(), since it doesn't pertain to the file itself, but
> rather just gates the ability of the calling task to get an executable
> memory mapping in general.
> 
> The purpose of the "mounter check" is to guard against using an
> overlayfs mount to gain file access that would otherwise be denied to
> the mounter. For execmem this is not relevant, as there is no further
> file access granted based on it (notice that the file's context is not
> used as the target in the check), so checking it also against the
> mounter credentials would be incorrect.
> 
> Fix this by passing a boolean to [__]file_map_prot_check() and
> selinux_mmap_file_common() that indicates if we are doing the "mounter
> check" and skiping the execmem check in that case. Since this boolean
> also indicates if we use current_cred() or the mounter cred as the
> subject, also remove the "cred" argument from these functions and
> determine it based on the boolean and the file struct.
> 
> Fixes: 82544d36b172 ("selinux: fix overlayfs mmap() and mprotect() access checks")
> Signed-off-by: Ondrej Mosnacek <[email protected]>
> Reviewed-by: Stephen Smalley <[email protected]>
> Tested-by: Stephen Smalley <[email protected]>
> ---
>  security/selinux/hooks.c | 39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1a713d96206f5..87790f6f0d4de 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3969,13 +3969,14 @@ 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(const struct cred *cred,
> -				 const struct file *file, unsigned long prot,
> -				 bool shared, bool bf_user_file)
> +static int __file_map_prot_check(const struct file *file, unsigned long prot,
> +				 bool shared, bool mounter_check,
> +				 bool bf_user_file)
>  {
>  	struct inode *inode = NULL;
>  	bool prot_exec = prot & PROT_EXEC;
>  	bool prot_write = prot & PROT_WRITE;
> +	const struct cred *cred = mounter_check ? file->f_cred : current_cred();

Moving the cred lookup down into these functions makes me nervous that
we could run into a problem someday when 'mounter_check' is true and
'file' is NULL (contrived given the current code, yes, but we've been
burned by code changes before).

Let's protect this cred lookup to ensure that 'file' is non-NULL before
we reference it.

>  	if (file) {
>  		if (bf_user_file)
> @@ -3984,7 +3985,7 @@ static int __file_map_prot_check(const struct cred *cred,
>  			inode = file_inode(file);
>  	}
>  
> -	if (default_noexec && prot_exec &&
> +	if (!mounter_check && default_noexec && prot_exec &&
>  	    (!file || IS_PRIVATE(inode) || (!shared && prot_write))) {
>  		int rc;
>  		u32 sid = cred_sid(cred);
> @@ -4013,11 +4014,11 @@ static int __file_map_prot_check(const struct cred *cred,
>  	return 0;
>  }
>  
> -static inline int file_map_prot_check(const struct cred *cred,
> -				      const struct file *file,
> -				      unsigned long prot, bool shared)
> +static inline int file_map_prot_check(const struct file *file,
> +				      unsigned long prot, bool shared,
> +				      bool mounter_check)
>  {
> -	return __file_map_prot_check(cred, file, prot, shared, false);
> +	return __file_map_prot_check(file, prot, shared, mounter_check, false);
>  }
>  
>  static int selinux_mmap_addr(unsigned long addr)
> @@ -4033,9 +4034,11 @@ static int selinux_mmap_addr(unsigned long addr)
>  	return rc;
>  }
>  
> -static int selinux_mmap_file_common(const struct cred *cred, struct file *file,
> -				    unsigned long prot, bool shared)
> +static int selinux_mmap_file_common(struct file *file, unsigned long prot,
> +				    bool shared, bool mounter_check)
>  {
> +	const struct cred *cred = mounter_check ? file->f_cred : current_cred();

See above comments.

>  	if (file) {
>  		int rc;
>  		struct common_audit_data ad;
> @@ -4047,15 +4050,16 @@ static int selinux_mmap_file_common(const struct cred *cred, struct file *file,
>  			return rc;
>  	}
>  
> -	return file_map_prot_check(cred, file, prot, shared);
> +	return file_map_prot_check(file, prot, shared, mounter_check);
>  }
>  
>  static int selinux_mmap_file(struct file *file,
>  			     unsigned long reqprot __always_unused,
>  			     unsigned long prot, unsigned long flags)
>  {
> -	return selinux_mmap_file_common(current_cred(), file, prot,
> -					(flags & MAP_TYPE) == MAP_SHARED);
> +	return selinux_mmap_file_common(file, prot,
> +					(flags & MAP_TYPE) == MAP_SHARED,
> +					false);
>  }
>  
>  /**
> @@ -4087,8 +4091,9 @@ static int selinux_mmap_backing_file(struct vm_area_struct *vma,
>  	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);
> +	return selinux_mmap_file_common(backing_file, prot,
> +					vma->vm_flags & VM_SHARED,
> +					true);
>  }
>  
>  static int selinux_file_mprotect(struct vm_area_struct *vma,
> @@ -4149,11 +4154,11 @@ static int selinux_file_mprotect(struct vm_area_struct *vma,
>  		}
>  	}
>  
> -	rc = __file_map_prot_check(cred, file, prot, shared, backing_file);
> +	rc = __file_map_prot_check(file, prot, shared, false, backing_file);
>  	if (rc)
>  		return rc;
>  	if (backing_file) {
> -		rc = file_map_prot_check(file->f_cred, file, prot, shared);
> +		rc = file_map_prot_check(file, prot, shared, true);
>  		if (rc)
>  			return rc;
>  	}
> -- 
> 2.55.0

--
paul-moore.com