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

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

[Ondrej Mosnacek]

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/

This series adds a new security hook that allows to initialize the security
context of kernfs properly, taking into account the parent context. 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
metadata.

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

Therefore, to allow maintaining similar behavior for kernfs nodes, a new LSM
hook is needed, which would allow initializing the kernfs node's security
context based on the context stored in the parent's node (if any).

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.

The first patch adds the new LSM hook; the second patch implements the hook
in SELinux; and the third patch modifies kernfs to use the new hook to
initialize the security context of kernfs nodes whenever its parent node
has a non-default context set.

Note: the patches are based on current selinux/next [3], but they seem to
apply cleanly on top of v5.0-rc1 as well.

Testing:
- passed SELinux testsuite on Fedora 29 (x86_64) when applied on top of
  current Rawhide kernel (5.0.0-0.rc1.git0.1) [4]
- passed the reproducer from the last patch

[1] https://github.com/SELinuxProject/selinux-kernel/issues/39
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
[3] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/log/?h=selinux-pr-20181224
[4] https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/842855/

Ondrej Mosnacek (3):
  LSM: Add new hook for generic node initialization
  selinux: Implement the object_init_security hook
  kernfs: Initialize security of newly created nodes

 fs/kernfs/dir.c             | 49 ++++++++++++++++++++++++++++++++++---
 fs/kernfs/inode.c           |  9 +++----
 fs/kernfs/kernfs-internal.h |  4 +++
 include/linux/lsm_hooks.h   | 30 +++++++++++++++++++++++
 include/linux/security.h    | 14 +++++++++++
 security/security.c         | 10 ++++++++
 security/selinux/hooks.c    | 41 +++++++++++++++++++++++++++++++
 7 files changed, 149 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] LSM: Add new hook for generic node initialization

[Ondrej Mosnacek]

This patch introduces a new security hook that is intended for
initializing the security data for newly created pseudo filesystem
objects (such as kernfs nodes) that provide a way of storing a
non-default security context, but need to operate independently from
mounts.

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.

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

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index aaeb7fa24dc4..3a2399d7721f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -429,6 +429,31 @@
  *	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 special file-like objects
+ *
+ * @object_init_security:
+ *	Obtain the security context for a newly created filesystem object
+ *	based on the security context of the parent node.  The purpose is
+ *	similar to @inode_init_security, but this hook is intended for
+ *	non-inode objects that need to behave like a directory tree (e.g.
+ *	kernfs nodes).  In this case it is assumed that the LSM assigns some
+ *	default context to the node by default and the object internally stores
+ *	a copy of the security context if (and only if) it has been set to a
+ *	non-default value explicitly (e.g. via *setxattr(2)).
+ *
+ *	@parent_ctx contains the security context of the parent directory
+ *	(must not be NULL -- if the parent has no explicit context set,
+ *	the child should also keep the default context and the hook should
+ *	not be called).
+ *	@parent_ctxlen contains the length of @parent_ctx data.
+ *	@qstr contains the last path component of the new object.
+ *	@mode contanis the file mode of the object.
+ *	@ctx is a pointer in which to place the allocated security context.
+ *	@ctxlen points to the place to put the length of @ctx.
+ *
+ *	Returns 0 if @ctx and @ctxlen have been successfully set or
+ *	-ENOMEM on memory allocation failure.
+ *
  * Security hooks for file operations
  *
  * @file_permission:
@@ -1556,6 +1581,10 @@ union security_list_options {
 	int (*inode_copy_up)(struct dentry *src, struct cred **new);
 	int (*inode_copy_up_xattr)(const char *name);
 
+	int (*object_init_security)(void *parent_ctx, u32 parent_ctxlen,
+				    const struct qstr *qstr, u16 mode,
+				    void **ctx, u32 *ctxlen);
+
 	int (*file_permission)(struct file *file, int mask);
 	int (*file_alloc_security)(struct file *file);
 	void (*file_free_security)(struct file *file);
@@ -1855,6 +1884,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 object_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 d170a5b031f3..1e7971d10fe6 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -315,6 +315,9 @@ 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_object_init_security(void *parent_ctx, u32 parent_ctxlen,
+				  const struct qstr *qstr, u16 mode,
+				  void **ctx, u32 *ctxlen);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
@@ -815,6 +818,17 @@ static inline int security_inode_copy_up_xattr(const char *name)
 	return -EOPNOTSUPP;
 }
 
+static inline int security_object_init_security(void *parent_ctx,
+						u32 parent_ctxlen,
+						const struct qstr *qstr,
+						u16 mode, void **ctx,
+						u32 *ctxlen)
+{
+	*ctx = NULL;
+	*ctxlen = 0;
+	return 0;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;
diff --git a/security/security.c b/security/security.c
index 04d173eb93f6..a010bfbe3fc6 100644
--- a/security/security.c
+++ b/security/security.c
@@ -879,6 +879,16 @@ int security_inode_copy_up_xattr(const char *name)
 }
 EXPORT_SYMBOL(security_inode_copy_up_xattr);
 
