On Thu, Jan 31, 2019 at 5:39 PM Casey Schaufler [off-list ref] wrote:

On 1/31/2019 2:20 AM, Ondrej Mosnacek wrote:

quoted

Hi Tejun,

On Wed, Jan 30, 2019 at 6:09 PM Tejun Heo [off-list ref] wrote:

quoted

Hello,

On Wed, Jan 30, 2019 at 12:41:50PM +0100, Ondrej Mosnacek wrote:

quoted

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

quoted

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.

quoted

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

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