* [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
* 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
* [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 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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).