[PATCH v6 0/5] Allow initializing the kernfs node's secctx based on its parent

[PATCH v6 0/5] Allow initializing the kernfs node's secctx based on its parent

[Ondrej Mosnacek]

Changes in v6:
- remove copy-pasted duplicate macro definition

v5: https://lore.kernel.org/selinux/20190205110638.30782-1-omosnace@redhat.com/T/
Changes in v5:
- fix misplaced semicolon detected by 0day robot

v4: https://lore.kernel.org/selinux/20190205085915.5183-1-omosnace@redhat.com/T/
Changes in v4:
- reorder and rename hook arguments
- avoid allocating kernfs_iattrs unless needed

v3: https://lore.kernel.org/selinux/20190130114150.27807-1-omosnace@redhat.com/T/
Changes in v3:
- rename the hook to "kernfs_init_security"
- change the hook interface to simply pass pointers to struct iattr and
  struct simple_xattrs of both the new node and its parent
- add full security xattr support to kernfs (and fixup SELinux behavior
  to handle it properly)

v2: https://lore.kernel.org/selinux/20190109162830.8309-1-omosnace@redhat.com/T/
Changes in v2:
- add docstring for the new hook in union security_list_options
- initialize *ctx to NULL and *ctxlen to 0 in case the hook is not
  implemented

v1: https://lore.kernel.org/selinux/20190109091028.24485-1-omosnace@redhat.com/T/

TL;DR:
This series adds a new security hook that allows to initialize the security
context of kernfs properly, taking into account the parent context (and
possibly other attributes). Kernfs nodes require special handling here, since
they are not bound to specific inodes/superblocks, but instead represent the
backing tree structure that is used to build the VFS tree when the kernfs
tree is mounted.

The kernfs nodes initially do not store any security context and rely on
the LSM to assign some default context to inodes created over them. Kernfs
inodes, however, allow setting an explicit context via the *setxattr(2)
syscalls, in which case the context is stored inside the kernfs node's
internal structure.

SELinux (and possibly other LSMs) initialize the context of newly created
FS objects based on the parent object's context (usually the child inherits
the parent's context, unless the policy dictates otherwise). This is done
by hooking the creation of the new inode corresponding to the newly created
file/directory via security_inode_init_security() (most filesystems always
create a fresh inode when a new FS object is created). However, kernfs nodes
can be created "behind the scenes" while the filesystem is not mounted
anywhere and thus no inodes can exist for them yet.

Therefore, to allow maintaining similar behavior for kernfs nodes, a new
LSM hook is needed, which will allow initializing the kernfs node's
security context based on its own attributes and those of the parent's
node.

The main motivation for this change is that the userspace users of cgroupfs
(which is built on kernfs) expect the usual security context inheritance
to work under SELinux (see [1] and [2]). This functionality is required for
better confinement of containers under SELinux.

Patch 1/5 changes SELinux to fetch security context from extended
attributes on kernfs filesystems, falling back to genfs-defined context
if that fails. Without this patch the 2/5 would be a regression for
SELinux (due to the removal of ...notifysecctx() call.

Patch 2/5 implements full security xattr support in kernfs using
simple_xattrs; patch 3/5 adds the new LSM hook; patch 4/5 implements the
new hook in SELinux; and patch 5/5 modifies kernfs to call the new hook
on new node creation.

Testing:
- passed the reproducer from the commit message of the last patch
- passed SELinux testsuite on Fedora 29 (x86_64) when applied on top of
  current Rawhide kernel (5.0.0-0.rc5.git0.1) [3]
  - including the new proposed selinux-testsuite subtest [4] (adapted
    from the reproducer)

[1] https://github.com/SELinuxProject/selinux-kernel/issues/39
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
[3] https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/854148/
[4] https://github.com/SELinuxProject/selinux-testsuite/pull/48

Ondrej Mosnacek (5):
  selinux: try security xattr after genfs for kernfs filesystems
  kernfs: use simple_xattrs for security attributes
  LSM: add new hook for kernfs node initialization
  selinux: implement the kernfs_init_security hook
  kernfs: initialize security of newly created nodes

 fs/kernfs/dir.c                     |  64 +++++++-
 fs/kernfs/inode.c                   | 125 +++++++---------
 fs/kernfs/kernfs-internal.h         |   7 +-
 include/linux/lsm_hooks.h           |  22 +++
 include/linux/security.h            |  14 ++
 include/linux/xattr.h               |  15 ++
 security/security.c                 |  10 ++
 security/selinux/hooks.c            | 222 +++++++++++++++++++---------
 security/selinux/include/security.h |   1 +
 9 files changed, 328 insertions(+), 152 deletions(-)

-- 
2.20.1

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

[Ondrej Mosnacek]

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@redhat.com>
---
 security/selinux/hooks.c            | 160 +++++++++++++++-------------
 security/selinux/include/security.h |   1 +
 2 files changed, 88 insertions(+), 73 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 81e012c66d95..7dea5b1a89a3 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)
 {
@@ -1400,9 +1467,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 	u32 task_sid, sid = 0;
 	u16 sclass;
 	struct dentry *dentry;
-#define INITCONTEXTLEN 255
-	char *context = NULL;
-	unsigned len = 0;
 	int rc = 0;
 
 	if (isec->initialized == LABEL_INITIALIZED)
@@ -1470,72 +1534,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 +1589,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="
-- 
2.20.1

[PATCH v6 2/5] kernfs: use simple_xattrs for security attributes

[Ondrej Mosnacek]

Replace the special handling of security xattrs with simple_xattrs, as
is already done for the trusted xattrs. This simplifies the code and
allows LSMs to use more than just a single xattr to do their business.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/kernfs/dir.c             |   7 ++-
 fs/kernfs/inode.c           | 100 +++++++++++++++---------------------
 fs/kernfs/kernfs-internal.h |   5 +-
 3 files changed, 46 insertions(+), 66 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 4ca0b5c18192..ad7e3356bcc5 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -532,11 +532,10 @@ void kernfs_put(struct kernfs_node *kn)
 	kfree_const(kn->name);
 
 	if (kn->iattr) {
-		if (kn->iattr->ia_secdata)
-			security_release_secctx(kn->iattr->ia_secdata,
-						kn->iattr->ia_secdata_len);
-		simple_xattrs_free(&kn->iattr->xattrs);
+		simple_xattrs_free(&kn->iattr->xattrs_trusted);
+		simple_xattrs_free(&kn->iattr->xattrs_security);
 	}
+
 	kfree(kn->iattr);
 	spin_lock(&kernfs_idr_lock);
 	idr_remove(&root->ino_idr, kn->id.ino);
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index 80cebcd94c90..f0e2cb4379c0 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -56,7 +56,8 @@ static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
 	iattrs->ia_mtime = iattrs->ia_atime;
 	iattrs->ia_ctime = iattrs->ia_atime;
 
-	simple_xattrs_init(&kn->iattr->xattrs);
+	simple_xattrs_init(&kn->iattr->xattrs_trusted);
+	simple_xattrs_init(&kn->iattr->xattrs_security);
 out_unlock:
 	ret = kn->iattr;
 	mutex_unlock(&iattr_mutex);
@@ -135,33 +136,31 @@ out:
 	return error;
 }
 
-static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
-				  u32 *secdata_len)
-{
-	void *old_secdata;
-	size_t old_secdata_len;
-
-	old_secdata = attrs->ia_secdata;
-	old_secdata_len = attrs->ia_secdata_len;
-
-	attrs->ia_secdata = *secdata;
-	attrs->ia_secdata_len = *secdata_len;
-
-	*secdata = old_secdata;
-	*secdata_len = old_secdata_len;
-	return 0;
-}
-
 ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size)
 {
 	struct kernfs_node *kn = kernfs_dentry_node(dentry);
+	struct inode *inode = d_inode(dentry);
 	struct kernfs_iattrs *attrs;
+	ssize_t ret, length = 0;
 
 	attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
-	return simple_xattr_list(d_inode(dentry), &attrs->xattrs, buf, size);
+	ret = simple_xattr_list(inode, &attrs->xattrs_trusted, buf, size);
+	if (ret < 0)
+		return ret;
+	length += ret;
+
+	buf += ret;
+	size -= ret;
+
+	ret = simple_xattr_list(inode, &attrs->xattrs_security, buf, size);
+	if (ret < 0)
+		return ret;
+	length += ret;
+
+	return length;
 }
 
 static inline void set_default_inode_attr(struct inode *inode, umode_t mode)
