[v3,5/5] kernfs: initialize security of newly created nodes
diff mbox series

Message ID 20190130114150.27807-6-omosnace@redhat.com
State New
Headers show
Series
  • Allow initializing the kernfs node's secctx based on its parent
Related show

Commit Message

Ondrej Mosnacek Jan. 30, 2019, 11:41 a.m. UTC
Use the new security_kernfs_init_security() hook to allow LSMs to
possibly assign a non-default security context to a newly created kernfs
node based on the attributes of the new node and also its parent node.

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

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

Actual result:

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

Expected result:

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

Link: https://github.com/SELinuxProject/selinux-kernel/issues/39
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/kernfs/dir.c             | 35 +++++++++++++++++++++++++++++++++--
 fs/kernfs/inode.c           |  2 +-
 fs/kernfs/kernfs-internal.h |  1 +
 3 files changed, 35 insertions(+), 3 deletions(-)

Comments

Tejun Heo Jan. 30, 2019, 5:09 p.m. UTC | #1
Hello,

On Wed, Jan 30, 2019 at 12:41:50PM +0100, Ondrej Mosnacek wrote:
> @@ -673,6 +698,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
>  			goto err_out3;
>  	}
>  
> +	if (parent) {
> +		ret = kernfs_node_init_security(parent, kn);
> +		if (ret)
> +			goto err_out3;
> +	}

So, doing this unconditionally isn't a good idea.  kernfs doesn't use
the usual dentry/inode because there are machines with 6, even 7 digit
number of kernfs nodes and some of them even failed to boot due to
memory shortage.  Please don't blow it up by default.

Thanks.
Ondrej Mosnacek Jan. 31, 2019, 10:20 a.m. UTC | #2
Hi Tejun,

On Wed, Jan 30, 2019 at 6:09 PM Tejun Heo <tj@kernel.org> wrote:
>
> Hello,
>
> On Wed, Jan 30, 2019 at 12:41:50PM +0100, Ondrej Mosnacek wrote:
> > @@ -673,6 +698,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> >                       goto err_out3;
> >       }
> >
> > +     if (parent) {
> > +             ret = kernfs_node_init_security(parent, kn);
> > +             if (ret)
> > +                     goto err_out3;
> > +     }
>
> So, doing this unconditionally isn't a good idea.  kernfs doesn't use
> the usual dentry/inode because there are machines with 6, even 7 digit
> number of kernfs nodes and some of them even failed to boot due to
> memory shortage.  Please don't blow it up by default.

Hm, I see... basically the only thing that gets allocated in
kernfs_node_init_security() by default (at least under SELinux/ no
LSM) is the kernfs_iattrs structures, so I assume you are pointing at
that. I think this can be easily fixed, if we again use the assumption
that whenever the parent node has only default attributes
(parent->iattrs == NULL), then the child node should also have just
default attributes (and so we don't need to call kernfs_iattrs() on it
nor call the security hook). Something along these lines:

