| Message ID | 20190130114150.27807-6-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow initializing the kernfs node's secctx based on its parent | expand |
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.
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.
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.
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. >
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. > >
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
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(-)