@@ -186,15 +185,12 @@ static void kernfs_refresh_inode(struct kernfs_node *kn, struct inode *inode)
 	struct kernfs_iattrs *attrs = kn->iattr;
 
 	inode->i_mode = kn->mode;
-	if (attrs) {
+	if (attrs)
 		/*
 		 * kernfs_node has non-default attributes get them from
 		 * persistent copy in kernfs_node.
 		 */
 		set_inode_attr(inode, &attrs->ia_iattr);
-		security_inode_notifysecctx(inode, attrs->ia_secdata,
-					    attrs->ia_secdata_len);
-	}
 
 	if (kernfs_type(kn) == KERNFS_DIR)
 		set_nlink(inode, kn->dir.subdirs + 2);
@@ -305,19 +301,29 @@ int kernfs_iop_permission(struct inode *inode, int mask)
 	return generic_permission(inode, mask);
 }
 
+static const struct xattr_handler kernfs_trusted_xattr_handler;
+static const struct xattr_handler kernfs_security_xattr_handler;
+
 static int kernfs_xattr_get(const struct xattr_handler *handler,
 			    struct dentry *unused, struct inode *inode,
 			    const char *suffix, void *value, size_t size)
 {
-	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 	struct kernfs_iattrs *attrs;
+	struct simple_xattrs *xattrs;
 
 	attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
-	return simple_xattr_get(&attrs->xattrs, name, value, size);
+	if (handler == &kernfs_trusted_xattr_handler)
+		xattrs = &attrs->xattrs_trusted;
+	else if (handler == &kernfs_security_xattr_handler)
+		xattrs = &attrs->xattrs_security;
+	else
+		return -EINVAL;
+
+	return simple_xattr_get(xattrs, suffix, value, size);
 }
 
 static int kernfs_xattr_set(const struct xattr_handler *handler,
@@ -325,15 +331,22 @@ static int kernfs_xattr_set(const struct xattr_handler *handler,
 			    const char *suffix, const void *value,
 			    size_t size, int flags)
 {
-	const char *name = xattr_full_name(handler, suffix);
 	struct kernfs_node *kn = inode->i_private;
 	struct kernfs_iattrs *attrs;
+	struct simple_xattrs *xattrs;
 
 	attrs = kernfs_iattrs(kn);
 	if (!attrs)
 		return -ENOMEM;
 
-	return simple_xattr_set(&attrs->xattrs, name, value, size, flags);
+	if (handler == &kernfs_trusted_xattr_handler)
+		xattrs = &attrs->xattrs_trusted;
+	else if (handler == &kernfs_security_xattr_handler)
+		xattrs = &attrs->xattrs_security;
+	else
+		return -EINVAL;
+
+	return simple_xattr_set(xattrs, suffix, value, size, flags);
 }
 
 static const struct xattr_handler kernfs_trusted_xattr_handler = {
@@ -342,41 +355,10 @@ static const struct xattr_handler kernfs_trusted_xattr_handler = {
 	.set = kernfs_xattr_set,
 };
 
-static int kernfs_security_xattr_set(const struct xattr_handler *handler,
-				     struct dentry *unused, struct inode *inode,
-				     const char *suffix, const void *value,
-				     size_t size, int flags)
-{
-	struct kernfs_node *kn = inode->i_private;
-	struct kernfs_iattrs *attrs;
-	void *secdata;
-	u32 secdata_len = 0;
-	int error;
-
-	attrs = kernfs_iattrs(kn);
-	if (!attrs)
-		return -ENOMEM;
-
-	error = security_inode_setsecurity(inode, suffix, value, size, flags);
-	if (error)
-		return error;
-	error = security_inode_getsecctx(inode, &secdata, &secdata_len);
-	if (error)
-		return error;
-
-	mutex_lock(&kernfs_mutex);
-	error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
-	mutex_unlock(&kernfs_mutex);
-
-	if (secdata)
-		security_release_secctx(secdata, secdata_len);
-	return error;
-}
-
 static const struct xattr_handler kernfs_security_xattr_handler = {
 	.prefix = XATTR_SECURITY_PREFIX,
 	.get = kernfs_xattr_get,
-	.set = kernfs_security_xattr_set,
+	.set = kernfs_xattr_set,
 };
 
 const struct xattr_handler *kernfs_xattr_handlers[] = {
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 3d83b114bb08..93bf1dcd0306 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -20,10 +20,9 @@
 
 struct kernfs_iattrs {
 	struct iattr		ia_iattr;
-	void			*ia_secdata;
-	u32			ia_secdata_len;
 
-	struct simple_xattrs	xattrs;
+	struct simple_xattrs	xattrs_trusted;
+	struct simple_xattrs	xattrs_security;
 };
 
 /* +1 to avoid triggering overflow warning when negating it */
-- 
2.20.1

[PATCH v6 3/5] LSM: add new hook for kernfs node initialization

[Ondrej Mosnacek]

This patch introduces a new security hook that is intended for
initializing the security data for newly created kernfs nodes, which
provide a way of storing a non-default security context, but need to
operate independently from mounts (and therefore may not have an
associated inode at the moment of creation).

The main motivation is to allow kernfs nodes to inherit the context of
the parent under SELinux, similar to the behavior of
security_inode_init_security(). Other LSMs may implement their own logic
for handling the creation of new nodes.

The interface of the new hook provides the following to the LSM:
 * a qstr containing the name of the new node
 * inode attributes of the parent node (directory)
 * initial inode attributes (struct iattr + simple_xattrs) of the new
   node

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/linux/lsm_hooks.h | 22 ++++++++++++++++++++++
 include/linux/security.h  | 14 ++++++++++++++
 security/security.c       | 10 ++++++++++
 3 files changed, 46 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9a0bdf91e646..558bbc0ff125 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -429,6 +429,21 @@
  *	to abort the copy up. Note that the caller is responsible for reading
  *	and writing the xattrs as this hook is merely a filter.
  *
+ * Security hooks for kernfs node operations
+ *
+ * @kernfs_init_security
+ *	Initialize the security context of a newlycreated kernfs node based
+ *	on its own and its parent's attributes. The security context (or other
+ *	LSM metadata) should be stored in @secattr as extended attributes.
+ *	The hook MAY NOT add/modify attributes in @dir_secattr; it should be
+ *	treated as a read-only list of attributes.
+ *
+ *	@qstr contains the last path component of the new node.
+ *	@dir_iattr contains the inode attributes of the parent node.
+ *	@dir_secattr is the list of security xattrs of the parent node.
+ *	@iattr contains the inode attributes of the new node.
+ *	@secattr is the list of security xattrs of the new node.
+ *
  * Security hooks for file operations
  *
  * @file_permission:
@@ -1558,6 +1573,12 @@ union security_list_options {
 	int (*inode_copy_up)(struct dentry *src, struct cred **new);
 	int (*inode_copy_up_xattr)(const char *name);
 
+	int (*kernfs_init_security)(const struct qstr *qstr,
+				    const struct iattr *dir_iattr,
+				    struct simple_xattrs *dir_secattr,
+				    const struct iattr *iattr,
+				    struct simple_xattrs *secattr);
+
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
 	void (*file_free_security)(struct file *file);
@@ -1858,6 +1879,7 @@ struct security_hook_heads {
 	struct hlist_head inode_getsecid;
 	struct hlist_head inode_copy_up;
 	struct hlist_head inode_copy_up_xattr;
+	struct hlist_head kernfs_init_security;
 	struct hlist_head file_permission;
 	struct hlist_head file_alloc_security;
 	struct hlist_head file_free_security;
diff --git a/include/linux/security.h b/include/linux/security.h
index dbfb5a66babb..581944d1e61e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -51,6 +51,7 @@ struct fown_struct;
 struct file_operations;
 struct msg_msg;
 struct xattr;
+struct simple_xattrs;
 struct xfrm_sec_ctx;
 struct mm_struct;
 
@@ -291,6 +292,11 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer
 void security_inode_getsecid(struct inode *inode, u32 *secid);
 int security_inode_copy_up(struct dentry *src, struct cred **new);
 int security_inode_copy_up_xattr(const char *name);
+int security_kernfs_init_security(const struct qstr *qstr,
+				  const struct iattr *dir_iattr,
+				  struct simple_xattrs *dir_secattr,
+				  const struct iattr *iattr,
+				  struct simple_xattrs *secattr);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -783,6 +789,14 @@ static inline int security_inode_copy_up(struct dentry *src, struct cred **new)
 	return 0;
 }
 
+static inline int security_kernfs_init_security(
+		const struct qstr *qstr, const struct iattr *dir_iattr,
+		struct simple_xattrs *dir_secattr, const struct iattr *iattr,
+		struct simple_xattrs *secattr)
+{
+	return 0;
+}
+
 static inline int security_inode_copy_up_xattr(const char *name)
 {
 	return -EOPNOTSUPP;
diff --git a/security/security.c b/security/security.c
index f1b8d2587639..836e0822874a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -892,6 +892,16 @@ int security_inode_copy_up_xattr(const char *name)
 }
 EXPORT_SYMBOL(security_inode_copy_up_xattr);
 
+int security_kernfs_init_security(const struct qstr *qstr,
+				  const struct iattr *dir_iattr,
+				  struct simple_xattrs *dir_secattr,
+				  const struct iattr *iattr,
+				  struct simple_xattrs *secattr)
+{
+	return call_int_hook(kernfs_init_security, 0, qstr, dir_iattr,
+			     dir_secattr, iattr, secattr);
+}
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
-- 
2.20.1

[PATCH v6 4/5] selinux: implement the kernfs_init_security hook

[Ondrej Mosnacek]

The hook applies the same logic as selinux_determine_inode_label(), with
the exception of the super_block handling, which will be enforced on the
actual inodes later by other hooks.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/hooks.c | 62 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7dea5b1a89a3..2b72aa50392d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3353,6 +3353,66 @@ static int selinux_inode_copy_up_xattr(const char *name)
 	return -EOPNOTSUPP;
 }
 
+/* kernfs node operations */
+
+int selinux_kernfs_init_security(const struct qstr *qstr,
+				 const struct iattr *dir_iattr,
+				 struct simple_xattrs *dir_secattr,
+				 const struct iattr *iattr,
+				 struct simple_xattrs *secattr)
+{
+	const struct task_security_struct *tsec = current_security();
+	u32 parent_sid, newsid, clen;
+	int rc;
+	char *context;
+
+	rc = simple_xattr_get(dir_secattr, XATTR_SELINUX_SUFFIX, NULL, 0);
+	if (rc == -ENODATA)
+		return 0;
+	else if (rc < 0)
+		return rc;
+
+	clen = (u32)rc;
+	context = kmalloc(clen, GFP_KERNEL);
+	if (!context)
+		return -ENOMEM;
+
+	rc = simple_xattr_get(dir_secattr, XATTR_SELINUX_SUFFIX, context, clen);
+	if (rc < 0) {
+		kfree(context);
+		return rc;
+	}
+
+	rc = security_context_to_sid(&selinux_state, context, clen, &parent_sid,
+				     GFP_KERNEL);
+	kfree(context);
+	if (rc)
+		return rc;
+
+	if (tsec->create_sid) {
+		newsid = tsec->create_sid;
+	} else {
+		u16 secclass = inode_mode_to_security_class(iattr->ia_mode);
+
+		rc = security_transition_sid(&selinux_state, tsec->sid,
+					     parent_sid, secclass, qstr,
+					     &newsid);
+		if (rc)
+			return rc;
+	}
+
+	rc = security_sid_to_context_force(&selinux_state, newsid,
+					   &context, &clen);
+	if (rc)
+		return rc;
+
+	rc = simple_xattr_set(secattr, XATTR_SELINUX_SUFFIX, context, clen,
+			      XATTR_CREATE);
+	kfree(context);
+	return rc;
+}
+
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -6799,6 +6859,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up),
 	LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr),
 
