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

[Casey Schaufler <casey@schaufler-ca.com>]

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

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

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