[...]
+static int kernfs_node_init_security(struct kernfs_node *parent,
+                                    struct kernfs_node *kn)
+{
+       struct kernfs_iattrs *attrs, *pattrs;
+       struct qstr q;
+
+       pattrs = parent->iattrs;
+       if (!pattrs)
+               return 0;
+
+       attrs = kernfs_iattrs(kn);
+       if (!attrs)
+               return -ENOMEM;
+
+       q.name = kn->name;
+       q.hash_len = hashlen_string(parent, kn->name);
[...]

Technically this might make some LSMs unhappy, if they want to set
some non-default context even if parent is all default, but this is
already impossible now and in this case I think we have no better
choice than sacrificing a bit of flexibility for memory efficiency,
which is apparently critical here.

Tejun, Casey, would the above modification be fine with you?

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
Tejun Heo Jan. 31, 2019, 2:22 p.m. UTC | #3
Hello,

On Thu, Jan 31, 2019 at 11:20:57AM +0100, Ondrej Mosnacek wrote:
> Hm, I see... basically the only thing that gets allocated in
> kernfs_node_init_security() by default (at least under SELinux/ no
> LSM) is the kernfs_iattrs structures, so I assume you are pointing at
> that. I think this can be easily fixed, if we again use the assumption

Yeap.

> Technically this might make some LSMs unhappy, if they want to set
> some non-default context even if parent is all default, but this is
> already impossible now and in this case I think we have no better
> choice than sacrificing a bit of flexibility for memory efficiency,
> which is apparently critical here.
> 
> Tejun, Casey, would the above modification be fine with you?

Generally looks good but maybe it can check the attr to see whether
there actually are things which need inheritance?

Thanks.
Casey Schaufler Jan. 31, 2019, 4:39 p.m. UTC | #4
On 1/31/2019 2:20 AM, Ondrej Mosnacek wrote:
> Hi Tejun,
>
> On Wed, Jan 30, 2019 at 6:09 PM Tejun Heo <tj@kernel.org> wrote:
>> Hello,
>>
>> On Wed, Jan 30, 2019 at 12:41:50PM +0100, Ondrej Mosnacek wrote:
>>> @@ -673,6 +698,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
>>>                       goto err_out3;
>>>       }
>>>
>>> +     if (parent) {
>>> +             ret = kernfs_node_init_security(parent, kn);
>>> +             if (ret)
>>> +                     goto err_out3;
>>> +     }
>> So, doing this unconditionally isn't a good idea.  kernfs doesn't use
>> the usual dentry/inode because there are machines with 6, even 7 digit
>> number of kernfs nodes and some of them even failed to boot due to
>> memory shortage.  Please don't blow it up by default.
> Hm, I see... basically the only thing that gets allocated in
> kernfs_node_init_security() by default (at least under SELinux/ no
> LSM) is the kernfs_iattrs structures, so I assume you are pointing at
> that. I think this can be easily fixed, if we again use the assumption
> that whenever the parent node has only default attributes
> (parent->iattrs == NULL), then the child node should also have just
> default attributes (and so we don't need to call kernfs_iattrs() on it
> nor call the security hook). Something along these lines:
>
> [...]
> +static int kernfs_node_init_security(struct kernfs_node *parent,
> +                                    struct kernfs_node *kn)
> +{
> +       struct kernfs_iattrs *attrs, *pattrs;
> +       struct qstr q;
> +
> +       pattrs = parent->iattrs;
> +       if (!pattrs)
> +               return 0;
> +
> +       attrs = kernfs_iattrs(kn);
> +       if (!attrs)
> +               return -ENOMEM;
> +
> +       q.name = kn->name;
> +       q.hash_len = hashlen_string(parent, kn->name);
> [...]
>
> Technically this might make some LSMs unhappy, if they want to set
> some non-default context even if parent is all default,

The only possibility I see as a potential problem is a kernfs
mounted with the smackfstransmute=Something option. This sets
the security.SMACK64 to "Something" and the security.SMACK64TRANSMUTE
to true on the root node. But that doesn't seem like a rational
thing to do for a kernfs based filesystem.

> but this is
> already impossible now and in this case I think we have no better
> choice than sacrificing a bit of flexibility for memory efficiency,
> which is apparently critical here.
>
> Tejun, Casey, would the above modification be fine with you?

I *think so*, but I can't say 100% that I really understand the
entire issue.

>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.
>
Ondrej Mosnacek Feb. 4, 2019, 9:48 a.m. UTC | #5
On Thu, Jan 31, 2019 at 5:39 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/31/2019 2:20 AM, Ondrej Mosnacek wrote:
> > Hi Tejun,
> >
> > On Wed, Jan 30, 2019 at 6:09 PM Tejun Heo <tj@kernel.org> wrote:
> >> Hello,
> >>
> >> On Wed, Jan 30, 2019 at 12:41:50PM +0100, Ondrej Mosnacek wrote:
> >>> @@ -673,6 +698,12 @@ static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
> >>>                       goto err_out3;
> >>>       }
> >>>
> >>> +     if (parent) {
> >>> +             ret = kernfs_node_init_security(parent, kn);
> >>> +             if (ret)
> >>> +                     goto err_out3;
> >>> +     }
> >> So, doing this unconditionally isn't a good idea.  kernfs doesn't use
> >> the usual dentry/inode because there are machines with 6, even 7 digit
> >> number of kernfs nodes and some of them even failed to boot due to
> >> memory shortage.  Please don't blow it up by default.
> > Hm, I see... basically the only thing that gets allocated in
> > kernfs_node_init_security() by default (at least under SELinux/ no
> > LSM) is the kernfs_iattrs structures, so I assume you are pointing at
> > that. I think this can be easily fixed, if we again use the assumption
> > that whenever the parent node has only default attributes
> > (parent->iattrs == NULL), then the child node should also have just
> > default attributes (and so we don't need to call kernfs_iattrs() on it
> > nor call the security hook). Something along these lines:
> >
> > [...]
> > +static int kernfs_node_init_security(struct kernfs_node *parent,
> > +                                    struct kernfs_node *kn)
> > +{
> > +       struct kernfs_iattrs *attrs, *pattrs;
> > +       struct qstr q;
> > +
> > +       pattrs = parent->iattrs;
> > +       if (!pattrs)
> > +               return 0;
> > +
> > +       attrs = kernfs_iattrs(kn);
> > +       if (!attrs)
> > +               return -ENOMEM;
> > +
> > +       q.name = kn->name;
> > +       q.hash_len = hashlen_string(parent, kn->name);
> > [...]
> >
> > Technically this might make some LSMs unhappy, if they want to set
> > some non-default context even if parent is all default,
>
> The only possibility I see as a potential problem is a kernfs
> mounted with the smackfstransmute=Something option. This sets
> the security.SMACK64 to "Something" and the security.SMACK64TRANSMUTE
> to true on the root node. But that doesn't seem like a rational
> thing to do for a kernfs based filesystem.

Actually... I am now experimenting with a slightly different
kernfs_node_init_security() implementation that should allow for
calling the hook every time and only allocating kernfs_iattrs when it
detects that the hook actually did add some security xattrs. It is
somewhat more hacky and complex, but it should provide the best
possible compromise. I will post it for review soon.

>
> > but this is
> > already impossible now and in this case I think we have no better
> > choice than sacrificing a bit of flexibility for memory efficiency,
> > which is apparently critical here.
> >
> > Tejun, Casey, would the above modification be fine with you?
>
> I *think so*, but I can't say 100% that I really understand the
> entire issue.
>
> >
> > --
> > Ondrej Mosnacek <omosnace at redhat dot com>
> > Associate Software Engineer, Security Technologies
> > Red Hat, Inc.
> >

Patch
diff mbox series

diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index ad7e3356bcc5..797199a748d7 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -15,6 +15,7 @@ 
 #include <linux/slab.h>
 #include <linux/security.h>
 #include <linux/hash.h>
+#include <linux/stringhash.h>
 
 #include "kernfs-internal.h"
 
@@ -616,7 +617,31 @@  struct kernfs_node *kernfs_node_from_dentry(struct dentry *dentry)
 	return NULL;
 }
 
