Re: [PATCH v2 2/2] selinux: do not override context on context mounts

[Stephen Smalley <sds@tycho.nsa.gov>]

On 12/21/18 3:18 PM, Ondrej Mosnacek wrote:
> Ignore all selinux_inode_notifysecctx() calls on mounts with SBLABEL_MNT
> flag unset. This is achived by returning -EOPNOTSUPP for this case in
> selinux_inode_setsecurtity() (because that function should not be called
> in such case anyway) and translating this error to 0 in
> selinux_inode_notifysecctx().
> 
> This fixes behavior of kernfs-based filesystems when mounted with the
> 'context=' option. Before this patch, if a node's context had been
> explicitly set to a non-default value and later the filesystem has been
> remounted with the 'context=' option, then this node would show up as
> having the manually-set context and not the mount-specified one.
> 
> Steps to reproduce:
>      # mount -t cgroup2 cgroup2 /sys/fs/cgroup/unified
>      # chcon unconfined_u:object_r:user_home_t:s0 /sys/fs/cgroup/unified/cgroup.stat
>      # ls -lZ /sys/fs/cgroup/unified
>      total 0
>      -r--r--r--. 1 root root system_u:object_r:cgroup_t:s0        0 Dec 13 10:41 cgroup.controllers
>      -rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0        0 Dec 13 10:41 cgroup.max.depth
>      -rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0        0 Dec 13 10:41 cgroup.max.descendants
>      -rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0        0 Dec 13 10:41 cgroup.procs
>      -r--r--r--. 1 root root unconfined_u:object_r:user_home_t:s0 0 Dec 13 10:41 cgroup.stat
>      -rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0        0 Dec 13 10:41 cgroup.subtree_control
>      -rw-r--r--. 1 root root system_u:object_r:cgroup_t:s0        0 Dec 13 10:41 cgroup.threads
>      # umount /sys/fs/cgroup/unified
>      # mount -o context=system_u:object_r:tmpfs_t:s0 -t cgroup2 cgroup2 /sys/fs/cgroup/unified
> 
> Result before:
>      # ls -lZ /sys/fs/cgroup/unified
>      total 0
>      -r--r--r--. 1 root root system_u:object_r:tmpfs_t:s0         0 Dec 13 10:41 cgroup.controllers
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0         0 Dec 13 10:41 cgroup.max.depth
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0         0 Dec 13 10:41 cgroup.max.descendants
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0         0 Dec 13 10:41 cgroup.procs
>      -r--r--r--. 1 root root unconfined_u:object_r:user_home_t:s0 0 Dec 13 10:41 cgroup.stat
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0         0 Dec 13 10:41 cgroup.subtree_control
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0         0 Dec 13 10:41 cgroup.threads
> 
> Result after:
>      # ls -lZ /sys/fs/cgroup/unified
>      total 0
>      -r--r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.controllers
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.max.depth
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.max.descendants
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.procs
>      -r--r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.stat
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.subtree_control
>      -rw-r--r--. 1 root root system_u:object_r:tmpfs_t:s0 0 Dec 13 10:41 cgroup.threads
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

The patch looks good to me but I am a little puzzled by the cgroup2 
behavior here.  I would have expected that unmounting would have killed 
the superblock and thus discarded the previously set label.  I guess 
something else is keeping it alive?

Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/hooks.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b4759bebeddc..fcf4af1e5157 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3477,12 +3477,16 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>   				     const void *value, size_t size, int flags)
>   {
>   	struct inode_security_struct *isec = inode_security_novalidate(inode);
> +	struct superblock_security_struct *sbsec = inode->i_sb->s_security;
>   	u32 newsid;
>   	int rc;
>   
>   	if (strcmp(name, XATTR_SELINUX_SUFFIX))
>   		return -EOPNOTSUPP;
>   
> +	if (!(sbsec->flags & SBLABEL_MNT))
> +		return -EOPNOTSUPP;
> +
>   	if (!value || !size)
>   		return -EACCES;
>   
> @@ -6625,7 +6629,10 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
>    */
>   static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>   {
> -	return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> +	int rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX,
> +					   ctx, ctxlen, 0);
> +	/* Do not return error when suppressing label (SBLABEL_MNT not set). */
> +	return rc == -EOPNOTSUPP ? 0 : rc;
>   }
>   
>   /*
>