* [PATCH v2 0/2] Fix SELinux context mount with the cgroup filesystem
@ 2018-12-21 20:18 Ondrej Mosnacek
2018-12-21 20:18 ` [PATCH v2 1/2] selinux: never allow relabeling on context mounts Ondrej Mosnacek
2018-12-21 20:18 ` [PATCH v2 2/2] selinux: do not override context " Ondrej Mosnacek
0 siblings, 2 replies; 8+ messages in thread
From: Ondrej Mosnacek @ 2018-12-21 20:18 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] selinux: never allow relabeling on context mounts
2018-12-21 20:18 [PATCH v2 0/2] Fix SELinux context mount with the cgroup filesystem Ondrej Mosnacek
@ 2018-12-21 20:18 ` Ondrej Mosnacek
2018-12-21 20:42 ` Stephen Smalley
2018-12-21 20:18 ` [PATCH v2 2/2] selinux: do not override context " Ondrej Mosnacek
1 sibling, 1 reply; 8+ messages in thread
From: Ondrej Mosnacek @ 2018-12-21 20:18 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] selinux: do not override context on context mounts
2018-12-21 20:18 [PATCH v2 0/2] Fix SELinux context mount with the cgroup filesystem Ondrej Mosnacek
2018-12-21 20:18 ` [PATCH v2 1/2] selinux: never allow relabeling on context mounts Ondrej Mosnacek
@ 2018-12-21 20:18 ` Ondrej Mosnacek
2018-12-21 20:47 ` Stephen Smalley
1 sibling, 1 reply; 8+ messages in thread
From: Ondrej Mosnacek @ 2018-12-21 20:18 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] selinux: never allow relabeling on context mounts
2018-12-21 20:18 ` [PATCH v2 1/2] selinux: never allow relabeling on context mounts Ondrej Mosnacek
@ 2018-12-21 20:42 ` Stephen Smalley
0 siblings, 0 replies; 8+ messages in thread
From: Stephen Smalley @ 2018-12-21 20:42 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
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;
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] selinux: do not override context on context mounts
2018-12-21 20:18 ` [PATCH v2 2/2] selinux: do not override context " Ondrej Mosnacek
@ 2018-12-21 20:47 ` Stephen Smalley
2018-12-21 21:49 ` Ondrej Mosnacek
0 siblings, 1 reply; 8+ messages in thread
From: Stephen Smalley @ 2018-12-21 20:47 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
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;
> }
>
> /*
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] selinux: do not override context on context mounts
2018-12-21 20:47 ` Stephen Smalley
@ 2018-12-21 21:49 ` Ondrej Mosnacek
2018-12-21 22:59 ` Paul Moore
0 siblings, 1 reply; 8+ messages in thread
From: Ondrej Mosnacek @ 2018-12-21 21:49 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux, Paul Moore
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] selinux: do not override context on context mounts
2018-12-21 21:49 ` Ondrej Mosnacek
@ 2018-12-21 22:59 ` Paul Moore
2019-01-11 2:28 ` Paul Moore
0 siblings, 1 reply; 8+ messages in thread
From: Paul Moore @ 2018-12-21 22:59 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: Stephen Smalley, selinux
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] selinux: do not override context on context mounts
2018-12-21 22:59 ` Paul Moore
@ 2019-01-11 2:28 ` Paul Moore
0 siblings, 0 replies; 8+ messages in thread
From: Paul Moore @ 2019-01-11 2:28 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: Stephen Smalley, selinux
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-11 2:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-21 20:18 [PATCH v2 0/2] Fix SELinux context mount with the cgroup filesystem Ondrej Mosnacek
2018-12-21 20:18 ` [PATCH v2 1/2] selinux: never allow relabeling on context mounts Ondrej Mosnacek
2018-12-21 20:42 ` Stephen Smalley
2018-12-21 20:18 ` [PATCH v2 2/2] selinux: do not override context " Ondrej Mosnacek
2018-12-21 20:47 ` Stephen Smalley
2018-12-21 21:49 ` Ondrej Mosnacek
2018-12-21 22:59 ` Paul Moore
2019-01-11 2:28 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.