[PATCH v2 0/2] Fix SELinux context mount with the cgroup filesystem

[PATCH v2 0/2] Fix SELinux context mount with the cgroup filesystem

[Ondrej Mosnacek]

This series contains two bugfixes that fix mounting the cgroup filesystem
with the 'context=' option under SELinux (and potentially other cases as well).

Changes in v2:
 - drop first patch (already picked up by cgroups maintainer)
 - extract the genfs special handling condition into a separate function
 - check for SBLABEL_MNT directly in selinux_inode_setsecurity() and in
   selinux_inode_notifysecctx() just translate -ENOTSUPP to 0
v1: https://lore.kernel.org/selinux/20181213141739.8534-1-omosnace@redhat.com/

Note that this series is testable only with patch [1] applied (it has already
been picked up by Tejun Heo, so I dopped it from this series).

The first patch fixes SELinux to always disallow relabeling inodes that
belong to a 'context=' mount.

The second patch fixes SELinux to ignore security_inode_notifysecctx() calls
and disallow security_inode_setsecurity() calls on inodes that belong to
a 'context=' mount.

Testing: Passed selinux-testsuite and verified using the reproducers.

[1] https://lore.kernel.org/selinux/20181213141739.8534-2-omosnace@redhat.com/

Ondrej Mosnacek (2):
  selinux: never allow relabeling on context mounts
  selinux: do not override context on context mounts

 security/selinux/hooks.c | 49 ++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 10 deletions(-)

-- 
2.19.2

[PATCH v2 1/2] selinux: never allow relabeling on context mounts

[Ondrej Mosnacek]

In the SECURITY_FS_USE_MNTPOINT case we never want to allow relabeling
files/directories, so we should never set the SBLABEL_MNT flag. The
'special handling' in selinux_is_sblabel_mnt() is only intended for when
the behavior is set to SECURITY_FS_USE_GENFS.

While there, make the logic in selinux_is_sblabel_mnt() more explicit
and add a BUILD_BUG_ON() to make sure that introducing a new
SECURITY_FS_USE_* forces a review of the logic.

Fixes: d5f3a5f6e7e7 ("selinux: add security in-core xattr support for pstore and debugfs")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7ce012d9ec51..b4759bebeddc 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -497,16 +497,10 @@ static int may_context_mount_inode_relabel(u32 sid,
 	return rc;
 }
 
-static int selinux_is_sblabel_mnt(struct super_block *sb)
+static int selinux_is_genfs_special_handling(struct super_block *sb)
 {
-	struct superblock_security_struct *sbsec = sb->s_security;
-
-	return sbsec->behavior == SECURITY_FS_USE_XATTR ||
-		sbsec->behavior == SECURITY_FS_USE_TRANS ||
-		sbsec->behavior == SECURITY_FS_USE_TASK ||
-		sbsec->behavior == SECURITY_FS_USE_NATIVE ||
-		/* Special handling. Genfs but also in-core setxattr handler */
-		!strcmp(sb->s_type->name, "sysfs") ||
+	/* Special handling. Genfs but also in-core setxattr handler */
+	return	!strcmp(sb->s_type->name, "sysfs") ||
 		!strcmp(sb->s_type->name, "pstore") ||
 		!strcmp(sb->s_type->name, "debugfs") ||
 		!strcmp(sb->s_type->name, "tracefs") ||
@@ -516,6 +510,34 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
 		  !strcmp(sb->s_type->name, "cgroup2")));
 }
 
