[RFC PATCH 0/3] Fix SELinux context mount with the cgroup filesystem

[RFC PATCH 0/3] Fix SELinux context mount with the cgroup filesystem

[Ondrej Mosnacek]

This series contains three independent bugfixes that together make it possible
to mount the cgroup filesystem with the 'context=' option under SELinux.

The first patch is trivial and fixes cgroupfs to correctly handle the case when
the mount options are just an empty string.

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

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

Paul, Stephen, please have a quick look at the last two patches, I'm not 100%
sure that I understand the expected behavior of the context mounts correctly.
(My assumption is that in a context mount we want the whole subtree to be
labeled with the given label, no matter what.)

Also, I'm not entirely satisfied with the code style in the second patch (and
it produces an annoying false positive with checkpatch.pl), but I didn't see
a better way to write it...

I haven't had time to do much testing on the patches (other than the
reproducers mentioned in the commit messages). I'd like to make sure that
I'm going in the right direction first.

Thanks,

O.M.

--
Ondrej Mosnacek (3):
  cgroup: fix parsing empty mount option string
  selinux: never allow relabeling on context mounts
  selinux: do not override context on context mounts

 kernel/cgroup/cgroup.c   |  2 +-
 security/selinux/hooks.c | 48 ++++++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 13 deletions(-)

-- 
2.19.2

[RFC PATCH 1/3] cgroup: fix parsing empty mount option string

[Ondrej Mosnacek]

This fixes the case where all mount options specified are consumed by an
LSM and all that's left is an empty string. In this case cgroupfs should
accept the string and not fail.

How to reproduce (with SELinux enabled):

    # umount /sys/fs/cgroup/unified
    # mount -o context=system_u:object_r:cgroup_t:s0 -t cgroup2 cgroup2 /sys/fs/cgroup/unified
    mount: /sys/fs/cgroup/unified: wrong fs type, bad option, bad superblock on cgroup2, missing codepage or helper program, or other error.
    # dmesg | tail -n 1
    [   31.575952] cgroup: cgroup2: unknown option ""

Fixes: 67e9c74b8a87 ("cgroup: replace __DEVEL__sane_behavior with cgroup2 fs type")
[NOTE: should apply on top of commit 5136f6365ce3 ("cgroup: implement "nsdelegate" mount option"), older versions need manual rebase]
Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 kernel/cgroup/cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 6aaf5dd5383b..8cb616232035 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -1744,7 +1744,7 @@ static int parse_cgroup_root_flags(char *data, unsigned int *root_flags)
 
 	*root_flags = 0;
 
