Re: [PATCH v5 1/5] selinux: try security xattr after genfs for kernfs filesystems

On Tue, Feb 5, 2019 at 3:23 PM Stephen Smalley [off-list ref] wrote:

On 2/5/19 6:06 AM, Ondrej Mosnacek wrote:

quoted

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 <redacted>
---
  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,

quoted

-     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.