On Tue, Jan 5, 2021 at 11:28 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > When a superblock is assigned the SECURITY_FS_USE_XATTR behavior by the > policy yet it lacks xattr support, try to fall back to genfs rather than > rejecting the mount. If a genfscon rule is found for the filesystem, > then change the behavior to SECURITY_FS_USE_GENFS, otherwise reject the > mount as before. A similar fallback is already done in security_fs_use() > if no behavior specification is found for the given filesystem. > > This is needed e.g. for virtiofs, which may or may not support xattrs > depending on the backing host filesystem. > > Example: > # seinfo --genfs | grep ' ramfs' > genfscon ramfs / system_u:object_r:ramfs_t:s0 > # echo '(fsuse xattr ramfs (system_u object_r fs_t ((s0) (s0))))' >ramfs_xattr.cil > # semodule -i ramfs_xattr.cil > # mount -t ramfs none /mnt > > Before: > mount: /mnt: mount(2) system call failed: Operation not supported. > > After: > (mount succeeds) > # ls -Z /mnt > system_u:object_r:ramfs_t:s0 /mnt > > See also: > https://lore.kernel.org/selinux/20210105142148.GA3200@xxxxxxxxxx/T/ > https://github.com/fedora-selinux/selinux-policy/pull/478 > > Cc: Vivek Goyal <vgoyal@xxxxxxxxxx> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> > --- > security/selinux/hooks.c | 78 +++++++++++++++++++++++++++------------- > 1 file changed, 53 insertions(+), 25 deletions(-) This generally looks good to me, just some small suggestions below to cleanup the code a bit. > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 6b1826fc3658e..0b9b4050b9315 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -484,38 +484,67 @@ static int selinux_is_sblabel_mnt(struct super_block *sb) > } > } > > -static int sb_finish_set_opts(struct super_block *sb) > +static int sb_check_xattr_support(struct super_block *sb) > { > struct superblock_security_struct *sbsec = sb->s_security; > struct dentry *root = sb->s_root; > struct inode *root_inode = d_backing_inode(root); > - int rc = 0; > + u32 sid; > + int rc; > > - if (sbsec->behavior == SECURITY_FS_USE_XATTR) { > - /* Make sure that the xattr handler exists and that no > - error other than -ENODATA is returned by getxattr on > - the root directory. -ENODATA is ok, as this may be > - the first boot of the SELinux kernel before we have > - assigned xattr values to the filesystem. */ > - if (!(root_inode->i_opflags & IOP_XATTR)) { > - pr_warn("SELinux: (dev %s, type %s) has no " > - "xattr support\n", sb->s_id, sb->s_type->name); > + /* > + * Make sure that the xattr handler exists and that no > + * error other than -ENODATA is returned by getxattr on > + * the root directory. -ENODATA is ok, as this may be > + * the first boot of the SELinux kernel before we have > + * assigned xattr values to the filesystem. > + */ > + if (!(root_inode->i_opflags & IOP_XATTR)) { > + pr_warn("SELinux: (dev %s, type %s) has no xattr support\n", > + sb->s_id, sb->s_type->name); > + rc = -EOPNOTSUPP; > + goto out; > + } > + > + rc = __vfs_getxattr(root, root_inode, XATTR_NAME_SELINUX, NULL, 0); > + if (rc < 0 && rc != -ENODATA) { > + if (rc == -EOPNOTSUPP) > + pr_warn("SELinux: (dev %s, type %s) has no security xattr handler\n", > + sb->s_id, sb->s_type->name); > + else > + pr_warn("SELinux: (dev %s, type %s) getxattr errno %d\n", > + sb->s_id, sb->s_type->name, -rc); > + goto out; > + } > + return 0; This is a borderline absurd nitpick, but a line of vertical whitespace before the jump label would be nice. > +out: > + /* No xattr support - try to fallback to genfs if possible. */ > + if (rc == -EOPNOTSUPP) { I'm not sure the "rc == -EOPNOTSUPP" is necessary is it? The only way we get to the "out" label is if rc *is* -EOPNOTSUPP, right? Somewhat related, since we're talking about minor things, why not change "out" to "fallback" or something similar, "out" isn't really "out" as it is used in this function. > + rc = security_genfs_sid(&selinux_state, sb->s_type->name, "/", > + SECCLASS_DIR, &sid); > + if (rc) { > rc = -EOPNOTSUPP; > - goto out; You might as well just "return -EOPNOSUPP" here. > + } else { > + pr_warn("SELinux: (dev %s, type %s) falling back to genfs\n", > + sb->s_id, sb->s_type->name); > + sbsec->behavior = SECURITY_FS_USE_GENFS; > + sbsec->sid = sid; > } It also might read a bit better if you got rid of the else block, for example: rc = security_genfs_sid(...); if (rc) return -EOPNOTSUPP; pr_warn(...); sbsec->blah = blahblah; return 0; > + } > + return rc; > +} > > - rc = __vfs_getxattr(root, root_inode, XATTR_NAME_SELINUX, NULL, 0); > - if (rc < 0 && rc != -ENODATA) { > - if (rc == -EOPNOTSUPP) > - pr_warn("SELinux: (dev %s, type " > - "%s) has no security xattr handler\n", > - sb->s_id, sb->s_type->name); > - else > - pr_warn("SELinux: (dev %s, type " > - "%s) getxattr errno %d\n", sb->s_id, > - sb->s_type->name, -rc); > - goto out; > - } > +static int sb_finish_set_opts(struct super_block *sb) > +{ > + struct superblock_security_struct *sbsec = sb->s_security; > + struct dentry *root = sb->s_root; > + struct inode *root_inode = d_backing_inode(root); > + int rc = 0; > + > + if (sbsec->behavior == SECURITY_FS_USE_XATTR) { > + rc = sb_check_xattr_support(sb); > + if (rc) > + return rc; > } > > sbsec->flags |= SE_SBINITIALIZED; > @@ -554,7 +583,6 @@ static int sb_finish_set_opts(struct super_block *sb) > spin_lock(&sbsec->isec_lock); > } > spin_unlock(&sbsec->isec_lock); > -out: > return rc; > } > > -- > 2.29.2 > -- paul moore www.paul-moore.com