+int security_object_init_security(void *parent_ctx, u32 parent_ctxlen,
+				  const struct qstr *qstr, u16 mode,
+				  void **ctx, u32 *ctxlen)
+{
+	*ctx = NULL;
+	*ctxlen = 0;
+	return call_int_hook(object_init_security, 0, parent_ctx, parent_ctxlen,
+			     qstr, mode, ctx, ctxlen);
+}
+
 int security_file_permission(struct file *file, int mask)
 {
 	int ret;
-- 
2.20.1

Re: [PATCH v2 1/3] LSM: Add new hook for generic node initialization

[Casey Schaufler]

On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> This patch introduces a new security hook that is intended for
> initializing the security data for newly created pseudo filesystem
> objects (such as kernfs nodes) that provide a way of storing a
> non-default security context, but need to operate independently from
> mounts.
>
> 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.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++++
>  include/linux/security.h  | 14 ++++++++++++++
>  security/security.c       | 10 ++++++++++
>  3 files changed, 54 insertions(+)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index aaeb7fa24dc4..3a2399d7721f 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -429,6 +429,31 @@
>   *	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 special file-like objects
> + *
> + * @object_init_security:

I don't like the name. There are too many things that are "objects"
for this to be meaningful. I also dislike seeing names like
security_object_init_security. How about init_from_parent? If there's
never a chance that it will be used anywhere but with kernfs, it could
be kernfs_node_init. The existing set of hook names are sufficiently
confusing without adding to the mystery.

> + *	Obtain the security context for a newly created filesystem object
> + *	based on the security context of the parent node.  The purpose is
> + *	similar to @inode_init_security, but this hook is intended for
> + *	non-inode objects that need to behave like a directory tree (e.g.
> + *	kernfs nodes).  In this case it is assumed that the LSM assigns some
> + *	default context to the node by default and the object internally stores
> + *	a copy of the security context if (and only if) it has been set to a
> + *	non-default value explicitly (e.g. via *setxattr(2)).
> + *
> + *	@parent_ctx contains the security context of the parent directory
> + *	(must not be NULL -- if the parent has no explicit context set,
> + *	the child should also keep the default context and the hook should
> + *	not be called).
> + *	@parent_ctxlen contains the length of @parent_ctx data.
> + *	@qstr contains the last path component of the new object.
> + *	@mode contanis the file mode of the object.

s/contanis/contains/

> + *	@ctx is a pointer in which to place the allocated security context.
> + *	@ctxlen points to the place to put the length of @ctx.
> + *
> + *	Returns 0 if @ctx and @ctxlen have been successfully set or
> + *	-ENOMEM on memory allocation failure.
> + *
>   * Security hooks for file operations
>   *
>   * @file_permission:
> @@ -1556,6 +1581,10 @@ union security_list_options {
>  	int (*inode_copy_up)(struct dentry *src, struct cred **new);
>  	int (*inode_copy_up_xattr)(const char *name);
>  
> +	int (*object_init_security)(void *parent_ctx, u32 parent_ctxlen,
> +				    const struct qstr *qstr, u16 mode,
> +				    void **ctx, u32 *ctxlen);
> +
>  	int (*file_permission)(struct file *file, int mask);
>  	int (*file_alloc_security)(struct file *file);
>  	void (*file_free_security)(struct file *file);
> @@ -1855,6 +1884,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 object_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 d170a5b031f3..1e7971d10fe6 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -315,6 +315,9 @@ 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_object_init_security(void *parent_ctx, u32 parent_ctxlen,
> +				  const struct qstr *qstr, u16 mode,
> +				  void **ctx, u32 *ctxlen);
>  int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
> @@ -815,6 +818,17 @@ static inline int security_inode_copy_up_xattr(const char *name)
>  	return -EOPNOTSUPP;
>  }
>  
> +static inline int security_object_init_security(void *parent_ctx,
> +						u32 parent_ctxlen,
> +						const struct qstr *qstr,
> +						u16 mode, void **ctx,
> +						u32 *ctxlen)
> +{
> +	*ctx = NULL;
> +	*ctxlen = 0;
> +	return 0;
> +}
> +
>  static inline int security_file_permission(struct file *file, int mask)
>  {
>  	return 0;
> diff --git a/security/security.c b/security/security.c
> index 04d173eb93f6..a010bfbe3fc6 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -879,6 +879,16 @@ int security_inode_copy_up_xattr(const char *name)
>  }
>  EXPORT_SYMBOL(security_inode_copy_up_xattr);
>  
> +int security_object_init_security(void *parent_ctx, u32 parent_ctxlen,
> +				  const struct qstr *qstr, u16 mode,
> +				  void **ctx, u32 *ctxlen)
> +{
> +	*ctx = NULL;
> +	*ctxlen = 0;
> +	return call_int_hook(object_init_security, 0, parent_ctx, parent_ctxlen,
> +			     qstr, mode, ctx, ctxlen);
> +}
> +
>  int security_file_permission(struct file *file, int mask)
>  {
>  	int ret;

Re: [PATCH v2 1/3] LSM: Add new hook for generic node initialization

[Paul Moore]

On Wed, Jan 9, 2019 at 12:08 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> > This patch introduces a new security hook that is intended for
> > initializing the security data for newly created pseudo filesystem
> > objects (such as kernfs nodes) that provide a way of storing a
> > non-default security context, but need to operate independently from
> > mounts.
> >
> > 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.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++++
> >  include/linux/security.h  | 14 ++++++++++++++
> >  security/security.c       | 10 ++++++++++
> >  3 files changed, 54 insertions(+)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index aaeb7fa24dc4..3a2399d7721f 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -429,6 +429,31 @@
> >   *   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 special file-like objects
> > + *
> > + * @object_init_security:
>
> I don't like the name. There are too many things that are "objects"
> for this to be meaningful. I also dislike seeing names like
> security_object_init_security. How about init_from_parent? If there's
> never a chance that it will be used anywhere but with kernfs, it could
> be kernfs_node_init. The existing set of hook names are sufficiently
> confusing without adding to the mystery.

I like the naming similarity with inode_init_security(), that seems
helpful.  Although I somewhat understand you concern about the generic
"object".  Could you live with kernfs_init_security()?  If another fs
adopts it, we could always changing the name later if needed.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 1/3] LSM: Add new hook for generic node initialization

[Casey Schaufler]

On 1/10/2019 5:57 PM, Paul Moore wrote:
> On Wed, Jan 9, 2019 at 12:08 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>> This patch introduces a new security hook that is intended for
>>> initializing the security data for newly created pseudo filesystem
>>> objects (such as kernfs nodes) that provide a way of storing a
>>> non-default security context, but need to operate independently from
>>> mounts.
>>>
>>> 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.
>>>
>>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>>> ---
>>>  include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++++
>>>  include/linux/security.h  | 14 ++++++++++++++
>>>  security/security.c       | 10 ++++++++++
>>>  3 files changed, 54 insertions(+)
>>>
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index aaeb7fa24dc4..3a2399d7721f 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -429,6 +429,31 @@
>>>   *   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 special file-like objects
>>> + *
>>> + * @object_init_security:
>> I don't like the name. There are too many things that are "objects"
>> for this to be meaningful. I also dislike seeing names like
>> security_object_init_security. How about init_from_parent? If there's
>> never a chance that it will be used anywhere but with kernfs, it could
>> be kernfs_node_init. The existing set of hook names are sufficiently
>> confusing without adding to the mystery.
> I like the naming similarity with inode_init_security(), that seems
> helpful.  Although I somewhat understand you concern about the generic
> "object".  Could you live with kernfs_init_security()?  If another fs
> adopts it, we could always changing the name later if needed.

I can live with it as is, but kernfs_init_security would
be better. The security_blah_security names seem crazy to
me, but changing object to kernfs is really what's important.

Re: [PATCH v2 1/3] LSM: Add new hook for generic node initialization

[Ondrej Mosnacek]

On Fri, Jan 11, 2019 at 2:57 AM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Jan 9, 2019 at 12:08 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> > > This patch introduces a new security hook that is intended for
> > > initializing the security data for newly created pseudo filesystem
> > > objects (such as kernfs nodes) that provide a way of storing a
> > > non-default security context, but need to operate independently from
> > > mounts.
> > >
> > > 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.
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  include/linux/lsm_hooks.h | 30 ++++++++++++++++++++++++++++++
> > >  include/linux/security.h  | 14 ++++++++++++++
> > >  security/security.c       | 10 ++++++++++
> > >  3 files changed, 54 insertions(+)
> > >
> > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > > index aaeb7fa24dc4..3a2399d7721f 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -429,6 +429,31 @@
> > >   *   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 special file-like objects
> > > + *
> > > + * @object_init_security:
> >
> > I don't like the name. There are too many things that are "objects"
> > for this to be meaningful. I also dislike seeing names like
> > security_object_init_security. How about init_from_parent? If there's
> > never a chance that it will be used anywhere but with kernfs, it could
> > be kernfs_node_init. The existing set of hook names are sufficiently
> > confusing without adding to the mystery.

TBH, I wasn't completely satisfied with the name either, I was hoping
to get some better suggestions along the way.

>
> I like the naming similarity with inode_init_security(), that seems
> helpful.  Although I somewhat understand you concern about the generic
> "object".  Could you live with kernfs_init_security()?  If another fs
> adopts it, we could always changing the name later if needed.

I decided to leave out kernfs out of the name, since it ended up
looking quite generic (nothing particularly kernfs-specific in the
argument list). On second thought, I'm starting to prefer
kernfs_init_security(), considering the very low likelihood that it
would be useful elsewhere and that it will likely end up having some
more arguments tailored specifically for kernfs in the next revisions
(w.r.t. the cover letter discussion).




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

[PATCH v2 2/3] selinux: Implement the object_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 by other hooks.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 security/selinux/hooks.c | 41 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7ce012d9ec51..29c038513504 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3526,6 +3526,45 @@ static int selinux_inode_copy_up_xattr(const char *name)
 	return -EOPNOTSUPP;
 }
 
