On Tue, Feb 5, 2019 at 3:23 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 2/5/19 6:06 AM, Ondrej Mosnacek wrote:
> > Since kernfs supports the security xattr handlers, we can simply use
> > these to determine the inode's context, dropping the need to update it
> > from kernfs explicitly using a security_inode_notifysecctx() call.
> >
> > We achieve this by setting a new sbsec flag SE_SBGENFS_XATTR to all
> > mounts that are known to use kernfs under the hood and then fetching the
> > xattrs after determining the fallback genfs sid in
> > inode_doinit_with_dentry() when this flag is set.
> >
> > This will allow implementing full security xattr support in kernfs and
> > removing the ...notifysecctx() call in a subsequent patch.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > security/selinux/hooks.c | 159 +++++++++++++++-------------
> > security/selinux/include/security.h | 1 +
> > 2 files changed, 88 insertions(+), 72 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 81e012c66d95..758a99d1086e 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -793,11 +793,13 @@ static int selinux_set_mnt_opts(struct super_block *sb,
> >
> > if (!strcmp(sb->s_type->name, "debugfs") ||
> > !strcmp(sb->s_type->name, "tracefs") ||
> > - !strcmp(sb->s_type->name, "sysfs") ||
> > - !strcmp(sb->s_type->name, "pstore") ||
> > + !strcmp(sb->s_type->name, "pstore"))
> > + sbsec->flags |= SE_SBGENFS;
> > +
> > + if (!strcmp(sb->s_type->name, "sysfs") ||
> > !strcmp(sb->s_type->name, "cgroup") ||
> > !strcmp(sb->s_type->name, "cgroup2"))
> > - sbsec->flags |= SE_SBGENFS;
> > + sbsec->flags |= SE_SBGENFS | SE_SBGENFS_XATTR;
> >
> > if (!sbsec->behavior) {
> > /*
> > @@ -1392,6 +1394,71 @@ static int selinux_genfs_get_sid(struct dentry *dentry,
> > return rc;
> > }
> >
> > +static int inode_doinit_use_xattr(struct inode *inode, struct dentry *dentry,
> > + u32 def_sid, u32 *sid)
> > +{
> > +#define INITCONTEXTLEN 255
> > + char *context = NULL;
> > + unsigned int len = 0;
> > + int rc;
> > +
> > + *sid = def_sid;
> > +
> > + if (!(inode->i_opflags & IOP_XATTR))
> > + return 0;
> > +
> > + len = INITCONTEXTLEN;
> > + context = kmalloc(len + 1, GFP_NOFS);
> > + if (!context)
> > + return -ENOMEM;
> > +
> > + context[len] = '\0';
> > + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len);
> > + if (rc == -ERANGE) {
> > + kfree(context);
> > +
> > + /* Need a larger buffer. Query for the right size. */
> > + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, NULL, 0);
> > + if (rc < 0)
> > + return rc;
> > +
> > + len = rc;
> > + context = kmalloc(len + 1, GFP_NOFS);
> > + if (!context)
> > + return -ENOMEM;
> > +
> > + context[len] = '\0';
> > + rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX,
> > + context, len);
> > + }
> > + if (rc < 0) {
> > + kfree(context);
> > + if (rc != -ENODATA) {
> > + pr_warn("SELinux: %s: getxattr returned %d for dev=%s ino=%ld\n",
> > + __func__, -rc, inode->i_sb->s_id, inode->i_ino);
> > + return rc;
> > + }
> > + return 0;
> > + }
> > +
> > + rc = security_context_to_sid_default(&selinux_state, context, rc, sid,
> > + def_sid, GFP_NOFS);
> > + if (rc) {
> > + char *dev = inode->i_sb->s_id;
> > + unsigned long ino = inode->i_ino;
> > +
> > + if (rc == -EINVAL) {
> > + pr_notice_ratelimited("SELinux: inode=%lu on dev=%s was found to have an invalid context=%s. This indicates you may need to relabel the inode or the filesystem in question.\n",
> > + ino, dev, context);
> > + } else {
> > + pr_warn("SELinux: %s: context_to_sid(%s) returned %d for dev=%s ino=%ld\n",
> > + __func__, context, -rc, dev, ino);
> > + }
> > + }
> > + kfree(context);
> > + return 0;
> > +}
> > +
> > /* The inode's security attributes must be initialized before first use. */
> > static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dentry)
> > {
> > @@ -1401,8 +1468,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
> > u16 sclass;
> > struct dentry *dentry;
> > #define INITCONTEXTLEN 255
>
> This definition can also be removed, having been moved up above.
Right, staged for fixing in v6.
Thanks,
>
> > - char *context = NULL;
> > - unsigned len = 0;
> > int rc = 0;
> >
> > if (isec->initialized == LABEL_INITIALIZED)
> > @@ -1470,72 +1535,11 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
> > goto out;
> > }
> >
> > - len = INITCONTEXTLEN;
> > - context = kmalloc(len+1, GFP_NOFS);
> > - if (!context) {
> > - rc = -ENOMEM;
> > - dput(dentry);
> > - goto out;
> > - }
> > - context[len] = '\0';
> > - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len);
> > - if (rc == -ERANGE) {
> > - kfree(context);
> > -
> > - /* Need a larger buffer. Query for the right size. */
> > - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, NULL, 0);
> > - if (rc < 0) {
> > - dput(dentry);
> > - goto out;
> > - }
> > - len = rc;
> > - context = kmalloc(len+1, GFP_NOFS);
> > - if (!context) {
> > - rc = -ENOMEM;
> > - dput(dentry);
> > - goto out;
> > - }
> > - context[len] = '\0';
> > - rc = __vfs_getxattr(dentry, inode, XATTR_NAME_SELINUX, context, len);
> > - }
> > + rc = inode_doinit_use_xattr(inode, dentry, sbsec->def_sid,
> > + &sid);
> > dput(dentry);
> > - if (rc < 0) {
> > - if (rc != -ENODATA) {
> > - pr_warn("SELinux: %s: getxattr returned "
> > - "%d for dev=%s ino=%ld\n", __func__,
> > - -rc, inode->i_sb->s_id, inode->i_ino);
> > - kfree(context);
> > - goto out;
> > - }
> > - /* Map ENODATA to the default file SID */
> > - sid = sbsec->def_sid;
> > - rc = 0;
> > - } else {
> > - rc = security_context_to_sid_default(&selinux_state,
> > - context, rc, &sid,
> > - sbsec->def_sid,
> > - GFP_NOFS);
> > - if (rc) {
> > - char *dev = inode->i_sb->s_id;
> > - unsigned long ino = inode->i_ino;
> > -
> > - if (rc == -EINVAL) {
> > - if (printk_ratelimit())
> > - pr_notice("SELinux: inode=%lu on dev=%s was found to have an invalid "
> > - "context=%s. This indicates you may need to relabel the inode or the "
> > - "filesystem in question.\n", ino, dev, context);
> > - } else {
> > - pr_warn("SELinux: %s: context_to_sid(%s) "
> > - "returned %d for dev=%s ino=%ld\n",
> > - __func__, context, -rc, dev, ino);
> > - }
> > - kfree(context);
> > - /* Leave with the unlabeled SID */
> > - rc = 0;
> > - break;
> > - }
> > - }
> > - kfree(context);
> > + if (rc)
> > + goto out;
> > break;
> > case SECURITY_FS_USE_TASK:
> > sid = task_sid;
> > @@ -1586,9 +1590,20 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
> > goto out;
> > rc = selinux_genfs_get_sid(dentry, sclass,
> > sbsec->flags, &sid);
> > - dput(dentry);
> > - if (rc)
> > + if (rc) {
> > + dput(dentry);
> > goto out;
> > + }
> > +
> > + if (sbsec->flags & SE_SBGENFS_XATTR) {
> > + rc = inode_doinit_use_xattr(inode, dentry,
> > + sid, &sid);
> > + if (rc) {
> > + dput(dentry);
> > + goto out;
> > + }
> > + }
> > + dput(dentry);
> > }
> > break;
> > }
> > diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> > index f68fb25b5702..6e5928f951da 100644
> > --- a/security/selinux/include/security.h
> > +++ b/security/selinux/include/security.h
> > @@ -58,6 +58,7 @@
> > #define SE_SBINITIALIZED 0x0100
> > #define SE_SBPROC 0x0200
> > #define SE_SBGENFS 0x0400
> > +#define SE_SBGENFS_XATTR 0x0800
> >
> > #define CONTEXT_STR "context="
> > #define FSCONTEXT_STR "fscontext="
> >
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
