Re: [PATCH v2] selinux: preserve user SID across nested backing files

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

Pre-existing issues:
- [High] SELinux mmap/mprotect checks evaluate execute permissions against the intermediate layer's inode instead of the top-level user file's inode in nested backing file setups, allowing a policy bypass.
--

--- Patch [1]: [PATCH v2] selinux: preserve user SID across nested backing files ---
commit 9cc55180a6ab3bcd9de427fe0ddf47951d8a72d6
Author: Karl Mehltretter <[email protected]>

selinux: preserve user SID across nested backing files

This commit fixes SID propagation across nested backing files so the top-level
user SID is preserved in the security blob. It prevents mprotect() from
incorrectly checking access against the mounter SID instead of the user SID.
The fix handles nested overlayfs or FUSE passthrough by copying the saved user
SID when user_file is a backing file.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1ead2eee1944d..171b90412ff1f 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3843,13 +3843,20 @@ static int selinux_file_alloc_security(struct file *file)
>  	return 0;
>  }
>  
> +static inline u32 selinux_file_user_sid(const struct file *file)
> +{
> +	if (unlikely(file->f_mode & FMODE_BACKING))
> +		return selinux_backing_file(file)->uf_sid;
> +	return selinux_file(file)->sid;
> +}
> +
>  static int selinux_backing_file_alloc(struct file *backing_file,
>  				      const struct file *user_file)
>  {
>  	struct backing_file_security_struct *bfsec;
>  
>  	bfsec = selinux_backing_file(backing_file);
> -	bfsec->uf_sid = selinux_file(user_file)->sid;
> +	bfsec->uf_sid = selinux_file_user_sid(user_file);

[Severity: High]
This is a pre-existing issue, but does evaluating the intermediate layer's
inode instead of the top-level user file's inode allow a policy bypass?

In security/selinux/hooks.c:__file_has_perm():

    bfsec = selinux_backing_file(file);
    path = backing_file_user_path(file);
    tsid_fd = bfsec->uf_sid;
    inode = d_inode(path->dentry);

When an attacker maps a file from a nested overlayfs (e.g., ovl1 over ovl2)
with PROT_READ, the top-level read permissions are checked. If the attacker
subsequently calls mprotect(PROT_EXEC), selinux_file_mprotect() invokes
__file_map_prot_check() on the lowest-level backing file. 

Because backing_file_open() assigns user_path to the immediate user_file's
path directly without recursively unwrapping, backing_file_user_path() points
to the intermediate overlayfs file. 

Does this mean FILE__EXECUTE is evaluated against the intermediate inode
rather than the top-level inode, which could bypass a restrictive execute
policy on the top-level mount if the intermediate mount allows execution?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1