| Message ID | 20181213141739.8534-4-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix SELinux context mount with the cgroup filesystem | expand |
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?
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.
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. >
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. > >
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); }
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(+)