+	LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security),
+
 	LSM_HOOK_INIT(file_permission, selinux_file_permission),
 	LSM_HOOK_INIT(file_alloc_security, selinux_file_alloc_security),
 	LSM_HOOK_INIT(file_free_security, selinux_file_free_security),
-- 
2.20.1

[PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Ondrej Mosnacek]

Use the new security_kernfs_init_security() hook to allow LSMs to
possibly assign a non-default security context to a newly created kernfs
node based on the attributes of the new node and also its parent node.

This fixes an issue with cgroupfs under SELinux, where newly created
cgroup subdirectories/files would not inherit its parent's context if
it had been set explicitly to a non-default value (other than the genfs
context specified by the policy). This can be reproduced as follows (on
Fedora/RHEL):

    # mkdir /sys/fs/cgroup/unified/test
    # # Need permissive to change the label under Fedora policy:
    # setenforce 0
    # chcon -t container_file_t /sys/fs/cgroup/unified/test
    # ls -lZ /sys/fs/cgroup/unified
    total 0
    -r--r--r--.  1 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 cgroup.controllers
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 cgroup.max.depth
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 cgroup.max.descendants
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 cgroup.procs
    -r--r--r--.  1 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 cgroup.stat
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 cgroup.subtree_control
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 cgroup.threads
    drwxr-xr-x.  2 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 init.scope
    drwxr-xr-x. 26 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:21 system.slice
    drwxr-xr-x.  3 root root system_u:object_r:container_file_t:s0 0 Jan 29 03:15 test
    drwxr-xr-x.  3 root root system_u:object_r:cgroup_t:s0         0 Jan 29 03:06 user.slice
    # mkdir /sys/fs/cgroup/unified/test/subdir

Actual result:

    # ls -ldZ /sys/fs/cgroup/unified/test/subdir
    drwxr-xr-x. 2 root root system_u:object_r:cgroup_t:s0 0 Jan 29 03:15 /sys/fs/cgroup/unified/test/subdir

Expected result:

    # ls -ldZ /sys/fs/cgroup/unified/test/subdir
    drwxr-xr-x. 2 root root unconfined_u:object_r:container_file_t:s0 0 Jan 29 03:15 /sys/fs/cgroup/unified/test/subdir

Link: https://github.com/SELinuxProject/selinux-kernel/issues/39
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/kernfs/dir.c             | 57 +++++++++++++++++++++++++++++++++++--
 fs/kernfs/inode.c           | 25 +++++++++-------
 fs/kernfs/kernfs-internal.h |  2 ++
 include/linux/xattr.h       | 15 ++++++++++
 4 files changed, 86 insertions(+), 13 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index ad7e3356bcc5..735a6d382d9d 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/security.h>
 #include <linux/hash.h>
+#include <linux/stringhash.h>
 
 #include "kernfs-internal.h"
 
@@ -616,7 +617,53 @@ struct kernfs_node *kernfs_node_from_dentry(struct dentry *dentry)
 	return NULL;
 }
 
+static int kernfs_node_init_security(struct kernfs_node *parent,
+				     struct kernfs_node *kn)
+{
+	struct simple_xattrs xattr_child, xattr_parent, *pxattr_parent;
+	struct iattr iattr_child, iattr_parent, *piattr_parent;
+	struct qstr q;
+	int ret;
+
+	if (!parent->iattr) {
+		kernfs_iattr_init(&iattr_parent, parent);
+		simple_xattrs_init(&xattr_parent);
+		piattr_parent = &iattr_parent;
+		pxattr_parent = &xattr_parent;
+	} else {
+		piattr_parent = &parent->iattr->ia_iattr;
+		pxattr_parent = &parent->iattr->xattrs_security;
+	}
+
+	kernfs_iattr_init(&iattr_child, kn);
+	simple_xattrs_init(&xattr_child);
+
+	q.name = kn->name;
+	q.hash_len = hashlen_string(parent, kn->name);
+
+	ret = security_kernfs_init_security(&q, piattr_parent, pxattr_parent,
+					    &iattr_child, &xattr_child);
+	if (pxattr_parent == &xattr_parent)
+		simple_xattrs_free(&xattr_parent);
+	if (!ret && !simple_xattrs_empty(&xattr_child)) {
+		/*
+		 * Child has new security xattrs, allocate its kernfs_iattrs
+		 * and put our local xattrs in there.
+		 */
+		struct kernfs_iattrs *attrs = kernfs_iattrs(kn);
+
+		if (!attrs) {
+			simple_xattrs_free(&xattr_child);
+			return -ENOMEM;
+		}
+		simple_xattrs_move(&attrs->xattrs_security, &xattr_child);
+	}
+	simple_xattrs_free(&xattr_child);
+	return ret;
+}
+
 static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
+					     struct kernfs_node *parent,
 					     const char *name, umode_t mode,
 					     kuid_t uid, kgid_t gid,
 					     unsigned flags)
@@ -673,6 +720,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
 			goto err_out3;
 	}
 
+	if (parent) {
+		ret = kernfs_node_init_security(parent, kn);
+		if (ret)
+			goto err_out3;
+	}
+
 	return kn;
 
  err_out3:
