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.
static int sb_finish_set_opts(struct super_block *sb)