-	if (!data)
+	if (!data || *data == '\0')
 		return 0;
 
 	while ((token = strsep(&data, ",")) != NULL) {
-- 
2.19.2

[RFC PATCH 2/3] 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 in this
case. The 'special handling' in selinux_is_sblabel_mnt() is only
intended for 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.

Note that checkpatch.pl produces some false positives here, likely
having problems recognizing the monstrous return statement...

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 | 41 ++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7ce012d9ec51..d6d29ec54eab 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -501,19 +501,36 @@ static int selinux_is_sblabel_mnt(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 ||
+	/*
+	 * 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:
 		/* Special handling. Genfs but also in-core setxattr handler */
-		!strcmp(sb->s_type->name, "sysfs") ||
-		!strcmp(sb->s_type->name, "pstore") ||
-		!strcmp(sb->s_type->name, "debugfs") ||
-		!strcmp(sb->s_type->name, "tracefs") ||
-		!strcmp(sb->s_type->name, "rootfs") ||
-		(selinux_policycap_cgroupseclabel() &&
-		 (!strcmp(sb->s_type->name, "cgroup") ||
-		  !strcmp(sb->s_type->name, "cgroup2")));
+		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") ||
+			!strcmp(sb->s_type->name, "rootfs") ||
+			(selinux_policycap_cgroupseclabel() &&
+			 (!strcmp(sb->s_type->name, "cgroup") ||
+			  !strcmp(sb->s_type->name, "cgroup2")));
+
+	/* 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)
-- 
2.19.2

[RFC PATCH 3/3] selinux: do not override context on context mounts

[Ondrej Mosnacek]

Ignore all selinux_inode_notifysecctx() calls on mounts with the
SECURITY_FS_USE_MNTPOINT behavior.

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 a different context.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index d6d29ec54eab..0ca5ed30afe1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6620,6 +6620,13 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
  */
 static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
 {
+	struct superblock_security_struct *sbsec = inode->i_sb->s_security;
+
+	/* Do not change context in SECURITY_FS_USE_MNTPOINT case */
+	if ((sbsec->flags & SE_SBINITIALIZED) &&
+	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
+		return 0;
+
 	return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
 }
 
-- 
2.19.2

Re: [RFC PATCH 1/3] cgroup: fix parsing empty mount option string

[Tejun Heo]

Hello,

On Thu, Dec 13, 2018 at 03:17:37PM +0100, Ondrej Mosnacek wrote:
> This fixes the case where all mount options specified are consumed by an
> LSM and all that's left is an empty string. In this case cgroupfs should
> accept the string and not fail.
> 
> How to reproduce (with SELinux enabled):
> 
>     # umount /sys/fs/cgroup/unified
>     # mount -o context=system_u:object_r:cgroup_t:s0 -t cgroup2 cgroup2 /sys/fs/cgroup/unified
>     mount: /sys/fs/cgroup/unified: wrong fs type, bad option, bad superblock on cgroup2, missing codepage or helper program, or other error.
>     # dmesg | tail -n 1
>     [   31.575952] cgroup: cgroup2: unknown option ""
> 
> Fixes: 67e9c74b8a87 ("cgroup: replace __DEVEL__sane_behavior with cgroup2 fs type")
> [NOTE: should apply on top of commit 5136f6365ce3 ("cgroup: implement "nsdelegate" mount option"), older versions need manual rebase]
> Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Applied to cgroup/for-4.21.

Thanks.

-- 
tejun

Re: [RFC PATCH 2/3] selinux: never allow relabeling on context mounts

[Stephen Smalley]

On 12/13/18 9:17 AM, 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 in this
> case. The 'special handling' in selinux_is_sblabel_mnt() is only
> intended for 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.
> 
> Note that checkpatch.pl produces some false positives here, likely
> having problems recognizing the monstrous return statement...
> 
> 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 | 41 ++++++++++++++++++++++++++++------------
>   1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7ce012d9ec51..d6d29ec54eab 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -501,19 +501,36 @@ static int selinux_is_sblabel_mnt(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 ||
> +	/*
> +	 * 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:
>   		/* Special handling. Genfs but also in-core setxattr handler */
> -		!strcmp(sb->s_type->name, "sysfs") ||
> -		!strcmp(sb->s_type->name, "pstore") ||
> -		!strcmp(sb->s_type->name, "debugfs") ||
> -		!strcmp(sb->s_type->name, "tracefs") ||
> -		!strcmp(sb->s_type->name, "rootfs") ||
> -		(selinux_policycap_cgroupseclabel() &&
> -		 (!strcmp(sb->s_type->name, "cgroup") ||
> -		  !strcmp(sb->s_type->name, "cgroup2")));
> +		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") ||
> +			!strcmp(sb->s_type->name, "rootfs") ||
> +			(selinux_policycap_cgroupseclabel() &&
> +			 (!strcmp(sb->s_type->name, "cgroup") ||
> +			  !strcmp(sb->s_type->name, "cgroup2")));
> +
> +	/* Never allow relabeling on context mounts */
> +	case SECURITY_FS_USE_MNTPOINT:
> +	case SECURITY_FS_USE_NONE:
> +	default:
> +		return 0;
> +	}
>   }

This looks sane to me. Note that 
https://github.com/SELinuxProject/selinux-kernel/issues/2 calls for 
replacing the use of hardcoded lists of filesystem types above with 
something more general.  Maybe you could at least abstract that part 
into its own function.

>   
>   static int sb_finish_set_opts(struct super_block *sb)
> 

Re: [RFC PATCH 3/3] selinux: do not override context on context mounts

[Stephen Smalley]

On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
> Ignore all selinux_inode_notifysecctx() calls on mounts with the
> SECURITY_FS_USE_MNTPOINT behavior.
> 
> 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 a different context.
> 
> 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 | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index d6d29ec54eab..0ca5ed30afe1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6620,6 +6620,13 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
>    */
>   static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>   {
> +	struct superblock_security_struct *sbsec = inode->i_sb->s_security;
> +
> +	/* Do not change context in SECURITY_FS_USE_MNTPOINT case */
> +	if ((sbsec->flags & SE_SBINITIALIZED) &&
> +	    (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
> +		return 0;
> +
>   	return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
>   }

Wondering if we ought to take this into selinux_inode_setsecurity() and 
return -EOPNOTSUPP in that case.  We already return -EOPNOTSUPP from 
selinux_inode_setxattr() if (!(sbsec->flags & SBLABEL_MNT)) and that 
should precede other calls to selinux_inode_setsecurity() IIRC.  Should 
we just be checking SBLABEL_MNT here instead?  And do we need to 
separately check SE_SBINITIALIZED?

Re: [RFC PATCH 2/3] selinux: never allow relabeling on context mounts

[Ondrej Mosnacek]

On Thu, Dec 13, 2018 at 5:16 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/13/18 9:17 AM, 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 in this
> > case. The 'special handling' in selinux_is_sblabel_mnt() is only
> > intended for 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.
> >
> > Note that checkpatch.pl produces some false positives here, likely
> > having problems recognizing the monstrous return statement...
> >
> > 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 | 41 ++++++++++++++++++++++++++++------------
> >   1 file changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 7ce012d9ec51..d6d29ec54eab 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -501,19 +501,36 @@ static int selinux_is_sblabel_mnt(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 ||
> > +     /*
> > +      * 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:
> >               /* Special handling. Genfs but also in-core setxattr handler */
> > -             !strcmp(sb->s_type->name, "sysfs") ||
> > -             !strcmp(sb->s_type->name, "pstore") ||
> > -             !strcmp(sb->s_type->name, "debugfs") ||
> > -             !strcmp(sb->s_type->name, "tracefs") ||
> > -             !strcmp(sb->s_type->name, "rootfs") ||
> > -             (selinux_policycap_cgroupseclabel() &&
> > -              (!strcmp(sb->s_type->name, "cgroup") ||
> > -               !strcmp(sb->s_type->name, "cgroup2")));
> > +             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") ||
> > +                     !strcmp(sb->s_type->name, "rootfs") ||
> > +                     (selinux_policycap_cgroupseclabel() &&
> > +                      (!strcmp(sb->s_type->name, "cgroup") ||
> > +                       !strcmp(sb->s_type->name, "cgroup2")));
> > +
> > +     /* Never allow relabeling on context mounts */
> > +     case SECURITY_FS_USE_MNTPOINT:
> > +     case SECURITY_FS_USE_NONE:
> > +     default:
> > +             return 0;
> > +     }
> >   }
>
> This looks sane to me. Note that
> https://github.com/SELinuxProject/selinux-kernel/issues/2 calls for
> replacing the use of hardcoded lists of filesystem types above with
> something more general.  Maybe you could at least abstract that part
> into its own function.

Agreed, that's a good idea. It should also allow checkpatch.pl to
correctly parse the control flow and not throw an error.

>
> >
> >   static int sb_finish_set_opts(struct super_block *sb)
> >
>

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

Re: [RFC PATCH 3/3] selinux: do not override context on context mounts

[Ondrej Mosnacek]

On Thu, Dec 13, 2018 at 5:25 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
> > Ignore all selinux_inode_notifysecctx() calls on mounts with the
> > SECURITY_FS_USE_MNTPOINT behavior.
> >
> > 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 a different context.
> >
> > 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 | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index d6d29ec54eab..0ca5ed30afe1 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6620,6 +6620,13 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
> >    */
> >   static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> >   {
> > +     struct superblock_security_struct *sbsec = inode->i_sb->s_security;
> > +
> > +     /* Do not change context in SECURITY_FS_USE_MNTPOINT case */
> > +     if ((sbsec->flags & SE_SBINITIALIZED) &&
> > +         (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
> > +             return 0;
> > +
> >       return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> >   }
>
> Wondering if we ought to take this into selinux_inode_setsecurity() and
> return -EOPNOTSUPP in that case.  We already return -EOPNOTSUPP from
> selinux_inode_setxattr() if (!(sbsec->flags & SBLABEL_MNT)) and that
> should precede other calls to selinux_inode_setsecurity() IIRC.

Maybe, but see below. In selinux_inode_setsecurity() we should indeed
check for SBLABEL_MNT, but only if it is called directly as a hook
(but I'm not sure if it is worth it in this case, since as you say, a
prior selinux_inode_setxattr() failure should always prevent this hook
from being called). selinux_inode_notifysecctx() has a bit different
semantics, IMHO.

> Should we just be checking SBLABEL_MNT here instead?

I don't think so. IIUC, the purpose of selinux_inode_notifysecctx() is
to adjust the sid that has been assigned by selinux_d_instantiate() by
the label that is 'stored' for the particular node internally by the
filesystem. I would say the fact whether we want to use the stored
label depends on the sbsec->behavior value (BTW, shouldn't we also
return 0 in case of SECURITY_FS_USE_TASK? or even
SECURITY_FS_USE_TRANS?). I understand the SBLABEL_MNT flag more as an
indication of whether we want the user to allow setting the label
explicitly (and probably also implicitly via tsec->create_sid).

> And do we need to separately check SE_SBINITIALIZED?

I'm not sure, but other places in the code check that flag before
checking sbsec->behavior, so it seemed to me as the right thing to do.

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

Re: [RFC PATCH 3/3] selinux: do not override context on context mounts

[Stephen Smalley]

On 12/18/18 10:50 AM, Ondrej Mosnacek wrote:
> On Thu, Dec 13, 2018 at 5:25 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
>>> Ignore all selinux_inode_notifysecctx() calls on mounts with the
>>> SECURITY_FS_USE_MNTPOINT behavior.
>>>
>>> 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 a different context.
>>>
>>> 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 | 7 +++++++
>>>    1 file changed, 7 insertions(+)
>>>
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index d6d29ec54eab..0ca5ed30afe1 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -6620,6 +6620,13 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
>>>     */
>>>    static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
>>>    {
>>> +     struct superblock_security_struct *sbsec = inode->i_sb->s_security;
>>> +
>>> +     /* Do not change context in SECURITY_FS_USE_MNTPOINT case */
>>> +     if ((sbsec->flags & SE_SBINITIALIZED) &&
>>> +         (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
>>> +             return 0;
>>> +
>>>        return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
>>>    }
>>
>> Wondering if we ought to take this into selinux_inode_setsecurity() and
>> return -EOPNOTSUPP in that case.  We already return -EOPNOTSUPP from
>> selinux_inode_setxattr() if (!(sbsec->flags & SBLABEL_MNT)) and that
>> should precede other calls to selinux_inode_setsecurity() IIRC.
> 
> Maybe, but see below. In selinux_inode_setsecurity() we should indeed
> check for SBLABEL_MNT, but only if it is called directly as a hook
> (but I'm not sure if it is worth it in this case, since as you say, a
> prior selinux_inode_setxattr() failure should always prevent this hook
> from being called). selinux_inode_notifysecctx() has a bit different
> semantics, IMHO.
> 
>> Should we just be checking SBLABEL_MNT here instead?
> 
> I don't think so. IIUC, the purpose of selinux_inode_notifysecctx() is
> to adjust the sid that has been assigned by selinux_d_instantiate() by
> the label that is 'stored' for the particular node internally by the
> filesystem. I would say the fact whether we want to use the stored
> label depends on the sbsec->behavior value (BTW, shouldn't we also
> return 0 in case of SECURITY_FS_USE_TASK? or even
> SECURITY_FS_USE_TRANS?). I understand the SBLABEL_MNT flag more as an
> indication of whether we want the user to allow setting the label
> explicitly (and probably also implicitly via tsec->create_sid).

selinux_inode_notifysecctx() provides the filesystem with a way to push 
a label for an inode to the security module as opposed to having the 
security module pull it via __vfs_getxattr.  The latter doesn't work 
well for NFSv4.2 security labeling nor for sysfs, albeit for different 
reasons.

SBLABEL_MNT is not so much whether we want to allow the user to do it 
but rather whether the filesystem and policy make it safe to do so.  It 
is set mostly based on the ->behavior with exceptions for the 
whitelisted filesystem types.  The same flag is checked in 
selinux_inode_init_security(), where we are returning a label for a 
newly created file.

Not sure we want to create two different ways of determining whether the 
filesystem supports labeling.

> 
>> And do we need to separately check SE_SBINITIALIZED?
> 
> I'm not sure, but other places in the code check that flag before
> checking sbsec->behavior, so it seemed to me as the right thing to do.
> 
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.
> 

Re: [RFC PATCH 3/3] selinux: do not override context on context mounts

[Ondrej Mosnacek]

On Tue, Dec 18, 2018 at 8:19 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/18/18 10:50 AM, Ondrej Mosnacek wrote:
> > On Thu, Dec 13, 2018 at 5:25 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> >> On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
> >>> Ignore all selinux_inode_notifysecctx() calls on mounts with the
> >>> SECURITY_FS_USE_MNTPOINT behavior.
> >>>
> >>> 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 a different context.
> >>>
> >>> 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 | 7 +++++++
> >>>    1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index d6d29ec54eab..0ca5ed30afe1 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -6620,6 +6620,13 @@ static void selinux_inode_invalidate_secctx(struct inode *inode)
> >>>     */
> >>>    static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> >>>    {
> >>> +     struct superblock_security_struct *sbsec = inode->i_sb->s_security;
> >>> +
> >>> +     /* Do not change context in SECURITY_FS_USE_MNTPOINT case */
> >>> +     if ((sbsec->flags & SE_SBINITIALIZED) &&
> >>> +         (sbsec->behavior == SECURITY_FS_USE_MNTPOINT))
> >>> +             return 0;
> >>> +
> >>>        return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
> >>>    }
> >>
> >> Wondering if we ought to take this into selinux_inode_setsecurity() and
> >> return -EOPNOTSUPP in that case.  We already return -EOPNOTSUPP from
> >> selinux_inode_setxattr() if (!(sbsec->flags & SBLABEL_MNT)) and that
> >> should precede other calls to selinux_inode_setsecurity() IIRC.
> >
> > Maybe, but see below. In selinux_inode_setsecurity() we should indeed
> > check for SBLABEL_MNT, but only if it is called directly as a hook
> > (but I'm not sure if it is worth it in this case, since as you say, a
> > prior selinux_inode_setxattr() failure should always prevent this hook
> > from being called). selinux_inode_notifysecctx() has a bit different
> > semantics, IMHO.
> >
> >> Should we just be checking SBLABEL_MNT here instead?
> >
> > I don't think so. IIUC, the purpose of selinux_inode_notifysecctx() is
> > to adjust the sid that has been assigned by selinux_d_instantiate() by
> > the label that is 'stored' for the particular node internally by the
> > filesystem. I would say the fact whether we want to use the stored
> > label depends on the sbsec->behavior value (BTW, shouldn't we also
> > return 0 in case of SECURITY_FS_USE_TASK? or even
> > SECURITY_FS_USE_TRANS?). I understand the SBLABEL_MNT flag more as an
> > indication of whether we want the user to allow setting the label
> > explicitly (and probably also implicitly via tsec->create_sid).
>
> selinux_inode_notifysecctx() provides the filesystem with a way to push
> a label for an inode to the security module as opposed to having the
> security module pull it via __vfs_getxattr.  The latter doesn't work
> well for NFSv4.2 security labeling nor for sysfs, albeit for different
> reasons.
>
> SBLABEL_MNT is not so much whether we want to allow the user to do it
> but rather whether the filesystem and policy make it safe to do so.  It
> is set mostly based on the ->behavior with exceptions for the
> whitelisted filesystem types.  The same flag is checked in
> selinux_inode_init_security(), where we are returning a label for a
> newly created file.
>
> Not sure we want to create two different ways of determining whether the
> filesystem supports labeling.
>

All right, I think we can say that any filesystem that does not
support labeling should never call security_inode_notifysecctx(). So
in this case "!(sbsec->flags & SBLABEL_MNT)" should be equivalent to
"sbsec->behavior == SECURITY_FS_USE_MNTPOINT". Considering that, I
don't have any strong arguments against checking SBLABEL_MNT instead
of behavior, so I'll change it to that in v2.

> >
> >> And do we need to separately check SE_SBINITIALIZED?
> >
> > I'm not sure, but other places in the code check that flag before
> > checking sbsec->behavior, so it seemed to me as the right thing to do.
> >
> > --
> > Ondrej Mosnacek <omosnace at redhat dot com>
> > Associate Software Engineer, Security Technologies
> > Red Hat, Inc.
> >

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

Re: [RFC PATCH 1/3] cgroup: fix parsing empty mount option string

[Ondrej Mosnacek]

Hi Tejun,

On Thu, Dec 13, 2018 at 5:03 PM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Thu, Dec 13, 2018 at 03:17:37PM +0100, Ondrej Mosnacek wrote:
> > This fixes the case where all mount options specified are consumed by an
> > LSM and all that's left is an empty string. In this case cgroupfs should
> > accept the string and not fail.
> >
> > How to reproduce (with SELinux enabled):
> >
> >     # umount /sys/fs/cgroup/unified
> >     # mount -o context=system_u:object_r:cgroup_t:s0 -t cgroup2 cgroup2 /sys/fs/cgroup/unified
> >     mount: /sys/fs/cgroup/unified: wrong fs type, bad option, bad superblock on cgroup2, missing codepage or helper program, or other error.
> >     # dmesg | tail -n 1
> >     [   31.575952] cgroup: cgroup2: unknown option ""
> >
> > Fixes: 67e9c74b8a87 ("cgroup: replace __DEVEL__sane_behavior with cgroup2 fs type")
> > [NOTE: should apply on top of commit 5136f6365ce3 ("cgroup: implement "nsdelegate" mount option"), older versions need manual rebase]
> > Suggested-by: Stephen Smalley <sds@tycho.nsa.gov>
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Applied to cgroup/for-4.21.

I still can't see the patch in your for-4.21 branch [1] (and it
doesn't seem to be included in your 4.21-rc1 pull request either [2]).
Did you perhaps forget to apply it?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git/log/?h=for-4.21
[2] https://lore.kernel.org/lkml/20181228021605.GI2509588@devbig004.ftw2.facebook.com/

Cheers,

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

Re: [RFC PATCH 1/3] cgroup: fix parsing empty mount option string

[Tejun Heo]

On Fri, Dec 28, 2018 at 04:14:01PM +0100, Ondrej Mosnacek wrote:
> > Applied to cgroup/for-4.21.
> 
> I still can't see the patch in your for-4.21 branch [1] (and it
> doesn't seem to be included in your 4.21-rc1 pull request either [2]).
> Did you perhaps forget to apply it?

Indeed.  I'll amend the pull request.

Thanks.

-- 
tejun