+/* file-like object operations */
+
+/* Used e.g. for kernfs_node for newly created nodes */
+static int selinux_object_init_security(void *parent_ctx, u32 parent_ctxlen,
+					const struct qstr *qstr, u16 mode,
+					void **ctx, u32 *ctxlen)
+{
+	const struct task_security_struct *tsec = current_security();
+	u32 parent_sid, newsid, clen;
+	int rc;
+	char *context;
+
+	rc = security_context_to_sid(&selinux_state, parent_ctx, parent_ctxlen,
+				     &parent_sid, GFP_KERNEL);
+	if (rc)
+		return rc;
+
+	if (tsec->create_sid) {
+		newsid = tsec->create_sid;
+	} else {
+		u16 secclass = inode_mode_to_security_class(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;
+
+	*ctx = context;
+	*ctxlen = clen;
+	return 0;
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)
@@ -6965,6 +7004,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(object_init_security, selinux_object_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 v2 3/3] kernfs: Initialize security of newly created nodes

[Ondrej Mosnacek]

Use the new security_object_init_security() hook to allow LSMs to
possibly assign a non-default security context to newly created nodes
based on the context of their parent node.

This fixes an issue with cgroupfs under SELinux, where newly created
cgroup subdirectories 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:

    # mkdir /sys/fs/cgroup/unified/test
    # chcon -R system_u:object_r:cgroup_t:s0:c123 /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  8 05:00 cgroup.controllers
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.max.depth
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.max.descendants
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.procs
    -r--r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.stat
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.subtree_control
    -rw-r--r--.  1 root root system_u:object_r:cgroup_t:s0      0 Jan  8 05:00 cgroup.threads
    drwxr-xr-x.  2 root root system_u:object_r:cgroup_t:s0      0 Jan  8 04:54 init.scope
    drwxr-xr-x. 25 root root system_u:object_r:cgroup_t:s0      0 Jan  8 04:54 system.slice
    drwxr-xr-x.  2 root root system_u:object_r:cgroup_t:s0:c123 0 Jan  8 04:59 test
    drwxr-xr-x.  3 root root system_u:object_r:cgroup_t:s0      0 Jan  8 04:55 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  8 05:10 /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:cgroup_t:s0:c123 0 Jan  8 05:10 /sys/fs/cgroup/unified/test/subdir

Link: https://github.com/SELinuxProject/selinux-kernel/issues/39
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 fs/kernfs/dir.c             | 49 ++++++++++++++++++++++++++++++++++---
 fs/kernfs/inode.c           |  9 +++----
 fs/kernfs/kernfs-internal.h |  4 +++
 3 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 4ca0b5c18192..8a678a934f65 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"
 
@@ -617,7 +618,43 @@ struct kernfs_node *kernfs_node_from_dentry(struct dentry *dentry)
 	return NULL;
 }
 
-static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
+static int kernfs_node_init_security(struct kernfs_node *parent,
+				     struct kernfs_node *kn, umode_t mode)
+{
+	struct kernfs_iattrs *attrs;
+	struct qstr q;
+	void *ctx;
+	u32 ctxlen;
+	int ret;
+
+	/* If parent has no explicit context set, leave child unset as well */
+	if (!parent->iattr)
+		return 0;
+	if (!parent->iattr->ia_secdata || !parent->iattr->ia_secdata_len)
+		return 0;
+
+	q.name = kn->name;
+	q.hash_len = hashlen_string(parent, kn->name);
+
+	ret = security_object_init_security(parent->iattr->ia_secdata,
+					    parent->iattr->ia_secdata_len,
+					    &q, (u16)mode, &ctx, &ctxlen);
+	if (ret)
+		return ret;
+
+	attrs = kernfs_iattrs(kn);
+	if (!attrs) {
+		security_release_secctx(ctx, ctxlen);
+		return -ENOMEM;
+	}
+
+	kernfs_node_setsecdata(attrs, &ctx, &ctxlen);
+	/* The inode is fresh, so the returned ctx is always NULL. */
+	return 0;
+}
+
+static struct kernfs_node *__kernfs_new_node(struct kernfs_node *parent,
+					     struct kernfs_root *root,
 					     const char *name, umode_t mode,
 					     kuid_t uid, kgid_t gid,
 					     unsigned flags)
@@ -674,6 +711,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
 			goto err_out3;
 	}
 
+	if (parent) {
+		ret = kernfs_node_init_security(parent, kn, mode);
+		if (ret)
+			goto err_out3;
+	}
+
 	return kn;
 
  err_out3:
@@ -692,7 +735,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(parent, kernfs_root(parent),
 			       name, mode, uid, gid, flags);
 	if (kn) {
 		kernfs_get(parent);
@@ -962,7 +1005,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(NULL, root, "", 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 80cebcd94c90..e6db8d23437b 100644
--- a/fs/kernfs/inode.c
+++ b/fs/kernfs/inode.c
@@ -31,7 +31,7 @@ static const struct inode_operations kernfs_iops = {
 	.listxattr	= kernfs_iop_listxattr,
 };
 
-static struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
+struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn)
 {
 	static DEFINE_MUTEX(iattr_mutex);
 	struct kernfs_iattrs *ret;
@@ -135,8 +135,8 @@ out:
 	return error;
 }
 
-static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
-				  u32 *secdata_len)
+void kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
+			    u32 *secdata_len)
 {
 	void *old_secdata;
 	size_t old_secdata_len;
@@ -149,7 +149,6 @@ static int kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
 
 	*secdata = old_secdata;
 	*secdata_len = old_secdata_len;
-	return 0;
 }
 
 ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size)
@@ -365,7 +364,7 @@ static int kernfs_security_xattr_set(const struct xattr_handler *handler,
 		return error;
 
 	mutex_lock(&kernfs_mutex);
-	error = kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
+	kernfs_node_setsecdata(attrs, &secdata, &secdata_len);
 	mutex_unlock(&kernfs_mutex);
 
 	if (secdata)
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 3d83b114bb08..f6fb2df24c30 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -92,6 +92,10 @@ int kernfs_iop_getattr(const struct path *path, struct kstat *stat,
 ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size);
 int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr);
 
+struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn);
+void kernfs_node_setsecdata(struct kernfs_iattrs *attrs, void **secdata,
+			    u32 *secdata_len);
+
 /*
  * dir.c
  */
-- 
2.20.1

Re: [PATCH v2 3/3] kernfs: Initialize security of newly created nodes

[Tejun Heo]

On Wed, Jan 09, 2019 at 05:28:30PM +0100, Ondrej Mosnacek wrote:
> Use the new security_object_init_security() hook to allow LSMs to
> possibly assign a non-default security context to newly created nodes
> based on the context of their parent node.
> 
> This fixes an issue with cgroupfs under SELinux, where newly created
> cgroup subdirectories 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:

I'm not yet sure about using selinux on cgroupfs.  Let's please
discuss that first.

Thanks.

-- 
tejun

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Casey Schaufler]

On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> 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/
>
> This series adds a new security hook that allows to initialize the security
> context of kernfs properly, taking into account the parent context. 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. 

This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
and expected filesystem behavior?

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

An LSM might use information about the parent other than the "context".
Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
to determine whether the Smack label of the new object should be taken
from the parent or the process. Passing the "context" of the parent is
insufficient for Smack.

> 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 exist.
>
> Therefore, to allow maintaining similar behavior for kernfs nodes, a new LSM
> hook is needed, which would allow initializing the kernfs node's security
> context based on the context stored in the parent's node (if any).
>
> 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.
>
> The first patch adds the new LSM hook; the second patch implements the hook
> in SELinux; and the third patch modifies kernfs to use the new hook to
> initialize the security context of kernfs nodes whenever its parent node
> has a non-default context set.
>
> Note: the patches are based on current selinux/next [3], but they seem to
> apply cleanly on top of v5.0-rc1 as well.
>
> Testing:
> - passed SELinux testsuite on Fedora 29 (x86_64) when applied on top of
>   current Rawhide kernel (5.0.0-0.rc1.git0.1) [4]
> - passed the reproducer from the last patch
>
> [1] https://github.com/SELinuxProject/selinux-kernel/issues/39
> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/log/?h=selinux-pr-20181224
> [4] https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/842855/
>
> Ondrej Mosnacek (3):
>   LSM: Add new hook for generic node initialization
>   selinux: Implement the object_init_security hook
>   kernfs: Initialize security of newly created nodes
>
>  fs/kernfs/dir.c             | 49 ++++++++++++++++++++++++++++++++++---
>  fs/kernfs/inode.c           |  9 +++----
>  fs/kernfs/kernfs-internal.h |  4 +++
>  include/linux/lsm_hooks.h   | 30 +++++++++++++++++++++++
>  include/linux/security.h    | 14 +++++++++++
>  security/security.c         | 10 ++++++++
>  security/selinux/hooks.c    | 41 +++++++++++++++++++++++++++++++
>  7 files changed, 149 insertions(+), 8 deletions(-)
>

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Stephen Smalley]

