On Tue, Jan 12, 2021 at 3:47 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > 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. Fair point. After the rename to "fallback" as suggested below it became visually ugly to me as well :) > > > +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? Not really, but it's true that the control flow can be simplified a bit... > 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. Agreed, "fallback" sounds better here. > > > + 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; Again a good point, will be like that in v2. Thank you for the valuable suggestions! > > > + } > > + 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 > -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