+static int selinux_is_sblabel_mnt(struct super_block *sb)
+{
+	struct superblock_security_struct *sbsec = sb->s_security;
+
+	/*
+	 * IMPORTANT: Double-check logic in this function when adding a new
+	 * SECURITY_FS_USE_* definition!
+	 */
+	BUILD_BUG_ON(SECURITY_FS_USE_MAX != 7);
+
+	switch (sbsec->behavior) {
+	case SECURITY_FS_USE_XATTR:
+	case SECURITY_FS_USE_TRANS:
+	case SECURITY_FS_USE_TASK:
+	case SECURITY_FS_USE_NATIVE:
+		return 1;
+
+	case SECURITY_FS_USE_GENFS:
+		return selinux_is_genfs_special_handling(sb);
+
+	/* Never allow relabeling on context mounts */
+	case SECURITY_FS_USE_MNTPOINT:
+	case SECURITY_FS_USE_NONE:
+	default:
+		return 0;
+	}
+}
+
 static int sb_finish_set_opts(struct super_block *sb)
 {
 	struct superblock_security_struct *sbsec = sb->s_security;
-- 
2.19.2

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

[Ondrej Mosnacek]

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>
---
 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;
 }
 
 /*
-- 
2.19.2

Re: [PATCH v2 1/2] selinux: never allow relabeling on context mounts

[Stephen Smalley]

On 12/21/18 3:18 PM, Ondrej Mosnacek wrote:
> In the SECURITY_FS_USE_MNTPOINT case we never want to allow relabeling
> files/directories, so we should never set the SBLABEL_MNT flag. The
> 'special handling' in selinux_is_sblabel_mnt() is only intended for when
> the behavior is set to SECURITY_FS_USE_GENFS.
> 
> While there, make the logic in selinux_is_sblabel_mnt() more explicit
> and add a BUILD_BUG_ON() to make sure that introducing a new
> SECURITY_FS_USE_* forces a review of the logic.
> 
> Fixes: d5f3a5f6e7e7 ("selinux: add security in-core xattr support for pstore and debugfs")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

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

> ---
>   security/selinux/hooks.c | 40 +++++++++++++++++++++++++++++++---------
>   1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7ce012d9ec51..b4759bebeddc 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -497,16 +497,10 @@ static int may_context_mount_inode_relabel(u32 sid,
>   	return rc;
>   }
>   
> -static int selinux_is_sblabel_mnt(struct super_block *sb)
> +static int selinux_is_genfs_special_handling(struct super_block *sb)
>   {
> -	struct superblock_security_struct *sbsec = sb->s_security;
> -
> -	return sbsec->behavior == SECURITY_FS_USE_XATTR ||
> -		sbsec->behavior == SECURITY_FS_USE_TRANS ||
> -		sbsec->behavior == SECURITY_FS_USE_TASK ||
> -		sbsec->behavior == SECURITY_FS_USE_NATIVE ||
> -		/* Special handling. Genfs but also in-core setxattr handler */
> -		!strcmp(sb->s_type->name, "sysfs") ||
> +	/* Special handling. Genfs but also in-core setxattr handler */
> +	return	!strcmp(sb->s_type->name, "sysfs") ||
>   		!strcmp(sb->s_type->name, "pstore") ||
>   		!strcmp(sb->s_type->name, "debugfs") ||
>   		!strcmp(sb->s_type->name, "tracefs") ||
> @@ -516,6 +510,34 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
>   		  !strcmp(sb->s_type->name, "cgroup2")));
>   }
>   
> +static int selinux_is_sblabel_mnt(struct super_block *sb)
> +{
> +	struct superblock_security_struct *sbsec = sb->s_security;
> +
> +	/*
> +	 * IMPORTANT: Double-check logic in this function when adding a new
> +	 * SECURITY_FS_USE_* definition!
> +	 */
> +	BUILD_BUG_ON(SECURITY_FS_USE_MAX != 7);
> +
> +	switch (sbsec->behavior) {
> +	case SECURITY_FS_USE_XATTR:
> +	case SECURITY_FS_USE_TRANS:
> +	case SECURITY_FS_USE_TASK:
> +	case SECURITY_FS_USE_NATIVE:
> +		return 1;
> +
> +	case SECURITY_FS_USE_GENFS:
> +		return selinux_is_genfs_special_handling(sb);
> +
> +	/* Never allow relabeling on context mounts */
> +	case SECURITY_FS_USE_MNTPOINT:
> +	case SECURITY_FS_USE_NONE:
> +	default:
> +		return 0;
> +	}
> +}
> +
>   static int sb_finish_set_opts(struct super_block *sb)
>   {
>   	struct superblock_security_struct *sbsec = sb->s_security;
> 

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

[Stephen Smalley]

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;
>   }
>   
>   /*
> 

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

[Ondrej Mosnacek]

On Fri, Dec 21, 2018 at 9:45 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 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?

It is because the context set via setxattr is stored inside the kernfs
node and the kernfs tree is preserved across mounts/unmounts. It
actually behaves sort of like a normal filesystem backed by a block
device, just the "device" is represented by an in-memory kernfs tree
which is discarded at poweroff.

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

Thanks!

>
> > ---
> >   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;
> >   }
> >
> >   /*
> >

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

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

[Paul Moore]

On Fri, Dec 21, 2018 at 4:49 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Fri, Dec 21, 2018 at 9:45 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > 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?
>
> It is because the context set via setxattr is stored inside the kernfs
> node and the kernfs tree is preserved across mounts/unmounts. It
> actually behaves sort of like a normal filesystem backed by a block
> device, just the "device" is represented by an in-memory kernfs tree
> which is discarded at poweroff.

That makes sense.

I'll take a closer look at these once we're past the upcoming merge
window, but they look okay after a quick glance.

> > Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
>
> Thanks!
>
> >
> > > ---
> > >   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;
> > >   }
> > >
> > >   /*
> > >
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Fri, Dec 21, 2018 at 5:59 PM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Dec 21, 2018 at 4:49 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Fri, Dec 21, 2018 at 9:45 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > 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?
> >
> > It is because the context set via setxattr is stored inside the kernfs
> > node and the kernfs tree is preserved across mounts/unmounts. It
> > actually behaves sort of like a normal filesystem backed by a block
> > device, just the "device" is represented by an in-memory kernfs tree
> > which is discarded at poweroff.
>
> That makes sense.
>
> I'll take a closer look at these once we're past the upcoming merge
> window, but they look okay after a quick glance.

Still looked fine to me, merged both 1/2 and 2/2 into selinux/next.
Thanks Ondrej.

-- 
paul moore
www.paul-moore.com