On 1/9/19 12:19 PM, Casey Schaufler wrote:
> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>> 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/
>>
>> This series adds a new security hook that allows to initialize the security
>> context of kernfs properly, taking into account the parent context. 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.
> 
> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
> and expected filesystem behavior?

sysfs / kernfs didn't support xattrs at all when we first added support 
for setting security contexts to it, so originally all sysfs / kernfs 
inodes had a single security context, and we only required separate 
storage for the inodes that were explicitly labeled by userspace.

Later kernfs grew support for trusted.* xattrs using simple_xattrs but 
the existing security.* support was left mostly unchanged.

> 
>> 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
>> metadata.
>>
>> 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).
> 
> An LSM might use information about the parent other than the "context".
> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
> to determine whether the Smack label of the new object should be taken
> from the parent or the process. Passing the "context" of the parent is
> insufficient for Smack.

IIUC, this would involve switching the handling of security.* xattrs in 
kernfs over to use simple_xattrs too (so that we can store multiple such 
attributes), and then pass the entire simple_xattrs list or at least 
anything with a security.* prefix when initializing a new node or 
refreshing an existing inode.  Then the security module could extract 
any security.* attributes of interest for use in determining the label 
of new inodes and in refreshing the label of an inode.

> 
>> 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 exist.
>>
>> Therefore, to allow maintaining similar behavior for kernfs nodes, a new LSM
>> hook is needed, which would allow initializing the kernfs node's security
>> context based on the context stored in the parent's node (if any).
>>
>> 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.
>>
>> The first patch adds the new LSM hook; the second patch implements the hook
>> in SELinux; and the third patch modifies kernfs to use the new hook to
>> initialize the security context of kernfs nodes whenever its parent node
>> has a non-default context set.
>>
>> Note: the patches are based on current selinux/next [3], but they seem to
>> apply cleanly on top of v5.0-rc1 as well.
>>
>> Testing:
>> - passed SELinux testsuite on Fedora 29 (x86_64) when applied on top of
>>    current Rawhide kernel (5.0.0-0.rc1.git0.1) [4]
>> - passed the reproducer from the last patch
>>
>> [1] https://github.com/SELinuxProject/selinux-kernel/issues/39
>> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/log/?h=selinux-pr-20181224
>> [4] https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/842855/
>>
>> Ondrej Mosnacek (3):
>>    LSM: Add new hook for generic node initialization
>>    selinux: Implement the object_init_security hook
>>    kernfs: Initialize security of newly created nodes
>>
>>   fs/kernfs/dir.c             | 49 ++++++++++++++++++++++++++++++++++---
>>   fs/kernfs/inode.c           |  9 +++----
>>   fs/kernfs/kernfs-internal.h |  4 +++
>>   include/linux/lsm_hooks.h   | 30 +++++++++++++++++++++++
>>   include/linux/security.h    | 14 +++++++++++
>>   security/security.c         | 10 ++++++++
>>   security/selinux/hooks.c    | 41 +++++++++++++++++++++++++++++++
>>   7 files changed, 149 insertions(+), 8 deletions(-)
>>
> 

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Casey Schaufler]

On 1/9/2019 12:37 PM, Stephen Smalley wrote:
> On 1/9/19 12:19 PM, Casey Schaufler wrote:
>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>> 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/
>>>
>>> This series adds a new security hook that allows to initialize the security
>>> context of kernfs properly, taking into account the parent context. 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.
>>
>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
>> and expected filesystem behavior?
>
> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
>
> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.

OK, so as I said, this seems like a bug in kernfs.

>
>>
>>> 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
>>> metadata.
>>>
>>> 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).
>>
>> An LSM might use information about the parent other than the "context".
>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
>> to determine whether the Smack label of the new object should be taken
>> from the parent or the process. Passing the "context" of the parent is
>> insufficient for Smack.
>
> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.

Right. But I'll point out that there is nothing to prevent an
LSM from using inode information outside of the xattrs (e.g. uids)
to determine the security state it wants to give a new object.

I suggest that the better solution would be for kernfs to
use inodes like a real filesystem. Every special case like this
results in special cases like this special hook. It's hard
enough to keep track of the general case in the Linux kernel.

>
>>
>>> 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 exist.
>>>
>>> Therefore, to allow maintaining similar behavior for kernfs nodes, a new LSM
>>> hook is needed, which would allow initializing the kernfs node's security
>>> context based on the context stored in the parent's node (if any).
>>>
>>> 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.
>>>
>>> The first patch adds the new LSM hook; the second patch implements the hook
>>> in SELinux; and the third patch modifies kernfs to use the new hook to
>>> initialize the security context of kernfs nodes whenever its parent node
>>> has a non-default context set.
>>>
>>> Note: the patches are based on current selinux/next [3], but they seem to
>>> apply cleanly on top of v5.0-rc1 as well.
>>>
>>> Testing:
>>> - passed SELinux testsuite on Fedora 29 (x86_64) when applied on top of
>>>    current Rawhide kernel (5.0.0-0.rc1.git0.1) [4]
>>> - passed the reproducer from the last patch
>>>
>>> [1] https://github.com/SELinuxProject/selinux-kernel/issues/39
>>> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
>>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/log/?h=selinux-pr-20181224
>>> [4] https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/842855/
>>>
>>> Ondrej Mosnacek (3):
>>>    LSM: Add new hook for generic node initialization
>>>    selinux: Implement the object_init_security hook
>>>    kernfs: Initialize security of newly created nodes
>>>
>>>   fs/kernfs/dir.c             | 49 ++++++++++++++++++++++++++++++++++---
>>>   fs/kernfs/inode.c           |  9 +++----
>>>   fs/kernfs/kernfs-internal.h |  4 +++
>>>   include/linux/lsm_hooks.h   | 30 +++++++++++++++++++++++
>>>   include/linux/security.h    | 14 +++++++++++
>>>   security/security.c         | 10 ++++++++
>>>   security/selinux/hooks.c    | 41 +++++++++++++++++++++++++++++++
>>>   7 files changed, 149 insertions(+), 8 deletions(-)
>>>
>>
>
>

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Stephen Smalley]

On 1/9/19 5:03 PM, Casey Schaufler wrote:
> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>>> 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/
>>>>
>>>> This series adds a new security hook that allows to initialize the security
>>>> context of kernfs properly, taking into account the parent context. 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.
>>>
>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
>>> and expected filesystem behavior?
>>
>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
>>
>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
> 
> OK, so as I said, this seems like a bug in kernfs.
> 
>>
>>>
>>>> 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
>>>> metadata.
>>>>
>>>> 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).
>>>
>>> An LSM might use information about the parent other than the "context".
>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
>>> to determine whether the Smack label of the new object should be taken
>>> from the parent or the process. Passing the "context" of the parent is
>>> insufficient for Smack.
>>
>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
> 
> Right. But I'll point out that there is nothing to prevent an
> LSM from using inode information outside of the xattrs (e.g. uids)
> to determine the security state it wants to give a new object.

If that's a real concern, the hook could pass the ia_iattr structure in 
addition to the simple_xattrs list and the security module could use any 
inode attributes it likes in making the decision.  Effectively it would 
be passing the entire kernfs_iattrs structure, but probably not directly 
since that definition is presently private to kernfs.

> I suggest that the better solution would be for kernfs to
> use inodes like a real filesystem. Every special case like this
> results in special cases like this special hook. It's hard
> enough to keep track of the general case in the Linux kernel.

