On Fri, Dec 21, 2018 at 4:49 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Fri, Dec 21, 2018 at 9:45 PM Stephen Smalley <sds@xxxxxxxxxxxxx> 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
>
> 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
