| Message ID | 20210105162830.1037459-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: fall back to SECURITY_FS_USE_GENFS if no xattr support | expand |
On Tue, Jan 5, 2021 at 11:28 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > https://github.com/fedora-selinux/selinux-policy/pull/478 > > Cc: Vivek Goyal <vgoyal@redhat.com> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > 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
On Tue, Jan 12, 2021 at 3:47 PM Paul Moore <paul@paul-moore.com> wrote: > On Tue, Jan 5, 2021 at 11:28 AM Ondrej Mosnacek <omosnace@redhat.com> 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@redhat.com/T/ > > https://github.com/fedora-selinux/selinux-policy/pull/478 > > > > Cc: Vivek Goyal <vgoyal@redhat.com> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > 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 >
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; +out: + /* No xattr support - try to fallback to genfs if possible. */ + if (rc == -EOPNOTSUPP) { + rc = security_genfs_sid(&selinux_state, sb->s_type->name, "/", + SECCLASS_DIR, &sid); + if (rc) { rc = -EOPNOTSUPP; - goto out; + } 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; } + } + 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; }
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@redhat.com/T/ https://github.com/fedora-selinux/selinux-policy/pull/478 Cc: Vivek Goyal <vgoyal@redhat.com> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/hooks.c | 78 +++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 25 deletions(-)