Feel free to propose an implementation if you like, but doing a complete 
rewrite of kernfs internals seems a bit out of scope.

> 
>>
>>>
>>>> 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 exist.
>>>>
>>>> Therefore, to allow maintaining similar behavior for kernfs nodes, a new LSM
>>>> hook is needed, which would allow initializing the kernfs node's security
>>>> context based on the context stored in the parent's node (if any).
>>>>
>>>> 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.
>>>>
>>>> The first patch adds the new LSM hook; the second patch implements the hook
>>>> in SELinux; and the third patch modifies kernfs to use the new hook to
>>>> initialize the security context of kernfs nodes whenever its parent node
>>>> has a non-default context set.
>>>>
>>>> Note: the patches are based on current selinux/next [3], but they seem to
>>>> apply cleanly on top of v5.0-rc1 as well.
>>>>
>>>> Testing:
>>>> - passed SELinux testsuite on Fedora 29 (x86_64) when applied on top of
>>>>     current Rawhide kernel (5.0.0-0.rc1.git0.1) [4]
>>>> - passed the reproducer from the last patch
>>>>
>>>> [1] https://github.com/SELinuxProject/selinux-kernel/issues/39
>>>> [2] https://bugzilla.redhat.com/show_bug.cgi?id=1553803
>>>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/log/?h=selinux-pr-20181224
>>>> [4] https://copr.fedorainfracloud.org/coprs/omos/kernel-testing/build/842855/
>>>>
>>>> Ondrej Mosnacek (3):
>>>>     LSM: Add new hook for generic node initialization
>>>>     selinux: Implement the object_init_security hook
>>>>     kernfs: Initialize security of newly created nodes
>>>>
>>>>    fs/kernfs/dir.c             | 49 ++++++++++++++++++++++++++++++++++---
>>>>    fs/kernfs/inode.c           |  9 +++----
>>>>    fs/kernfs/kernfs-internal.h |  4 +++
>>>>    include/linux/lsm_hooks.h   | 30 +++++++++++++++++++++++
>>>>    include/linux/security.h    | 14 +++++++++++
>>>>    security/security.c         | 10 ++++++++
>>>>    security/selinux/hooks.c    | 41 +++++++++++++++++++++++++++++++
>>>>    7 files changed, 149 insertions(+), 8 deletions(-)
>>>>
>>>
>>
>>
> 

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Casey Schaufler]

Resending after email configuration repair.

On 1/10/2019 6:15 AM, Stephen Smalley wrote:
> On 1/9/19 5:03 PM, Casey Schaufler wrote:
>> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
>>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
>>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>>>> 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/
>>>>>
>>>>> This series adds a new security hook that allows to initialize the security
>>>>> context of kernfs properly, taking into account the parent context. 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.
>>>>
>>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
>>>> and expected filesystem behavior?
>>>
>>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
>>>
>>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
>>
>> OK, so as I said, this seems like a bug in kernfs.
>>
>>>
>>>>
>>>>> 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
>>>>> metadata.
>>>>>
>>>>> 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).
>>>>
>>>> An LSM might use information about the parent other than the "context".
>>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
>>>> to determine whether the Smack label of the new object should be taken
>>>> from the parent or the process. Passing the "context" of the parent is
>>>> insufficient for Smack.
>>>
>>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
>>
>> Right. But I'll point out that there is nothing to prevent an
>> LSM from using inode information outside of the xattrs (e.g. uids)
>> to determine the security state it wants to give a new object.
>
> If that's a real concern, the hook could pass the ia_iattr structure in addition to the simple_xattrs list and the security module could use any inode attributes it likes in making the decision.  Effectively it would be passing the entire kernfs_iattrs structure, but probably not directly since that definition is presently private to kernfs.

Yes, it's a real concern. And no, just passing all of the kernfs internal data
out in j-random formats does not pass muster. Al Viro was commenting the other
day on how bad the LSM infrastructure interfaces are. The original proposal here
is already big, cluttered and inadequate. Adding more to it to make up for its
shortcomings should be sending up red flags.

I've been wallowing in the LSM infrastructure for the past seven years.
Interfaces like this one, that propagate the idiosyncrasies of both
the caller (kernfs) and one potential callee (SELinux) are much too
common. I understand that there is a problem that needs a solution.
This isn't it.


>> I suggest that the better solution would be for kernfs to
>> use inodes like a real filesystem. Every special case like this
>> results in special cases like this special hook. It's hard
>> enough to keep track of the general case in the Linux kernel.
>
> Feel free to propose an implementation if you like, but doing a complete rewrite of kernfs internals seems a bit out of scope.

If this issue points out a serious problem with the kernfs implementation
then I would expect that addressing the problem at its source would be in
everyone's best interest. Did anyone even look at the possibility? If I
said that I would do that, how long would you be willing to wait for it?

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Stephen Smalley]

On 1/10/19 12:54 PM, Casey Schaufler wrote:
> 
> Resending after email configuration repair.
> 
> On 1/10/2019 6:15 AM, Stephen Smalley wrote:
>> On 1/9/19 5:03 PM, Casey Schaufler wrote:
>>> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
>>>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
>>>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>>>>> 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/
>>>>>>
>>>>>> This series adds a new security hook that allows to initialize the security
>>>>>> context of kernfs properly, taking into account the parent context. 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.
>>>>>
>>>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
>>>>> and expected filesystem behavior?
>>>>
>>>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
>>>>
>>>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
>>>
>>> OK, so as I said, this seems like a bug in kernfs.
>>>
>>>>
>>>>>
>>>>>> 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
>>>>>> metadata.
>>>>>>
>>>>>> 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).
>>>>>
>>>>> An LSM might use information about the parent other than the "context".
>>>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
>>>>> to determine whether the Smack label of the new object should be taken
>>>>> from the parent or the process. Passing the "context" of the parent is
>>>>> insufficient for Smack.
>>>>
>>>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
>>>
>>> Right. But I'll point out that there is nothing to prevent an
>>> LSM from using inode information outside of the xattrs (e.g. uids)
>>> to determine the security state it wants to give a new object.
>>
>> If that's a real concern, the hook could pass the ia_iattr structure in addition to the simple_xattrs list and the security module could use any inode attributes it likes in making the decision.  Effectively it would be passing the entire kernfs_iattrs structure, but probably not directly since that definition is presently private to kernfs.
> 
> Yes, it's a real concern. And no, just passing all of the kernfs internal data
> out in j-random formats does not pass muster. Al Viro was commenting the other
> day on how bad the LSM infrastructure interfaces are. The original proposal here
> is already big, cluttered and inadequate. Adding more to it to make up for its
> shortcomings should be sending up red flags
I don't quite see how the original patch set or hook can be called big 
and cluttered.  Switching the handling of security xattrs in kernfs to 
use simple_xattrs (a natural and seemingly straightforward cleanup) and 
passing the entire simple_xattrs list to the hook interface would allow 
you to support SMACK64TRANSMUTE, which was the one actual inadequacy you 
identified.  You claim that someone might need/want the parent uid/gid 
too, but there are no in-tree security modules that do so nor any 
submitted AFAIK, and if that situation arises, all we need to do to 
support it is to add the iattrs.  Obviously they can all be wrapped up 
in some larger structure if desired. At that point the security modules 
would have access to all of the inode attributes supported by kernfs.

> I've been wallowing in the LSM infrastructure for the past seven years.
> Interfaces like this one, that propagate the idiosyncrasies of both
> the caller (kernfs) and one potential callee (SELinux) are much too
> common. I understand that there is a problem that needs a solution.
> This isn't it.
The solution sketched above should be capable of supporting the needs of 
any current security modules and of being easily extended for others if 
the need arises.

