On Thu, Dec 13, 2018 at 5:16 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 12/13/18 9:17 AM, Ondrej Mosnacek wrote:
> > In the SECURITY_FS_USE_MNTPOINT case we never want to allow relabeling
> > files/directories, so we should never set the SBLABEL_MNT flag in this
> > case. The 'special handling' in selinux_is_sblabel_mnt() is only
> > intended for SECURITY_FS_USE_GENFS.
> >
> > While there, make the logic in selinux_is_sblabel_mnt() more explicit
> > and add a BUILD_BUG_ON() to make sure that introducing a new
> > SECURITY_FS_USE_* forces a review of the logic.
> >
> > Note that checkpatch.pl produces some false positives here, likely
> > having problems recognizing the monstrous return statement...
> >
> > Fixes: d5f3a5f6e7e7 ("selinux: add security in-core xattr support for pstore and debugfs")
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > security/selinux/hooks.c | 41 ++++++++++++++++++++++++++++------------
> > 1 file changed, 29 insertions(+), 12 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 7ce012d9ec51..d6d29ec54eab 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -501,19 +501,36 @@ static int selinux_is_sblabel_mnt(struct super_block *sb)
> > {
> > struct superblock_security_struct *sbsec = sb->s_security;
> >
> > - return sbsec->behavior == SECURITY_FS_USE_XATTR ||
> > - sbsec->behavior == SECURITY_FS_USE_TRANS ||
> > - sbsec->behavior == SECURITY_FS_USE_TASK ||
> > - sbsec->behavior == SECURITY_FS_USE_NATIVE ||
> > + /*
> > + * IMPORTANT: Double-check logic in this function when adding a new
> > + * SECURITY_FS_USE_* definition!
> > + */
> > + BUILD_BUG_ON(SECURITY_FS_USE_MAX != 7);
> > +
> > + switch (sbsec->behavior) {
> > + case SECURITY_FS_USE_XATTR:
> > + case SECURITY_FS_USE_TRANS:
> > + case SECURITY_FS_USE_TASK:
> > + case SECURITY_FS_USE_NATIVE:
> > + return 1;
> > +
> > + case SECURITY_FS_USE_GENFS:
> > /* Special handling. Genfs but also in-core setxattr handler */
> > - !strcmp(sb->s_type->name, "sysfs") ||
> > - !strcmp(sb->s_type->name, "pstore") ||
> > - !strcmp(sb->s_type->name, "debugfs") ||
> > - !strcmp(sb->s_type->name, "tracefs") ||
> > - !strcmp(sb->s_type->name, "rootfs") ||
> > - (selinux_policycap_cgroupseclabel() &&
> > - (!strcmp(sb->s_type->name, "cgroup") ||
> > - !strcmp(sb->s_type->name, "cgroup2")));
> > + return !strcmp(sb->s_type->name, "sysfs") ||
> > + !strcmp(sb->s_type->name, "pstore") ||
> > + !strcmp(sb->s_type->name, "debugfs") ||
> > + !strcmp(sb->s_type->name, "tracefs") ||
> > + !strcmp(sb->s_type->name, "rootfs") ||
> > + (selinux_policycap_cgroupseclabel() &&
> > + (!strcmp(sb->s_type->name, "cgroup") ||
> > + !strcmp(sb->s_type->name, "cgroup2")));
> > +
> > + /* Never allow relabeling on context mounts */
> > + case SECURITY_FS_USE_MNTPOINT:
> > + case SECURITY_FS_USE_NONE:
> > + default:
> > + return 0;
> > + }
> > }
>
> This looks sane to me. Note that
> https://github.com/SELinuxProject/selinux-kernel/issues/2 calls for
> replacing the use of hardcoded lists of filesystem types above with
> something more general. Maybe you could at least abstract that part
> into its own function.
Agreed, that's a good idea. It should also allow checkpatch.pl to
correctly parse the control flow and not throw an error.
>
> >
> > static int sb_finish_set_opts(struct super_block *sb)
> >
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