@@ -691,7 +744,7 @@ struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
 {
 	struct kernfs_node *kn;
 
-	kn = __kernfs_new_node(kernfs_root(parent),
+	kn = __kernfs_new_node(kernfs_root(parent), parent,
 			       name, mode, uid, gid, flags);
 	if (kn) {
 		kernfs_get(parent);
@@ -961,7 +1014,7 @@ struct kernfs_root *kernfs_create_root(struct kernfs_syscall_ops *scops,
 	INIT_LIST_HEAD(&root->supers);
 	root->next_generation = 1;
 
-	kn = __kernfs_new_node(root, "", S_IFDIR | S_IRUGO | S_IXUGO,
+	kn = __kernfs_new_node(root, NULL, "", S_IFDIR | S_IRUGO | S_IXUGO,
 			       GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
 			       KERNFS_DIR);
 	if (!kn) {
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index f0e2cb4379c0..6a9084aecbe5 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -31,11 +31,22 @@ static const struct inode_operations kernfs_iops = {
 	.listxattr	= kernfs_iop_listxattr,
 };
 
-static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
+void kernfs_iattr_init(struct iattr *iattrs, struct kernfs_node *kn)
+{
+	/* assign default attributes */
+	iattrs->ia_mode = kn->mode;
+	iattrs->ia_uid = GLOBAL_ROOT_UID;
+	iattrs->ia_gid = GLOBAL_ROOT_GID;
+
+	ktime_get_real_ts64(&iattrs->ia_atime);
+	iattrs->ia_mtime = iattrs->ia_atime;
+	iattrs->ia_ctime = iattrs->ia_atime;
+}
+
+struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
 {
 	static DEFINE_MUTEX(iattr_mutex);
 	struct kernfs_iattrs *ret;
-	struct iattr *iattrs;
 
 	mutex_lock(&iattr_mutex);
 
@@ -45,16 +56,8 @@ static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
 	kn->iattr = kzalloc(sizeof(struct kernfs_iattrs), GFP_KERNEL);
 	if (!kn->iattr)
 		goto out_unlock;
-	iattrs = &kn->iattr->ia_iattr;
-
-	/* assign default attributes */
-	iattrs->ia_mode = kn->mode;
-	iattrs->ia_uid = GLOBAL_ROOT_UID;
-	iattrs->ia_gid = GLOBAL_ROOT_GID;
 
-	ktime_get_real_ts64(&iattrs->ia_atime);
-	iattrs->ia_mtime = iattrs->ia_atime;
-	iattrs->ia_ctime = iattrs->ia_atime;
+	kernfs_iattr_init(&kn->iattr->ia_iattr, kn);
 
 	simple_xattrs_init(&kn->iattr->xattrs_trusted);
 	simple_xattrs_init(&kn->iattr->xattrs_security);
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 93bf1dcd0306..ad80f438d8d4 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -90,6 +90,8 @@ int kernfs_iop_getattr(const struct path *path, struct kstat *stat,
 		       u32 request_mask, unsigned int query_flags);
 ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size);
 int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr);
+void kernfs_iattr_init(struct iattr *iattrs, struct kernfs_node *kn);
+struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn);
 
 /*
  * dir.c
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 6dad031be3c2..05fc6812d554 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -108,4 +108,19 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs, cha
 void simple_xattr_list_add(struct simple_xattrs *xattrs,
 			   struct simple_xattr *new_xattr);
 
+static inline int simple_xattrs_empty(struct simple_xattrs *xattrs)
+{
+	return list_empty(&xattrs->head);
+}
+
+/**
+ * Move the xattr list from @src to @dst, leaving @src empty.
+ */
+static inline void simple_xattrs_move(struct simple_xattrs *dst,
+				      struct simple_xattrs *src)
+{
+	simple_xattrs_free(dst);
+	list_replace_init(&src->head, &dst->head);
+}
+
 #endif	/* _LINUX_XATTR_H */
-- 
2.20.1

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Tejun Heo]

On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
> +static int kernfs_node_init_security(struct kernfs_node *parent,
> +				     struct kernfs_node *kn)

Can we skip the whole thing if security is not enabled?

Thanks.

-- 
tejun

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

[Stephen Smalley]

On 2/14/19 4:50 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@redhat.com>
> ---
>   security/selinux/hooks.c            | 160 +++++++++++++++-------------
>   security/selinux/include/security.h |   1 +
>   2 files changed, 88 insertions(+), 73 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 81e012c66d95..7dea5b1a89a3 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;

No need to initialize here since no uses or gotos prior to first assignment?

> +	int rc;
> +
> +	*sid = def_sid;
> +
> +	if (!(inode->i_opflags & IOP_XATTR))
> +		return 0;

Is this a change in behavior from before the patch?  Would we have 
previously called __vfs_getxattr -> xattr_resolve_name and returned 
either -EIO (is_bad_inode) or -EOPNOTSUPP to the caller?  Perhaps it is 
fine to return 0 with the default SID here, but wanted to check.

> +
> +	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)
>   {
> @@ -1400,9 +1467,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>   	u32 task_sid, sid = 0;
>   	u16 sclass;
>   	struct dentry *dentry;
> -#define INITCONTEXTLEN 255
> -	char *context = NULL;
> -	unsigned len = 0;
>   	int rc = 0;
>   
>   	if (isec->initialized == LABEL_INITIALIZED)
> @@ -1470,72 +1534,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 +1589,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="
> 

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Ondrej Mosnacek]

On Thu, Feb 14, 2019 at 4:49 PM Tejun Heo <tj@kernel.org> wrote:
> On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
> > +static int kernfs_node_init_security(struct kernfs_node *parent,
> > +                                  struct kernfs_node *kn)
>
> Can we skip the whole thing if security is not enabled?

Do you mean just skipping the whole part when CONFIG_SECURITY=n? That
is easy to do and I can add it in the next respin (although the
compiler should be able to optimize most of it out in that case).

If you mean dynamically checking if calling the hook would actually do
anything (i.e. if any LSM actually registered that particular hook),
then that should also be possible (just check if
security_hook_heads.kernfs_init_security is a non-empty list), but I'm
not sure if that wouldn't be too hacky...

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

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

[Ondrej Mosnacek]

On Thu, Feb 14, 2019 at 9:49 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 2/14/19 4:50 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@redhat.com>
> > ---
> >   security/selinux/hooks.c            | 160 +++++++++++++++-------------
> >   security/selinux/include/security.h |   1 +
> >   2 files changed, 88 insertions(+), 73 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 81e012c66d95..7dea5b1a89a3 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;
>
> No need to initialize here since no uses or gotos prior to first assignment?

Yes, fixed locally.

>
> > +     int rc;
> > +
> > +     *sid = def_sid;
> > +
> > +     if (!(inode->i_opflags & IOP_XATTR))
> > +             return 0;
>
> Is this a change in behavior from before the patch?  Would we have
> previously called __vfs_getxattr -> xattr_resolve_name and returned
> either -EIO (is_bad_inode) or -EOPNOTSUPP to the caller?  Perhaps it is
> fine to return 0 with the default SID here, but wanted to check.

In the 'case SECURITY_FS_USE_XATTR' block (where this code was
originally), there is the same check actually duplicated at the
beginning (it is not visible in the diff context), for that code path
this check is a no-op. Since moving the check into the function would
possibly change the behavior there, I chose to duplicate the check
inside the new function (and in the new call site we want to leave the
default (genfs sid) and return 0).

I realized now that checking for IOP_XATTR along with SE_SBGENFS_XATTR
and removing the check from inode_doinit_use_xattr() actually makes
more sense. Will do that in the next respin.

>
> > +
> > +     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)
> >   {
> > @@ -1400,9 +1467,6 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
> >       u32 task_sid, sid = 0;
> >       u16 sclass;
> >       struct dentry *dentry;
> > -#define INITCONTEXTLEN 255
> > -     char *context = NULL;
> > -     unsigned len = 0;
> >       int rc = 0;
> >
> >       if (isec->initialized == LABEL_INITIALIZED)
> > @@ -1470,72 +1534,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 +1589,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.

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Tejun Heo]

On Fri, Feb 15, 2019 at 04:45:44PM +0100, Ondrej Mosnacek wrote:
> On Thu, Feb 14, 2019 at 4:49 PM Tejun Heo <tj@kernel.org> wrote:
> > On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
> > > +static int kernfs_node_init_security(struct kernfs_node *parent,
> > > +                                  struct kernfs_node *kn)
> >
> > Can we skip the whole thing if security is not enabled?
> 
> Do you mean just skipping the whole part when CONFIG_SECURITY=n? That
> is easy to do and I can add it in the next respin (although the
> compiler should be able to optimize most of it out in that case).

So the goal is allowing folks who don't use this to not pay.  It'd be
better the evaulation can be as late as possible but obviously there's
a point where that'd be too complicated.  Maybe "ever enabled in this
boot" is a good and simple enough at the same time?

Thanks.

-- 
tejun

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Ondrej Mosnacek]