> 
> 
>>> I suggest that the better solution would be for kernfs to
>>> use inodes like a real filesystem. Every special case like this
>>> results in special cases like this special hook. It's hard
>>> enough to keep track of the general case in the Linux kernel.
>>
>> Feel free to propose an implementation if you like, but doing a complete rewrite of kernfs internals seems a bit out of scope.
> 
> If this issue points out a serious problem with the kernfs implementation
> then I would expect that addressing the problem at its source would be in
> everyone's best interest. Did anyone even look at the possibility? If I
> said that I would do that, how long would you be willing to wait for it?

I don't know that it points to a serious problem with kernfs.  But I'll 
let you convince the kernfs maintainers of that. Meanwhile, we have a 
proposed solution that solves the problem for all in-tree security 
modules.  I see no reason to hold that up. Don't over-design.

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Paul Moore]

On Thu, Jan 10, 2019 at 2:36 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 1/10/19 12:54 PM, Casey Schaufler wrote:
> > Resending after email configuration repair.
> >
> > On 1/10/2019 6:15 AM, Stephen Smalley wrote:
> >> On 1/9/19 5:03 PM, Casey Schaufler wrote:
> >>> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
> >>>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
> >>>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> >>>>>> 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/
> >>>>>>
> >>>>>> This series adds a new security hook that allows to initialize the security
> >>>>>> context of kernfs properly, taking into account the parent context. 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.
> >>>>>
> >>>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
> >>>>> and expected filesystem behavior?
> >>>>
> >>>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
> >>>>
> >>>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
> >>>
> >>> OK, so as I said, this seems like a bug in kernfs.
> >>>
> >>>>
> >>>>>
> >>>>>> 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
> >>>>>> metadata.
> >>>>>>
> >>>>>> 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).
> >>>>>
> >>>>> An LSM might use information about the parent other than the "context".
> >>>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
> >>>>> to determine whether the Smack label of the new object should be taken
> >>>>> from the parent or the process. Passing the "context" of the parent is
> >>>>> insufficient for Smack.
> >>>>
> >>>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
> >>>
> >>> Right. But I'll point out that there is nothing to prevent an
> >>> LSM from using inode information outside of the xattrs (e.g. uids)
> >>> to determine the security state it wants to give a new object.
> >>
> >> If that's a real concern, the hook could pass the ia_iattr structure in addition to the simple_xattrs list and the security module could use any inode attributes it likes in making the decision.  Effectively it would be passing the entire kernfs_iattrs structure, but probably not directly since that definition is presently private to kernfs.
> >
> > Yes, it's a real concern. And no, just passing all of the kernfs internal data
> > out in j-random formats does not pass muster. Al Viro was commenting the other
> > day on how bad the LSM infrastructure interfaces are. The original proposal here
> > is already big, cluttered and inadequate. Adding more to it to make up for its
> > shortcomings should be sending up red flags
>
> I don't quite see how the original patch set or hook can be called big
> and cluttered.  Switching the handling of security xattrs in kernfs to
> use simple_xattrs (a natural and seemingly straightforward cleanup) and
> passing the entire simple_xattrs list to the hook interface would allow
> you to support SMACK64TRANSMUTE, which was the one actual inadequacy you
> identified.  You claim that someone might need/want the parent uid/gid
> too, but there are no in-tree security modules that do so nor any
> submitted AFAIK, and if that situation arises, all we need to do to
> support it is to add the iattrs.  Obviously they can all be wrapped up
> in some larger structure if desired. At that point the security modules
> would have access to all of the inode attributes supported by kernfs.

I'm with Stephen on this; if Ondrej changes it over to simple_xattrs
as described above so that Smack would have what it needs, I don't see
why we should hold off on this.

Everything we are talking about is a kernel internal issue, we can
change it as needed to take into account new LSMs or new functionality
in existing LSMs.

Ondrej, a gentle reminder that it would be nice to have a simple
selinux-testsuite test to make sure we are labeling
kernfs-based/cgroup files correctly.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Ondrej Mosnacek]

On Fri, Jan 11, 2019 at 3:44 AM Paul Moore <paul@paul-moore.com> wrote:
> [...]
>
> Ondrej, a gentle reminder that it would be nice to have a simple
> selinux-testsuite test to make sure we are labeling
> kernfs-based/cgroup files correctly.

OK, I'll see if I can adapt the reproducer from the last patch into
the testsuite.



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

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Casey Schaufler]

On 1/10/2019 11:37 AM, Stephen Smalley wrote:
> On 1/10/19 12:54 PM, Casey Schaufler wrote:
>>
>> Resending after email configuration repair.
>>
>> On 1/10/2019 6:15 AM, Stephen Smalley wrote:
>>> On 1/9/19 5:03 PM, Casey Schaufler wrote:
>>>> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
>>>>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
>>>>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>>>>>> 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/
>>>>>>>
>>>>>>> This series adds a new security hook that allows to initialize the security
>>>>>>> context of kernfs properly, taking into account the parent context. 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.
>>>>>>
>>>>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
>>>>>> and expected filesystem behavior?
>>>>>
>>>>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
>>>>>
>>>>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
>>>>
>>>> OK, so as I said, this seems like a bug in kernfs.
>>>>
>>>>>
>>>>>>
>>>>>>> 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
>>>>>>> metadata.
>>>>>>>
>>>>>>> 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).
>>>>>>
>>>>>> An LSM might use information about the parent other than the "context".
>>>>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
>>>>>> to determine whether the Smack label of the new object should be taken
>>>>>> from the parent or the process. Passing the "context" of the parent is
>>>>>> insufficient for Smack.
>>>>>
>>>>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
>>>>
>>>> Right. But I'll point out that there is nothing to prevent an
>>>> LSM from using inode information outside of the xattrs (e.g. uids)
>>>> to determine the security state it wants to give a new object.
>>>
>>> If that's a real concern, the hook could pass the ia_iattr structure in addition to the simple_xattrs list and the security module could use any inode attributes it likes in making the decision.  Effectively it would be passing the entire kernfs_iattrs structure, but probably not directly since that definition is presently private to kernfs.
>>
>> Yes, it's a real concern. And no, just passing all of the kernfs internal data
>> out in j-random formats does not pass muster. Al Viro was commenting the other
>> day on how bad the LSM infrastructure interfaces are. The original proposal here
>> is already big, cluttered and inadequate. Adding more to it to make up for its
>> shortcomings should be sending up red flags
> I don't quite see how the original patch set or hook can be called big and cluttered.  Switching the handling of security xattrs in kernfs to use simple_xattrs (a natural and seemingly straightforward cleanup) and passing the entire simple_xattrs list to the hook interface would allow you to support SMACK64TRANSMUTE, which was the one actual inadequacy you identified.  You claim that someone might need/want the parent uid/gid too, but there are no in-tree security modules that do so nor any submitted AFAIK, and if that situation arises, all we need to do to support it is to add the iattrs.  Obviously they can all be wrapped up in some larger structure if desired. At that point the security modules would have access to all of the inode attributes supported by kernfs.

We already have a structure to wrap all this in. It's an inode.

But, as you point out, there are no in tree LSMs that use
anything beyond the xattrs. So the change you're suggesting
is arguably sufficient, and considerably easier.

>
>> I've been wallowing in the LSM infrastructure for the past seven years.
>> Interfaces like this one, that propagate the idiosyncrasies of both
>> the caller (kernfs) and one potential callee (SELinux) are much too
>> common. I understand that there is a problem that needs a solution.
>> This isn't it.
> The solution sketched above should be capable of supporting the needs of any current security modules and of being easily extended for others if the need arises.

It, like security_inode_init(), is going to be
challenging to get right in the stacking environment.
But, that's my problem.