+static int kernfs_node_init_security(struct kernfs_node *parent,
+				     struct kernfs_node *kn)
+{
+	struct kernfs_iattrs *attrs, *pattrs;
+	struct qstr q;
+
+	attrs = kernfs_iattrs(kn);
+	if (!attrs)
+		return -ENOMEM;
+
+	pattrs = kernfs_iattrs(parent);
+	if (!pattrs)
+		return -ENOMEM;
+
+	q.name = kn->name;
+	q.hash_len = hashlen_string(parent, kn->name);
+
+	return security_kernfs_init_security(&q, &attrs->ia_iattr,
+					     &attrs->xattrs_security,
+					     &pattrs->ia_iattr,
+					     &pattrs->xattrs_security);
+}
+
 static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
+					     struct kernfs_node *parent,
 					     const char *name, umode_t mode,
 					     kuid_t uid, kgid_t gid,
 					     unsigned flags)
@@ -673,6 +698,12 @@  static struct kernfs_node *__kernfs_new_node(struct kernfs_root *root,
 			goto err_out3;
 	}
 
+	if (parent) {
+		ret = kernfs_node_init_security(parent, kn);
+		if (ret)
+			goto err_out3;
+	}
+
 	return kn;
 
  err_out3:
@@ -691,7 +722,7 @@  struct kernfs_node *kernfs_new_node(struct kernfs_node *parent,
 {
 	struct kernfs_node *kn;
 
-	kn = __kernfs_new_node(kernfs_root(parent),
+	kn = __kernfs_new_node(kernfs_root(parent), parent,
 			       name, mode, uid, gid, flags);
 	if (kn) {
 		kernfs_get(parent);
@@ -961,7 +992,7 @@  struct kernfs_root *kernfs_create_root(struct kernfs_syscall_ops *scops,
 	INIT_LIST_HEAD(&root->supers);
 	root->next_generation = 1;
 
-	kn = __kernfs_new_node(root, "", S_IFDIR | S_IRUGO | S_IXUGO,
+	kn = __kernfs_new_node(root, NULL, "", S_IFDIR | S_IRUGO | S_IXUGO,
 			       GLOBAL_ROOT_UID, GLOBAL_ROOT_GID,
 			       KERNFS_DIR);
 	if (!kn) {
diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
index f0e2cb4379c0..645c404b8644 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;
diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
index 93bf1dcd0306..90215f8e503a 100644
--- a/fs/kernfs/kernfs-internal.h
+++ b/fs/kernfs/kernfs-internal.h
@@ -90,6 +90,7 @@  int kernfs_iop_getattr(const struct path *path, struct kstat *stat,
 		       u32 request_mask, unsigned int query_flags);
 ssize_t kernfs_iop_listxattr(struct dentry *dentry, char *buf, size_t size);
 int __kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr);
+struct kernfs_iattrs *kernfs_iattrs(struct kernfs_node *kn);
 
 /*
  * dir.c