On Fri, Feb 15, 2019 at 4:50 PM Tejun Heo <tj@kernel.org> wrote:
> On Fri, Feb 15, 2019 at 04:45:44PM +0100, Ondrej Mosnacek wrote:
> > On Thu, Feb 14, 2019 at 4:49 PM Tejun Heo <tj@kernel.org> wrote:
> > > On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
> > > > +static int kernfs_node_init_security(struct kernfs_node *parent,
> > > > +                                  struct kernfs_node *kn)
> > >
> > > Can we skip the whole thing if security is not enabled?
> >
> > Do you mean just skipping the whole part when CONFIG_SECURITY=n? That
> > is easy to do and I can add it in the next respin (although the
> > compiler should be able to optimize most of it out in that case).
>
> So the goal is allowing folks who don't use this to not pay.  It'd be
> better the evaulation can be as late as possible but obviously there's
> a point where that'd be too complicated.  Maybe "ever enabled in this
> boot" is a good and simple enough at the same time?

I don't think there is a way currently to check whether some LSM has
been enabled at boot or not. I suppose we could add such function for
this kind of heuristics, but I'm not sure how it would interplay with
the plans to allow multiple LSM to be enabled simultaneously...
Perhaps it would be better/easier to just add a
security_kernfs_needs_init() function, which would simply check if the
list of registered kernfs_init_security hooks is empty.