>
>>
>>
>>>> I suggest that the better solution would be for kernfs to
>>>> use inodes like a real filesystem. Every special case like this
>>>> results in special cases like this special hook. It's hard
>>>> enough to keep track of the general case in the Linux kernel.
>>>
>>> Feel free to propose an implementation if you like, but doing a complete rewrite of kernfs internals seems a bit out of scope.
>>
>> If this issue points out a serious problem with the kernfs implementation
>> then I would expect that addressing the problem at its source would be in
>> everyone's best interest. Did anyone even look at the possibility? If I
>> said that I would do that, how long would you be willing to wait for it?
>
> I don't know that it points to a serious problem with kernfs.  But I'll let you convince the kernfs maintainers of that. Meanwhile, we have a proposed solution that solves the problem for all in-tree security modules.  I see no reason to hold that up. Don't over-design.

I think the patch presented was hasty. It clearly didn't
account for any LSM but SELinux. I understand why. Adding an
LSM interface needs to account for the entire security sub-system.

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Ondrej Mosnacek]

On Thu, Jan 10, 2019 at 6:55 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> Resending after email configuration repair.
>
> On 1/10/2019 6:15 AM, Stephen Smalley wrote:
> > On 1/9/19 5:03 PM, Casey Schaufler wrote:
> >> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
> >>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
> >>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> >>>>> 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/
> >>>>>
> >>>>> This series adds a new security hook that allows to initialize the security
> >>>>> context of kernfs properly, taking into account the parent context. 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.
> >>>>
> >>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
> >>>> and expected filesystem behavior?
> >>>
> >>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
> >>>
> >>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
> >>
> >> OK, so as I said, this seems like a bug in kernfs.
> >>
> >>>
> >>>>
> >>>>> 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
> >>>>> metadata.
> >>>>>
> >>>>> 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).
> >>>>
> >>>> An LSM might use information about the parent other than the "context".
> >>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
> >>>> to determine whether the Smack label of the new object should be taken
> >>>> from the parent or the process. Passing the "context" of the parent is
> >>>> insufficient for Smack.
> >>>
> >>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.

I actually had a patch to do just that at one point because I thought
for a while that it would be required to call
security_inode_init_security() (which I had tried to somehow force
into the kernfs node creation at some point), but then I realized it
is not actually needed (although would make thing a bit nicer) and put
it away... I will try to dig it out and reuse here.

> >>
> >> Right. But I'll point out that there is nothing to prevent an
> >> LSM from using inode information outside of the xattrs (e.g. uids)
> >> to determine the security state it wants to give a new object.
> >
> > If that's a real concern, the hook could pass the ia_iattr structure in addition to the simple_xattrs list and the security module could use any inode attributes it likes in making the decision.  Effectively it would be passing the entire kernfs_iattrs structure, but probably not directly since that definition is presently private to kernfs.
>
> Yes, it's a real concern. And no, just passing all of the kernfs internal data
> out in j-random formats does not pass muster. Al Viro was commenting the other
> day on how bad the LSM infrastructure interfaces are. The original proposal here
> is already big, cluttered and inadequate. Adding more to it to make up for its
> shortcomings should be sending up red flags.

I understand the concern about cluttering up things, but I just don't
see any nicer solution right now...

>
> I've been wallowing in the LSM infrastructure for the past seven years.
> Interfaces like this one, that propagate the idiosyncrasies of both
> the caller (kernfs) and one potential callee (SELinux) are much too
> common. I understand that there is a problem that needs a solution.
> This isn't it.
>
>
> >> I suggest that the better solution would be for kernfs to
> >> use inodes like a real filesystem. Every special case like this
> >> results in special cases like this special hook. It's hard
> >> enough to keep track of the general case in the Linux kernel.
> >
> > Feel free to propose an implementation if you like, but doing a complete rewrite of kernfs internals seems a bit out of scope.
>
> If this issue points out a serious problem with the kernfs implementation
> then I would expect that addressing the problem at its source would be in
> everyone's best interest. Did anyone even look at the possibility? If I
> said that I would do that, how long would you be willing to wait for it?

Granted, the "inodeless" abstractions in kernfs have perhaps gone too
far, but I believe that trying undo it would just shift the complexity
into kernfs and its users... IMHO that this solution (with the changes
proposed by Stephen) is not overly invasive and does not make the
potential future rework of kernfs and its handling by LSMs much more
difficult than it would be now. I'd prefer to apply an imperfect but
noninvasive solution to a practical problem now and leave extensive
refactoring as a separate task/discussion.

(Anyway, I don't want to rush this. I'll keep sending patches and
hopefully we'll eventually converge to some solution acceptable to
everyone.)

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

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Ondrej Mosnacek]

On Mon, Jan 14, 2019 at 10:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Jan 10, 2019 at 6:55 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > Resending after email configuration repair.
> >
> > On 1/10/2019 6:15 AM, Stephen Smalley wrote:
> > > On 1/9/19 5:03 PM, Casey Schaufler wrote:
> > >> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
> > >>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
> > >>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
> > >>>>> 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/
> > >>>>>
> > >>>>> This series adds a new security hook that allows to initialize the security
> > >>>>> context of kernfs properly, taking into account the parent context. 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.
> > >>>>
> > >>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
> > >>>> and expected filesystem behavior?
> > >>>
> > >>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
> > >>>
> > >>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
> > >>
> > >> OK, so as I said, this seems like a bug in kernfs.
> > >>
> > >>>
> > >>>>
> > >>>>> 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
> > >>>>> metadata.
> > >>>>>
> > >>>>> 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).
> > >>>>
> > >>>> An LSM might use information about the parent other than the "context".
> > >>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
> > >>>> to determine whether the Smack label of the new object should be taken
> > >>>> from the parent or the process. Passing the "context" of the parent is
> > >>>> insufficient for Smack.
> > >>>
> > >>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
>
> I actually had a patch to do just that at one point because I thought
> for a while that it would be required to call
> security_inode_init_security() (which I had tried to somehow force
> into the kernfs node creation at some point), but then I realized it
> is not actually needed (although would make thing a bit nicer) and put
> it away... I will try to dig it out and reuse here.

Okay, now that I tried to do this with full xattr support I ran into a
problem. Along with converting kernfs to use simple_xattrs for
security attributes, I removed the call to
security_inode_notifysecctx() from kernfs_refresh_inode(), as it no
longer makes sense (kernfs doesn't know which attribute contains the
context; the LSM should now be able to pull it out via
vfs_getxattr()). However, SELinux now doesn't set the right security
context in the selinux_d_instantiate() hook, because the policy tells
it to use genfs, not xattr.

So... I'm not sure how to fix this. Setting fs_use_xattr for cgroupfs
in the policy won't work, because then all nodes will be unlabeled_t
by default. Maybe we could patch the genfs case in
inode_doinit_with_dentry() to try fetching the xattr first? I'm not
very confident about touching that part of the code, so I would
welcome some advice here.

This is the code I have so far, in case it helps:
https://gitlab.com/omos/linux-public/compare/selinux-next...selinux-fix-cgroupfs-v8

Thanks,

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

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Stephen Smalley]