I propose something like the patch below (the whitespace is mangled -
intended just for visual review). I plan to fold it into the next
respin if there are no objections to this approach.

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 735a6d382d9d..5b99205da919 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -625,6 +625,9 @@ static int kernfs_node_init_security(struct
kernfs_node *parent,
        struct qstr q;
        int ret;

+       if (!security_kernfs_needs_init() || !parent)
+               return 0;
+
        if (!parent->iattr) {
                kernfs_iattr_init(&iattr_parent, parent);
                simple_xattrs_init(&xattr_parent);
@@ -720,11 +723,9 @@ static struct kernfs_node
*__kernfs_new_node(struct kernfs_root *root,
                        goto err_out3;
        }

-       if (parent) {
-               ret = kernfs_node_init_security(parent, kn);
-               if (ret)
-                       goto err_out3;
-       }
+       ret = kernfs_node_init_security(parent, kn);
+       if (ret)
+               goto err_out3;

        return kn;

diff --git a/include/linux/security.h b/include/linux/security.h
index 581944d1e61e..49a083dbc464 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -292,6 +292,7 @@ int security_inode_listsecurity(struct inode
*inode, char *buffer, size_t buffer
 void security_inode_getsecid(struct inode *inode, u32 *secid);
 int security_inode_copy_up(struct dentry *src, struct cred **new);
 int security_inode_copy_up_xattr(const char *name);
+int security_kernfs_needs_init(void);
 int security_kernfs_init_security(const struct qstr *qstr,
                                  const struct iattr *dir_iattr,
                                  struct simple_xattrs *dir_secattr,
@@ -789,6 +790,11 @@ static inline int security_inode_copy_up(struct
dentry *src, struct cred **new)
        return 0;
 }

+static inline int security_kernfs_needs_init(void)
+{
+       return 0;
+}
+
 static inline int security_kernfs_init_security(
                const struct qstr *qstr, const struct iattr *dir_iattr,
                struct simple_xattrs *dir_secattr, const struct iattr *iattr,
diff --git a/security/security.c b/security/security.c
index 836e0822874a..3c8b9b5baabc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -892,6 +892,11 @@ int security_inode_copy_up_xattr(const char *name)
 }
 EXPORT_SYMBOL(security_inode_copy_up_xattr);

+int security_kernfs_needs_init(void)
+{
+       return !hlist_empty(&security_hook_heads.kernfs_init_security);
+}
+
 int security_kernfs_init_security(const struct qstr *qstr,
                                  const struct iattr *dir_iattr,
                                  struct simple_xattrs *dir_secattr,

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Tejun Heo]

Hello,

On Mon, Feb 18, 2019 at 11:03:58AM +0100, Ondrej Mosnacek wrote:
> I don't think there is a way currently to check whether some LSM has
> been enabled at boot or not. I suppose we could add such function for
> this kind of heuristics, but I'm not sure how it would interplay with
> the plans to allow multiple LSM to be enabled simultaneously...
> Perhaps it would be better/easier to just add a
> security_kernfs_needs_init() function, which would simply check if the
> list of registered kernfs_init_security hooks is empty.
> 
> I propose something like the patch below (the whitespace is mangled -
> intended just for visual review). I plan to fold it into the next
> respin if there are no objections to this approach.

Sounds good to me.

Thanks.

-- 
tejun

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Casey Schaufler]

On 2/18/2019 2:03 AM, Ondrej Mosnacek wrote:
> On Fri, Feb 15, 2019 at 4:50 PM Tejun Heo <tj@kernel.org> wrote:
>> On Fri, Feb 15, 2019 at 04:45:44PM +0100, Ondrej Mosnacek wrote:
>>> On Thu, Feb 14, 2019 at 4:49 PM Tejun Heo <tj@kernel.org> wrote:
>>>> On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
>>>>> +static int kernfs_node_init_security(struct kernfs_node *parent,
>>>>> +                                  struct kernfs_node *kn)
>>>> Can we skip the whole thing if security is not enabled?
>>> Do you mean just skipping the whole part when CONFIG_SECURITY=n? That
>>> is easy to do and I can add it in the next respin (although the
>>> compiler should be able to optimize most of it out in that case).
>> So the goal is allowing folks who don't use this to not pay.  It'd be
>> better the evaulation can be as late as possible but obviously there's
>> a point where that'd be too complicated.  Maybe "ever enabled in this
>> boot" is a good and simple enough at the same time?
> I don't think there is a way currently to check whether some LSM has
> been enabled at boot or not. I suppose we could add such function for
> this kind of heuristics, but I'm not sure how it would interplay with
> the plans to allow multiple LSM to be enabled simultaneously...
> Perhaps it would be better/easier to just add a
> security_kernfs_needs_init() function, which would simply check if the
> list of registered kernfs_init_security hooks is empty.
>
> I propose something like the patch below (the whitespace is mangled -
> intended just for visual review). I plan to fold it into the next
> respin if there are no objections to this approach.
>
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 735a6d382d9d..5b99205da919 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -625,6 +625,9 @@ static int kernfs_node_init_security(struct
> kernfs_node *parent,
>          struct qstr q;
>          int ret;
>
> +       if (!security_kernfs_needs_init() || !parent)
> +               return 0;
> +
>          if (!parent->iattr) {
>                  kernfs_iattr_init(&iattr_parent, parent);
>                  simple_xattrs_init(&xattr_parent);
> @@ -720,11 +723,9 @@ static struct kernfs_node
> *__kernfs_new_node(struct kernfs_root *root,
>                          goto err_out3;
>          }
>
> -       if (parent) {
> -               ret = kernfs_node_init_security(parent, kn);
> -               if (ret)
> -                       goto err_out3;
> -       }
> +       ret = kernfs_node_init_security(parent, kn);
> +       if (ret)
> +               goto err_out3;
>
>          return kn;
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 581944d1e61e..49a083dbc464 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -292,6 +292,7 @@ int security_inode_listsecurity(struct inode
> *inode, char *buffer, size_t buffer
>   void security_inode_getsecid(struct inode *inode, u32 *secid);
>   int security_inode_copy_up(struct dentry *src, struct cred **new);
>   int security_inode_copy_up_xattr(const char *name);
> +int security_kernfs_needs_init(void);
>   int security_kernfs_init_security(const struct qstr *qstr,
>                                    const struct iattr *dir_iattr,
>                                    struct simple_xattrs *dir_secattr,
> @@ -789,6 +790,11 @@ static inline int security_inode_copy_up(struct
> dentry *src, struct cred **new)
>          return 0;
>   }
>
> +static inline int security_kernfs_needs_init(void)
> +{
> +       return 0;
> +}
> +
>   static inline int security_kernfs_init_security(
>                  const struct qstr *qstr, const struct iattr *dir_iattr,
>                  struct simple_xattrs *dir_secattr, const struct iattr *iattr,
> diff --git a/security/security.c b/security/security.c
> index 836e0822874a..3c8b9b5baabc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -892,6 +892,11 @@ int security_inode_copy_up_xattr(const char *name)
>   }
>   EXPORT_SYMBOL(security_inode_copy_up_xattr);
>
> +int security_kernfs_needs_init(void)
> +{
> +       return !hlist_empty(&security_hook_heads.kernfs_init_security);
> +}
> +

Yuck. That's an awful lot of infrastructure just to track
that state. May I suggest that instead you have the
security_kernfs_init_security() hook return -EOPNOTSUPP
in the no-LSM case (2nd argument to call_in_hook). You could
then have a state flag in kernfs that you can set to indicate
you don't need to call security_kernfs_init_security() again.

>   int security_kernfs_init_security(const struct qstr *qstr,
>                                    const struct iattr *dir_iattr,
>                                    struct simple_xattrs *dir_secattr,
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Ondrej Mosnacek]

On Tue, Feb 19, 2019 at 1:28 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/18/2019 2:03 AM, Ondrej Mosnacek wrote:
> > On Fri, Feb 15, 2019 at 4:50 PM Tejun Heo <tj@kernel.org> wrote:
> >> On Fri, Feb 15, 2019 at 04:45:44PM +0100, Ondrej Mosnacek wrote:
> >>> On Thu, Feb 14, 2019 at 4:49 PM Tejun Heo <tj@kernel.org> wrote:
> >>>> On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
> >>>>> +static int kernfs_node_init_security(struct kernfs_node *parent,
> >>>>> +                                  struct kernfs_node *kn)
> >>>> Can we skip the whole thing if security is not enabled?
> >>> Do you mean just skipping the whole part when CONFIG_SECURITY=n? That
> >>> is easy to do and I can add it in the next respin (although the
> >>> compiler should be able to optimize most of it out in that case).
> >> So the goal is allowing folks who don't use this to not pay.  It'd be
> >> better the evaulation can be as late as possible but obviously there's
> >> a point where that'd be too complicated.  Maybe "ever enabled in this
> >> boot" is a good and simple enough at the same time?
> > I don't think there is a way currently to check whether some LSM has
> > been enabled at boot or not. I suppose we could add such function for
> > this kind of heuristics, but I'm not sure how it would interplay with
> > the plans to allow multiple LSM to be enabled simultaneously...
> > Perhaps it would be better/easier to just add a
> > security_kernfs_needs_init() function, which would simply check if the
> > list of registered kernfs_init_security hooks is empty.
> >
> > I propose something like the patch below (the whitespace is mangled -
> > intended just for visual review). I plan to fold it into the next
> > respin if there are no objections to this approach.
> >
> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index 735a6d382d9d..5b99205da919 100644
> > --- a/fs/kernfs/dir.c
> > +++ b/fs/kernfs/dir.c
> > @@ -625,6 +625,9 @@ static int kernfs_node_init_security(struct
> > kernfs_node *parent,
> >          struct qstr q;
> >          int ret;
> >
> > +       if (!security_kernfs_needs_init() || !parent)
> > +               return 0;
> > +
> >          if (!parent->iattr) {
> >                  kernfs_iattr_init(&iattr_parent, parent);
> >                  simple_xattrs_init(&xattr_parent);
> > @@ -720,11 +723,9 @@ static struct kernfs_node
> > *__kernfs_new_node(struct kernfs_root *root,
> >                          goto err_out3;
> >          }
> >
> > -       if (parent) {
> > -               ret = kernfs_node_init_security(parent, kn);
> > -               if (ret)
> > -                       goto err_out3;
> > -       }
> > +       ret = kernfs_node_init_security(parent, kn);
> > +       if (ret)
> > +               goto err_out3;
> >
> >          return kn;
> >
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 581944d1e61e..49a083dbc464 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -292,6 +292,7 @@ int security_inode_listsecurity(struct inode
> > *inode, char *buffer, size_t buffer
> >   void security_inode_getsecid(struct inode *inode, u32 *secid);
> >   int security_inode_copy_up(struct dentry *src, struct cred **new);
> >   int security_inode_copy_up_xattr(const char *name);
> > +int security_kernfs_needs_init(void);
> >   int security_kernfs_init_security(const struct qstr *qstr,
> >                                    const struct iattr *dir_iattr,
> >                                    struct simple_xattrs *dir_secattr,
> > @@ -789,6 +790,11 @@ static inline int security_inode_copy_up(struct
> > dentry *src, struct cred **new)
> >          return 0;
> >   }
> >
> > +static inline int security_kernfs_needs_init(void)
> > +{
> > +       return 0;
> > +}
> > +
> >   static inline int security_kernfs_init_security(
> >                  const struct qstr *qstr, const struct iattr *dir_iattr,
> >                  struct simple_xattrs *dir_secattr, const struct iattr *iattr,
> > diff --git a/security/security.c b/security/security.c
> > index 836e0822874a..3c8b9b5baabc 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -892,6 +892,11 @@ int security_inode_copy_up_xattr(const char *name)
> >   }
> >   EXPORT_SYMBOL(security_inode_copy_up_xattr);
> >
> > +int security_kernfs_needs_init(void)
> > +{
> > +       return !hlist_empty(&security_hook_heads.kernfs_init_security);
> > +}
> > +
>
> Yuck. That's an awful lot of infrastructure just to track
> that state. May I suggest that instead you have the
> security_kernfs_init_security() hook return -EOPNOTSUPP
> in the no-LSM case (2nd argument to call_in_hook). You could
> then have a state flag in kernfs that you can set to indicate
> you don't need to call security_kernfs_init_security() again.

Well, maintaining a global variable sounds even more yucky to me...
And I don't understand why you'd consider a simple one-line function
to be "an awful lot of infrastructure" :) But at the end of the day it
is up to the maintainers - Greg/Tejun and James/Serge (who I forgot to
Cc on these patches, sorry) - what works better for them.

>
> >   int security_kernfs_init_security(const struct qstr *qstr,
> >                                    const struct iattr *dir_iattr,
> >                                    struct simple_xattrs *dir_secattr,
> >
> > --
> > Ondrej Mosnacek <omosnace at redhat dot com>
> > Associate Software Engineer, Security Technologies
> > Red Hat, Inc.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Tejun Heo]

Hello,

On Tue, Feb 19, 2019 at 03:10:30PM +0100, Ondrej Mosnacek wrote:
> Well, maintaining a global variable sounds even more yucky to me...
> And I don't understand why you'd consider a simple one-line function
> to be "an awful lot of infrastructure" :) But at the end of the day it
> is up to the maintainers - Greg/Tejun and James/Serge (who I forgot to
> Cc on these patches, sorry) - what works better for them.

As long as the cost can be avoided for folks who don't use the
relevant features, I don't have a strong opinion on how that's done.

Thanks.

-- 
tejun

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Casey Schaufler]

On 2/19/2019 6:10 AM, Ondrej Mosnacek wrote:
> On Tue, Feb 19, 2019 at 1:28 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 2/18/2019 2:03 AM, Ondrej Mosnacek wrote:
>>> On Fri, Feb 15, 2019 at 4:50 PM Tejun Heo <tj@kernel.org> wrote:
>>>> On Fri, Feb 15, 2019 at 04:45:44PM +0100, Ondrej Mosnacek wrote:
>>>>> On Thu, Feb 14, 2019 at 4:49 PM Tejun Heo <tj@kernel.org> wrote:
>>>>>> On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
>>>>>>> +static int kernfs_node_init_security(struct kernfs_node *parent,
>>>>>>> +                                  struct kernfs_node *kn)
>>>>>> Can we skip the whole thing if security is not enabled?
>>>>> Do you mean just skipping the whole part when CONFIG_SECURITY=n? That
>>>>> is easy to do and I can add it in the next respin (although the
>>>>> compiler should be able to optimize most of it out in that case).
>>>> So the goal is allowing folks who don't use this to not pay.  It'd be
>>>> better the evaulation can be as late as possible but obviously there's
>>>> a point where that'd be too complicated.  Maybe "ever enabled in this
>>>> boot" is a good and simple enough at the same time?
>>> I don't think there is a way currently to check whether some LSM has
>>> been enabled at boot or not. I suppose we could add such function for
>>> this kind of heuristics, but I'm not sure how it would interplay with
>>> the plans to allow multiple LSM to be enabled simultaneously...
>>> Perhaps it would be better/easier to just add a
>>> security_kernfs_needs_init() function, which would simply check if the
>>> list of registered kernfs_init_security hooks is empty.
>>>
>>> I propose something like the patch below (the whitespace is mangled -
>>> intended just for visual review). I plan to fold it into the next
>>> respin if there are no objections to this approach.
>>>
>>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
>>> index 735a6d382d9d..5b99205da919 100644
>>> --- a/fs/kernfs/dir.c
>>> +++ b/fs/kernfs/dir.c
>>> @@ -625,6 +625,9 @@ static int kernfs_node_init_security(struct
>>> kernfs_node *parent,
>>>           struct qstr q;
>>>           int ret;
>>>
>>> +       if (!security_kernfs_needs_init() || !parent)
>>> +               return 0;
>>> +
>>>           if (!parent->iattr) {
>>>                   kernfs_iattr_init(&iattr_parent, parent);
>>>                   simple_xattrs_init(&xattr_parent);
>>> @@ -720,11 +723,9 @@ static struct kernfs_node
>>> *__kernfs_new_node(struct kernfs_root *root,
>>>                           goto err_out3;
>>>           }
>>>
>>> -       if (parent) {
>>> -               ret = kernfs_node_init_security(parent, kn);
>>> -               if (ret)
>>> -                       goto err_out3;
>>> -       }
>>> +       ret = kernfs_node_init_security(parent, kn);
>>> +       if (ret)
>>> +               goto err_out3;
>>>
>>>           return kn;
>>>
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index 581944d1e61e..49a083dbc464 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -292,6 +292,7 @@ int security_inode_listsecurity(struct inode
>>> *inode, char *buffer, size_t buffer
>>>    void security_inode_getsecid(struct inode *inode, u32 *secid);
>>>    int security_inode_copy_up(struct dentry *src, struct cred **new);
>>>    int security_inode_copy_up_xattr(const char *name);
>>> +int security_kernfs_needs_init(void);
>>>    int security_kernfs_init_security(const struct qstr *qstr,
>>>                                     const struct iattr *dir_iattr,
>>>                                     struct simple_xattrs *dir_secattr,
>>> @@ -789,6 +790,11 @@ static inline int security_inode_copy_up(struct
>>> dentry *src, struct cred **new)
>>>           return 0;
>>>    }
>>>
>>> +static inline int security_kernfs_needs_init(void)
>>> +{
>>> +       return 0;
>>> +}
>>> +
>>>    static inline int security_kernfs_init_security(
>>>                   const struct qstr *qstr, const struct iattr *dir_iattr,
>>>                   struct simple_xattrs *dir_secattr, const struct iattr *iattr,
>>> diff --git a/security/security.c b/security/security.c
>>> index 836e0822874a..3c8b9b5baabc 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -892,6 +892,11 @@ int security_inode_copy_up_xattr(const char *name)
>>>    }
>>>    EXPORT_SYMBOL(security_inode_copy_up_xattr);
>>>
>>> +int security_kernfs_needs_init(void)
>>> +{
>>> +       return !hlist_empty(&security_hook_heads.kernfs_init_security);
>>> +}
>>> +
>> Yuck. That's an awful lot of infrastructure just to track
>> that state. May I suggest that instead you have the
>> security_kernfs_init_security() hook return -EOPNOTSUPP
>> in the no-LSM case (2nd argument to call_in_hook). You could
>> then have a state flag in kernfs that you can set to indicate
>> you don't need to call security_kernfs_init_security() again.
> Well, maintaining a global variable sounds even more yucky to me...

The state you're maintaining is kernfs state, not LSM
infrastructure state. The state should be maintained in
kernfs, not in the LSM infrastructure.

> And I don't understand why you'd consider a simple one-line function
> to be "an awful lot of infrastructure" :)

That is because you haven't been working with the LSM
infrastructure very long. If you'll recall, I have already
objected to kernfs specific LSM hooks. Now, when you find
that your approach for using a hook has issues, you want
to add another function that does nothing but tell you
that there's nothing to do. You can get that by calling
the security_kernfs_init_security() hook. The LSM infrastructure
is set up to be as painless as possible when you don't
want to use it.

>   But at the end of the day it
> is up to the maintainers - Greg/Tejun and James/Serge (who I forgot to
> Cc on these patches, sorry)

Yes, James and Serge are the maintainers for the
security sub-system, but I have done the transition
from hook vector to hook lists and am currently doing
the transition from module to infrastructure blob management.
I know it better, and have more invested in it, than anyone
else just now.

>   - what works better for them.

Kernfs is an important component of the kernel. So is
the security infrastructure. I would hope you don't want
to turn this into a contest to see which maintainer has
the biggest clout.

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Ondrej Mosnacek]

On Tue, Feb 19, 2019 at 5:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/19/2019 6:10 AM, Ondrej Mosnacek wrote:
> > On Tue, Feb 19, 2019 at 1:28 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 2/18/2019 2:03 AM, Ondrej Mosnacek wrote:
> >>> On Fri, Feb 15, 2019 at 4:50 PM Tejun Heo <tj@kernel.org> wrote:
> >>>> On Fri, Feb 15, 2019 at 04:45:44PM +0100, Ondrej Mosnacek wrote:
> >>>>> On Thu, Feb 14, 2019 at 4:49 PM Tejun Heo <tj@kernel.org> wrote:
> >>>>>> On Thu, Feb 14, 2019 at 10:50:15AM +0100, Ondrej Mosnacek wrote:
> >>>>>>> +static int kernfs_node_init_security(struct kernfs_node *parent,
> >>>>>>> +                                  struct kernfs_node *kn)
> >>>>>> Can we skip the whole thing if security is not enabled?
> >>>>> Do you mean just skipping the whole part when CONFIG_SECURITY=n? That
> >>>>> is easy to do and I can add it in the next respin (although the
> >>>>> compiler should be able to optimize most of it out in that case).
> >>>> So the goal is allowing folks who don't use this to not pay.  It'd be
> >>>> better the evaulation can be as late as possible but obviously there's
> >>>> a point where that'd be too complicated.  Maybe "ever enabled in this
> >>>> boot" is a good and simple enough at the same time?
> >>> I don't think there is a way currently to check whether some LSM has
> >>> been enabled at boot or not. I suppose we could add such function for
> >>> this kind of heuristics, but I'm not sure how it would interplay with
> >>> the plans to allow multiple LSM to be enabled simultaneously...
> >>> Perhaps it would be better/easier to just add a
> >>> security_kernfs_needs_init() function, which would simply check if the
> >>> list of registered kernfs_init_security hooks is empty.
> >>>
> >>> I propose something like the patch below (the whitespace is mangled -
> >>> intended just for visual review). I plan to fold it into the next
> >>> respin if there are no objections to this approach.
> >>>
> >>> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> >>> index 735a6d382d9d..5b99205da919 100644
> >>> --- a/fs/kernfs/dir.c
> >>> +++ b/fs/kernfs/dir.c
> >>> @@ -625,6 +625,9 @@ static int kernfs_node_init_security(struct
> >>> kernfs_node *parent,
> >>>           struct qstr q;
> >>>           int ret;
> >>>
> >>> +       if (!security_kernfs_needs_init() || !parent)
> >>> +               return 0;
> >>> +
> >>>           if (!parent->iattr) {
> >>>                   kernfs_iattr_init(&iattr_parent, parent);
> >>>                   simple_xattrs_init(&xattr_parent);
> >>> @@ -720,11 +723,9 @@ static struct kernfs_node
> >>> *__kernfs_new_node(struct kernfs_root *root,
> >>>                           goto err_out3;
> >>>           }
> >>>
> >>> -       if (parent) {
> >>> -               ret = kernfs_node_init_security(parent, kn);
> >>> -               if (ret)
> >>> -                       goto err_out3;
> >>> -       }
> >>> +       ret = kernfs_node_init_security(parent, kn);
> >>> +       if (ret)
> >>> +               goto err_out3;
> >>>
> >>>           return kn;
> >>>
> >>> diff --git a/include/linux/security.h b/include/linux/security.h
> >>> index 581944d1e61e..49a083dbc464 100644
> >>> --- a/include/linux/security.h
> >>> +++ b/include/linux/security.h
> >>> @@ -292,6 +292,7 @@ int security_inode_listsecurity(struct inode
> >>> *inode, char *buffer, size_t buffer
> >>>    void security_inode_getsecid(struct inode *inode, u32 *secid);
> >>>    int security_inode_copy_up(struct dentry *src, struct cred **new);
> >>>    int security_inode_copy_up_xattr(const char *name);
> >>> +int security_kernfs_needs_init(void);
> >>>    int security_kernfs_init_security(const struct qstr *qstr,
> >>>                                     const struct iattr *dir_iattr,
> >>>                                     struct simple_xattrs *dir_secattr,
> >>> @@ -789,6 +790,11 @@ static inline int security_inode_copy_up(struct
> >>> dentry *src, struct cred **new)
> >>>           return 0;
> >>>    }
> >>>
> >>> +static inline int security_kernfs_needs_init(void)
> >>> +{
> >>> +       return 0;
> >>> +}
> >>> +
> >>>    static inline int security_kernfs_init_security(
> >>>                   const struct qstr *qstr, const struct iattr *dir_iattr,
> >>>                   struct simple_xattrs *dir_secattr, const struct iattr *iattr,
> >>> diff --git a/security/security.c b/security/security.c
> >>> index 836e0822874a..3c8b9b5baabc 100644
> >>> --- a/security/security.c
> >>> +++ b/security/security.c
> >>> @@ -892,6 +892,11 @@ int security_inode_copy_up_xattr(const char *name)
> >>>    }
> >>>    EXPORT_SYMBOL(security_inode_copy_up_xattr);
> >>>
> >>> +int security_kernfs_needs_init(void)
> >>> +{
> >>> +       return !hlist_empty(&security_hook_heads.kernfs_init_security);
> >>> +}
> >>> +
> >> Yuck. That's an awful lot of infrastructure just to track
> >> that state. May I suggest that instead you have the
> >> security_kernfs_init_security() hook return -EOPNOTSUPP
> >> in the no-LSM case (2nd argument to call_in_hook). You could
> >> then have a state flag in kernfs that you can set to indicate
> >> you don't need to call security_kernfs_init_security() again.
> > Well, maintaining a global variable sounds even more yucky to me...
>
> The state you're maintaining is kernfs state, not LSM
> infrastructure state. The state should be maintained in
> kernfs, not in the LSM infrastructure.

But I'm not maintaining any state. I'm merely trying to answer the
query "Is there anything that will handle this hook? Do I need to
prepare stuff for it?", which is obviously a query about the LSM
state. Granted, ideally we wouldn't need to do any preparatory work at
all, but that would require exposing more of the kernfs internals
(which brings its own issues, but maybe I'll need to look into that
approach more...).

>
> > And I don't understand why you'd consider a simple one-line function
> > to be "an awful lot of infrastructure" :)
>
> That is because you haven't been working with the LSM
> infrastructure very long. If you'll recall, I have already
> objected to kernfs specific LSM hooks. Now, when you find
> that your approach for using a hook has issues, you want
> to add another function that does nothing but tell you
> that there's nothing to do. You can get that by calling
> the security_kernfs_init_security() hook. The LSM infrastructure
> is set up to be as painless as possible when you don't
> want to use it.
>
> >   But at the end of the day it
> > is up to the maintainers - Greg/Tejun and James/Serge (who I forgot to
> > Cc on these patches, sorry)
>
> Yes, James and Serge are the maintainers for the
> security sub-system, but I have done the transition
> from hook vector to hook lists and am currently doing
> the transition from module to infrastructure blob management.
> I know it better, and have more invested in it, than anyone
> else just now.

Fair enough.

>
> >   - what works better for them.
>
> Kernfs is an important component of the kernel. So is
> the security infrastructure. I would hope you don't want
> to turn this into a contest to see which maintainer has
> the biggest clout.

Oh, no, you misunderstood my intention. I just got a feeling that this
thread was turning into a discussion about perceived code ugliness
(and about which subsystem that ugliness ends up in), which is
naturally a very subjective topic, so I wanted to know what is the
opinion of the people that have the final decision about whether the
code should get in or not. Anyway, I'll try to find a more elegant
variant of the solution once again, hopefully I manage to get to
something less controversial.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Casey Schaufler]

On 2/21/2019 1:13 AM, Ondrej Mosnacek wrote:
> On Tue, Feb 19, 2019 at 5:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> .....
>> The state you're maintaining is kernfs state, not LSM
>> infrastructure state. The state should be maintained in
>> kernfs, not in the LSM infrastructure.
> But I'm not maintaining any state. I'm merely trying to answer the
> query "Is there anything that will handle this hook? Do I need to
> prepare stuff for it?", which is obviously a query about the LSM
> state. Granted, ideally we wouldn't need to do any preparatory work at
> all, but that would require exposing more of the kernfs internals
> (which brings its own issues, but maybe I'll need to look into that
> approach more...).

It sounds like you're bumping up against the limitations
of the finely honed optimized implementation of kernfs. :(
If it where still the pre-android era, when using an LSM
was rare, the check for an LSM might have made sense. Today,
with the vast majority of systems using LSMs*, optimizing for
the no LSM case is nonsensical.

---
* Android, Tizen, Fedora/RHEL, Ubuntu

> ...
> Kernfs is an important component of the kernel. So is
> the security infrastructure. I would hope you don't want
> to turn this into a contest to see which maintainer has
> the biggest clout.
> Oh, no, you misunderstood my intention. I just got a feeling that this
> thread was turning into a discussion about perceived code ugliness
> (and about which subsystem that ugliness ends up in), which is
> naturally a very subjective topic, so I wanted to know what is the
> opinion of the people that have the final decision about whether the
> code should get in or not. Anyway, I'll try to find a more elegant
> variant of the solution once again, hopefully I manage to get to
> something less controversial.

Thank you. I believe (which, of course, doesn't make it true)
that when a component goes outside the general system architecture
the way that kernfs does *even for performance reasons* that it is
responsible for the edge cases it encounters. I know that I've had
to do a good bit of that in Smack.
  

Re: [PATCH v6 5/5] kernfs: initialize security of newly created nodes

[Ondrej Mosnacek]

On Thu, Feb 21, 2019 at 5:52 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/21/2019 1:13 AM, Ondrej Mosnacek wrote:
> > On Tue, Feb 19, 2019 at 5:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > .....
> >> The state you're maintaining is kernfs state, not LSM
> >> infrastructure state. The state should be maintained in
> >> kernfs, not in the LSM infrastructure.
> > But I'm not maintaining any state. I'm merely trying to answer the
> > query "Is there anything that will handle this hook? Do I need to
> > prepare stuff for it?", which is obviously a query about the LSM
> > state. Granted, ideally we wouldn't need to do any preparatory work at
> > all, but that would require exposing more of the kernfs internals
> > (which brings its own issues, but maybe I'll need to look into that
> > approach more...).
>
> It sounds like you're bumping up against the limitations
> of the finely honed optimized implementation of kernfs. :(
> If it where still the pre-android era, when using an LSM
> was rare, the check for an LSM might have made sense. Today,
> with the vast majority of systems using LSMs*, optimizing for
> the no LSM case is nonsensical.

I imagine it might make sense on some very minimal embedded systems
(microcontrollers?), where you almost certainly need kernfs (via sysfs
or debugfs), but you don't necessarily need advanced security controls
(you might just have a single monolithic userspace process running
there and very limited memory/CPU resources). I'm hardly an expert on
embedded platforms, but it sounds like a reasonable use case.
(Although in such cases I'd expect CONFIG_SECURITY to be simply set to
'n', so no need for runtime checks anyway...).

>
> ---
> * Android, Tizen, Fedora/RHEL, Ubuntu
>
> > ...
> > Kernfs is an important component of the kernel. So is
> > the security infrastructure. I would hope you don't want
> > to turn this into a contest to see which maintainer has
> > the biggest clout.
> > Oh, no, you misunderstood my intention. I just got a feeling that this
> > thread was turning into a discussion about perceived code ugliness
> > (and about which subsystem that ugliness ends up in), which is
> > naturally a very subjective topic, so I wanted to know what is the
> > opinion of the people that have the final decision about whether the
> > code should get in or not. Anyway, I'll try to find a more elegant
> > variant of the solution once again, hopefully I manage to get to
> > something less controversial.
>
> Thank you. I believe (which, of course, doesn't make it true)
> that when a component goes outside the general system architecture
> the way that kernfs does *even for performance reasons* that it is
> responsible for the edge cases it encounters. I know that I've had
> to do a good bit of that in Smack.

OK, so I tried taking the other approach (pass kernfs nodes and expose
necessary kernfs internals) and I'm quite happy with the result. It
turns out the only thing I actually need to expose (at least for
SELinux) is getting and setting the security xattrs, which is just two
simple functions. The rest of the attributes (uid, gid, and access
times) don't seem important and they can always be exposed by adding
more helper functions as needed. With this design the last patch now
becomes embarrassingly simple - there is just a single call that will
either call an LSM hook or do nothing at all.

I still need to do some testing on the new patches before posting. In
the meantime they are available here for the curious:

https://gitlab.com/omos/linux-public/compare/selinux-next...selinux-fix-cgroupfs-v12

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.