On 1/22/19 3:49 AM, Ondrej Mosnacek wrote:
> On Mon, Jan 14, 2019 at 10:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> On Thu, Jan 10, 2019 at 6:55 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> Resending after email configuration repair.
>>>
>>> On 1/10/2019 6:15 AM, Stephen Smalley wrote:
>>>> On 1/9/19 5:03 PM, Casey Schaufler wrote:
>>>>> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
>>>>>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
>>>>>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>>>>>>> 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/
>>>>>>>>
>>>>>>>> This series adds a new security hook that allows to initialize the security
>>>>>>>> context of kernfs properly, taking into account the parent context. 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.
>>>>>>>
>>>>>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to the usual
>>>>>>> and expected filesystem behavior?
>>>>>>
>>>>>> sysfs / kernfs didn't support xattrs at all when we first added support for setting security contexts to it, so originally all sysfs / kernfs inodes had a single security context, and we only required separate storage for the inodes that were explicitly labeled by userspace.
>>>>>>
>>>>>> Later kernfs grew support for trusted.* xattrs using simple_xattrs but the existing security.* support was left mostly unchanged.
>>>>>
>>>>> OK, so as I said, this seems like a bug in kernfs.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>> 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
>>>>>>>> metadata.
>>>>>>>>
>>>>>>>> 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).
>>>>>>>
>>>>>>> An LSM might use information about the parent other than the "context".
>>>>>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the parent
>>>>>>> to determine whether the Smack label of the new object should be taken
>>>>>>> from the parent or the process. Passing the "context" of the parent is
>>>>>>> insufficient for Smack.
>>>>>>
>>>>>> IIUC, this would involve switching the handling of security.* xattrs in kernfs over to use simple_xattrs too (so that we can store multiple such attributes), and then pass the entire simple_xattrs list or at least anything with a security.* prefix when initializing a new node or refreshing an existing inode.  Then the security module could extract any security.* attributes of interest for use in determining the label of new inodes and in refreshing the label of an inode.
>>
>> I actually had a patch to do just that at one point because I thought
>> for a while that it would be required to call
>> security_inode_init_security() (which I had tried to somehow force
>> into the kernfs node creation at some point), but then I realized it
>> is not actually needed (although would make thing a bit nicer) and put
>> it away... I will try to dig it out and reuse here.
> 
> Okay, now that I tried to do this with full xattr support I ran into a
> problem. Along with converting kernfs to use simple_xattrs for
> security attributes, I removed the call to
> security_inode_notifysecctx() from kernfs_refresh_inode(), as it no
> longer makes sense (kernfs doesn't know which attribute contains the
> context; the LSM should now be able to pull it out via
> vfs_getxattr()). However, SELinux now doesn't set the right security
> context in the selinux_d_instantiate() hook, because the policy tells
> it to use genfs, not xattr.
> 
> So... I'm not sure how to fix this. Setting fs_use_xattr for cgroupfs
> in the policy won't work, because then all nodes will be unlabeled_t
> by default. Maybe we could patch the genfs case in
> inode_doinit_with_dentry() to try fetching the xattr first? I'm not
> very confident about touching that part of the code, so I would
> welcome some advice here.
> 
> This is the code I have so far, in case it helps:
> https://gitlab.com/omos/linux-public/compare/selinux-next...selinux-fix-cgroupfs-v8

I would have left security_inode_notifysecctx() or an equivalent that 
passes all of the xattrs to push the security attributes to the security 
module.

Blindly calling __vfs_getxattr() on genfs could be a problem; IIRC, 
doing so on fuse filesytems can create a deadlock during mount.  Or at 
least that was the issue with switching fuse to fs_use_xattr in the past.

Re: [PATCH v2 0/3] Allow initializing the kernfs node's secctx based on its parent

[Stephen Smalley]

On 1/22/19 9:17 AM, Stephen Smalley wrote:
> On 1/22/19 3:49 AM, Ondrej Mosnacek wrote:
>> On Mon, Jan 14, 2019 at 10:01 AM Ondrej Mosnacek <omosnace@redhat.com> 
>> wrote:
>>> On Thu, Jan 10, 2019 at 6:55 PM Casey Schaufler 
>>> <casey@schaufler-ca.com> wrote:
>>>> Resending after email configuration repair.
>>>>
>>>> On 1/10/2019 6:15 AM, Stephen Smalley wrote:
>>>>> On 1/9/19 5:03 PM, Casey Schaufler wrote:
>>>>>> On 1/9/2019 12:37 PM, Stephen Smalley wrote:
>>>>>>> On 1/9/19 12:19 PM, Casey Schaufler wrote:
>>>>>>>> On 1/9/2019 8:28 AM, Ondrej Mosnacek wrote:
>>>>>>>>> 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/ 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This series adds a new security hook that allows to initialize 
>>>>>>>>> the security
>>>>>>>>> context of kernfs properly, taking into account the parent 
>>>>>>>>> context. 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.
>>>>>>>>
>>>>>>>> This seems like a bug in kernfs. Why doesn't kernfs adhere to 
>>>>>>>> the usual
>>>>>>>> and expected filesystem behavior?
>>>>>>>
>>>>>>> sysfs / kernfs didn't support xattrs at all when we first added 
>>>>>>> support for setting security contexts to it, so originally all 
>>>>>>> sysfs / kernfs inodes had a single security context, and we only 
>>>>>>> required separate storage for the inodes that were explicitly 
>>>>>>> labeled by userspace.
>>>>>>>
>>>>>>> Later kernfs grew support for trusted.* xattrs using 
>>>>>>> simple_xattrs but the existing security.* support was left mostly 
>>>>>>> unchanged.
>>>>>>
>>>>>> OK, so as I said, this seems like a bug in kernfs.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> 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
>>>>>>>>> metadata.
>>>>>>>>>
>>>>>>>>> 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).
>>>>>>>>
>>>>>>>> An LSM might use information about the parent other than the 
>>>>>>>> "context".
>>>>>>>> Smack, for example, uses an attribute SMACK64TRANSMUTE from the 
>>>>>>>> parent
>>>>>>>> to determine whether the Smack label of the new object should be 
>>>>>>>> taken
>>>>>>>> from the parent or the process. Passing the "context" of the 
>>>>>>>> parent is
>>>>>>>> insufficient for Smack.
>>>>>>>
>>>>>>> IIUC, this would involve switching the handling of security.* 
>>>>>>> xattrs in kernfs over to use simple_xattrs too (so that we can 
>>>>>>> store multiple such attributes), and then pass the entire 
>>>>>>> simple_xattrs list or at least anything with a security.* prefix 
>>>>>>> when initializing a new node or refreshing an existing inode.  
>>>>>>> Then the security module could extract any security.* attributes 
>>>>>>> of interest for use in determining the label of new inodes and in 
>>>>>>> refreshing the label of an inode.
>>>
>>> I actually had a patch to do just that at one point because I thought
>>> for a while that it would be required to call
>>> security_inode_init_security() (which I had tried to somehow force
>>> into the kernfs node creation at some point), but then I realized it
>>> is not actually needed (although would make thing a bit nicer) and put
>>> it away... I will try to dig it out and reuse here.
>>
>> Okay, now that I tried to do this with full xattr support I ran into a
>> problem. Along with converting kernfs to use simple_xattrs for
>> security attributes, I removed the call to
>> security_inode_notifysecctx() from kernfs_refresh_inode(), as it no
>> longer makes sense (kernfs doesn't know which attribute contains the
>> context; the LSM should now be able to pull it out via
>> vfs_getxattr()). However, SELinux now doesn't set the right security
>> context in the selinux_d_instantiate() hook, because the policy tells
>> it to use genfs, not xattr.
>>
>> So... I'm not sure how to fix this. Setting fs_use_xattr for cgroupfs
>> in the policy won't work, because then all nodes will be unlabeled_t
>> by default. Maybe we could patch the genfs case in
>> inode_doinit_with_dentry() to try fetching the xattr first? I'm not
>> very confident about touching that part of the code, so I would
>> welcome some advice here.
>>
>> This is the code I have so far, in case it helps:
>> https://gitlab.com/omos/linux-public/compare/selinux-next...selinux-fix-cgroupfs-v8 
>>
> 
> I would have left security_inode_notifysecctx() or an equivalent that 
> passes all of the xattrs to push the security attributes to the security 
> module.
> 
> Blindly calling __vfs_getxattr() on genfs could be a problem; IIRC, 
> doing so on fuse filesytems can create a deadlock during mount.  Or at 
> least that was the issue with switching fuse to fs_use_xattr in the past.

See commits 4d546f81717d253ab67643bf072c6d8821a9249c, 
102aefdda4d8275ce7d7100bc16c88c74272b260, 
089be43e403a78cd6889cde2fba164fefe9dfd89, 
811f3799279e567aa354c649ce22688d949ac7a9 and
https://bugzilla.redhat.com/show_bug.cgi?id=1256635#c34 for some prior 
work and discussions in this area.