Message-ID: <5dbb9430-1e26-ec12-26a2-3718c84e33c2@schaufler-ca.com>
Date:   Tue, 4 Apr 2023 19:08:50 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Paul Moore <paul@...l-moore.com>,
        Roberto Sassu <roberto.sassu@...weicloud.com>
Cc:     zohar@...ux.ibm.com, dmitry.kasatkin@...il.com, jmorris@...ei.org,
        serge@...lyn.com, stephen.smalley.work@...il.com,
        eparis@...isplace.org, reiserfs-devel@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        bpf@...r.kernel.org, kpsingh@...nel.org, keescook@...omium.org,
        nicolas.bouchinet@...p-os.org,
        Roberto Sassu <roberto.sassu@...wei.com>,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v10 2/4] security: Allow all LSMs to provide xattrs for
 inode_init_security hook

On 4/4/2023 11:54 AM, Paul Moore wrote:
> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
> <roberto.sassu@...weicloud.com> wrote:
>> From: Roberto Sassu <roberto.sassu@...wei.com>
>>
>> Currently, security_inode_init_security() supports only one LSM providing
>> an xattr and EVM calculating the HMAC on that xattr, plus other inode
>> metadata.
>>
>> Allow all LSMs to provide one or multiple xattrs, by extending the security
>> blob reservation mechanism. Introduce the new lbs_xattr_count field of the
>> lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
>> needs, and the LSM infrastructure knows how many xattr slots it should
>> allocate.
>>
>> Dynamically allocate the new_xattrs array to be populated by LSMs with the
>> inode_init_security hook, and pass it to the latter instead of the
>> name/value/len triple. Unify the !initxattrs and initxattrs case, simply
>> don't allocate the new_xattrs array in the former.
>>
>> Also, pass to the hook the number of xattrs filled by each LSM, so that
>> there are no gaps when the next LSM fills the array. Gaps might occur
>> because an LSM can legitimately request xattrs to the LSM infrastructure,
>> but not fill the reserved slots, if it was not initialized.
>>
>> Update the documentation of security_inode_init_security() to reflect the
>> changes, and fix the description of the xattr name, as it is not allocated
>> anymore.
>>
>> Finally, adapt both SELinux and Smack to use the new definition of the
>> inode_init_security hook, and to fill the reserved slots in the xattr
>> array. Introduce the lsm_get_xattr_slot() helper to retrieve an available
>> slot to fill, and to increment the number of filled slots.
>>
>> Move the xattr->name assignment after the xattr->value one, so that it is
>> done only in case of successful memory allocation. For Smack, also reserve
>> space for the other defined xattrs although they are not set yet in
>> smack_inode_init_security().
>>
>> Reported-by: Nicolas Bouchinet <nicolas.bouchinet@...p-os.org> (EVM crash)
>> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
>> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
>> ---
>>  include/linux/lsm_hook_defs.h |  6 +--
>>  include/linux/lsm_hooks.h     | 20 ++++++++++
>>  security/security.c           | 71 +++++++++++++++++++++++------------
>>  security/selinux/hooks.c      | 17 +++++----
>>  security/smack/smack_lsm.c    | 32 ++++++++++------
>>  5 files changed, 99 insertions(+), 47 deletions(-)
> This looks good aside from a few small things (below).  From what I
> can see, there are only two outstanding issues to answer: the number
> of Smack xattrs, sign-off from Casey for the Smack bits.
>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index 6bb55e61e8e..a1896f90089 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -111,9 +111,9 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
>>          unsigned int obj_type)
>>  LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
>>  LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
>> -LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
>> -        struct inode *dir, const struct qstr *qstr, const char **name,
>> -        void **value, size_t *len)
>> +LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
>> +        struct inode *dir, const struct qstr *qstr, struct xattr *xattrs,
>> +        int *xattr_count)
>>  LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
>>          const struct qstr *name, const struct inode *context_inode)
>>  LSM_HOOK(int, 0, inode_create, struct inode *dir, struct dentry *dentry,
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index c2be66c669a..9eb9b686493 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -28,6 +28,7 @@
>>  #include <linux/security.h>
>>  #include <linux/init.h>
>>  #include <linux/rculist.h>
>> +#include <linux/xattr.h>
>>
>>  union security_list_options {
>>         #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
>> @@ -63,8 +64,27 @@ struct lsm_blob_sizes {
>>         int     lbs_ipc;
>>         int     lbs_msg_msg;
>>         int     lbs_task;
>> +       int     lbs_xattr_count; /* number of xattr slots in new_xattrs array */
>>  };
>>
>> +/**
>> + * lsm_get_xattr_slot - Return the next available slot and increment the index
>> + * @xattrs: array storing LSM-provided xattrs
>> + * @xattr_count: number of already stored xattrs (updated)
>> + *
>> + * Retrieve the first available slot in the @xattrs array to fill with an xattr,
>> + * and increment @xattr_count.
>> + *
>> + * Return: The slot to fill in @xattrs if non-NULL, NULL otherwise.
>> + */
>> +static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
>> +                                              int *xattr_count)
>> +{
>> +       if (unlikely(!xattrs))
>> +               return NULL;
>> +       return xattrs + (*xattr_count)++;
>> +}
>> +
>>  /*
>>   * LSM_RET_VOID is used as the default value in LSM_HOOK definitions for void
>>   * LSM hooks (in include/linux/lsm_hook_defs.h).
>> diff --git a/security/security.c b/security/security.c
>> index f4170efcddd..1aeaa8ce449 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -31,8 +31,6 @@
>>  #include <linux/msg.h>
>>  #include <net/flow.h>
>>
>> -#define MAX_LSM_EVM_XATTR      2
>> -
>>  /* How many LSMs were built into the kernel? */
>>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>>
>> @@ -212,6 +210,8 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>>         lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
>>         lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
>>         lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
>> +       lsm_set_blob_size(&needed->lbs_xattr_count,
>> +                         &blob_sizes.lbs_xattr_count);
>>  }
>>
>>  /* Prepare LSM for initialization. */
>> @@ -378,6 +378,7 @@ static void __init ordered_lsm_init(void)
>>         init_debug("msg_msg blob size    = %d\n", blob_sizes.lbs_msg_msg);
>>         init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
>>         init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
>> +       init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr_count);
>>
>>         /*
>>          * Create any kmem_caches needed for blobs
>> @@ -1591,11 +1592,15 @@ EXPORT_SYMBOL(security_dentry_create_files_as);
>>   * created inode and set up the incore security field for the new inode.  This
>>   * hook is called by the fs code as part of the inode creation transaction and
>>   * provides for atomic labeling of the inode, unlike the post_create/mkdir/...
>> - * hooks called by the VFS.  The hook function is expected to allocate the name
>> - * and value via kmalloc, with the caller being responsible for calling kfree
>> - * after using them.  If the security module does not use security attributes
>> - * or does not wish to put a security attribute on this particular inode, then
>> - * it should return -EOPNOTSUPP to skip this processing.
>> + * hooks called by the VFS.  The hook function is expected to populate the
>> + * @xattrs array, by calling lsm_get_xattr_slot() to retrieve the slots
>> + * reserved by the security module with the lbs_xattr_count field of the
>> + * lsm_blob_sizes structure.  For each slot, the hook function should set ->name
>> + * to the attribute name suffix (e.g. selinux), to allocate ->value (will be
>> + * freed by the caller) and set it to the attribute value, to set ->value_len to
>> + * the length of the value.  If the security module does not use security
>> + * attributes or does not wish to put a security attribute on this particular
>> + * inode, then it should return -EOPNOTSUPP to skip this processing.
>>   *
>>   * Return: Returns 0 on success, -EOPNOTSUPP if no security attribute is
>>   * needed, or -ENOMEM on memory allocation failure.
>> @@ -1604,33 +1609,51 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>>                                  const struct qstr *qstr,
>>                                  const initxattrs initxattrs, void *fs_data)
>>  {
>> -       struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>> -       struct xattr *lsm_xattr, *evm_xattr, *xattr;
>> -       int ret;
>> +       struct security_hook_list *P;
>> +       struct xattr *new_xattrs = NULL;
>> +       int ret = -EOPNOTSUPP, xattr_count = 0;
>>
>>         if (unlikely(IS_PRIVATE(inode)))
>>                 return 0;
>>
>> -       if (!initxattrs)
>> -               return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
>> -                                    dir, qstr, NULL, NULL, NULL);
>> -       memset(new_xattrs, 0, sizeof(new_xattrs));
>> -       lsm_xattr = new_xattrs;
>> -       ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
>> -                           &lsm_xattr->name,
>> -                           &lsm_xattr->value,
>> -                           &lsm_xattr->value_len);
>> -       if (ret)
>> +       if (!blob_sizes.lbs_xattr_count)
>> +               return 0;
>> +
>> +       if (initxattrs) {
>> +               /* Allocate +1 for EVM and +1 as terminator. */
>> +               new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2,
>> +                                    sizeof(*new_xattrs), GFP_NOFS);
>> +               if (!new_xattrs)
>> +                       return -ENOMEM;
>> +       }
>> +
>> +       hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
>> +                            list) {
>> +               ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs,
>> +                                                 &xattr_count);
>> +               if (ret && ret != -EOPNOTSUPP)
>> +                       goto out;
>> +               /*
>> +                * As documented in lsm_hooks.h, -EOPNOTSUPP in this context
>> +                * means that the LSM is not willing to provide an xattr, not
>> +                * that it wants to signal an error. Thus, continue to invoke
>> +                * the remaining LSMs.
>> +                */
>> +       }
>> +
>> +       /* If initxattrs() is NULL, xattr_count is zero, skip the call. */
>> +       if (!xattr_count)
>>                 goto out;
>>
>> -       evm_xattr = lsm_xattr + 1;
>> -       ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
>> +       ret = evm_inode_init_security(inode, new_xattrs,
>> +                                     new_xattrs + xattr_count);
> I think it's cleaner to write '&new_xattrs[xattr_count]' for the third
> parameter above (no concerns around pointer math), and stylistically
> it matches better with the for-kfree loop below.
>
>>         if (ret)
>>                 goto out;
>>         ret = initxattrs(inode, new_xattrs, fs_data);
>>  out:
>> -       for (xattr = new_xattrs; xattr->value != NULL; xattr++)
>> -               kfree(xattr->value);
>> +       for (; xattr_count > 0; xattr_count--)
>> +               kfree(new_xattrs[xattr_count - 1].value);
>> +       kfree(new_xattrs);
>>         return (ret == -EOPNOTSUPP) ? 0 : ret;
>>  }
>>  EXPORT_SYMBOL(security_inode_init_security);
> ..
>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index cfcbb748da2..8392983334b 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -52,6 +52,15 @@
>>  #define SMK_RECEIVING  1
>>  #define SMK_SENDING    2
>>
>> +/*
>> + * Smack uses multiple xattrs.
>> + * SMACK64 - for access control, SMACK64EXEC - label for the program,
> I think it would be good to move SMACK64EXEC to its own line; it took
> me a minute to figure out why SMACK_INODE_INIT_XATTRS was set to '4'
> when I only say three comment lines ... ;)
>
>> + * SMACK64MMAP - controls library loading,
>> + * SMACK64TRANSMUTE - label initialization,
>> + * Not saved on files - SMACK64IPIN and SMACK64IPOUT
>> + */
>> +#define SMACK_INODE_INIT_XATTRS 4
> If smack_inode_init_security() only ever populates a single xattr, and
> that is the only current user of SMACK_INODE_INIT_XATTRS, can we make
> this '1' and shrink the xattr allocation a bit?

If the parent directory is marked with SMACK64_TRANSMUTE, the access
rule allowing the access has the "t" mode, and the object being initialized
is a directory, the new inode should get the SMACK64_TRANSMUTE attribute.
The callers of security_inode_init_security() don't seem to care.
I can't say if the evm code is getting SMACK64_TRANSMUTE or, for that
matter, SMACK64_EXEC and SMACK64_MMAP, some other way. The older system
allowed for multiple Smack xattrs, but I'm not clear on exactly how.


>>  #ifdef SMACK_IPV6_PORT_LABELING
>>  static DEFINE_MUTEX(smack_ipv6_lock);
>>  static LIST_HEAD(smk_ipv6_port_list);
>> @@ -939,26 +948,23 @@ static int smack_inode_alloc_security(struct inode *inode)
>>   * @inode: the newly created inode
>>   * @dir: containing directory object
>>   * @qstr: unused
>> - * @name: where to put the attribute name
>> - * @value: where to put the attribute value
>> - * @len: where to put the length of the attribute
>> + * @xattrs: where to put the attributes
>> + * @xattr_count: current number of LSM-provided xattrs (updated)
>>   *
>>   * Returns 0 if it all works out, -ENOMEM if there's no memory
>>   */
>>  static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>> -                                    const struct qstr *qstr, const char **name,
>> -                                    void **value, size_t *len)
>> +                                    const struct qstr *qstr,
>> +                                    struct xattr *xattrs, int *xattr_count)
>>  {
>>         struct inode_smack *issp = smack_inode(inode);
>>         struct smack_known *skp = smk_of_current();
>>         struct smack_known *isp = smk_of_inode(inode);
>>         struct smack_known *dsp = smk_of_inode(dir);
>> +       struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count);
>>         int may;
>>
>> -       if (name)
>> -               *name = XATTR_SMACK_SUFFIX;
>> -
>> -       if (value && len) {
>> +       if (xattr) {
>>                 rcu_read_lock();
>>                 may = smk_access_entry(skp->smk_known, dsp->smk_known,
>>                                        &skp->smk_rules);
>> @@ -976,11 +982,12 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
>>                         issp->smk_flags |= SMK_INODE_CHANGED;
>>                 }
>>
>> -               *value = kstrdup(isp->smk_known, GFP_NOFS);
>> -               if (*value == NULL)
>> +               xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
>> +               if (xattr->value == NULL)
>>                         return -ENOMEM;
>>
>> -               *len = strlen(isp->smk_known);
>> +               xattr->value_len = strlen(isp->smk_known);
>> +               xattr->name = XATTR_SMACK_SUFFIX;
>>         }
>>
>>         return 0;
>> @@ -4854,6 +4861,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
>>         .lbs_ipc = sizeof(struct smack_known *),
>>         .lbs_msg_msg = sizeof(struct smack_known *),
>>         .lbs_superblock = sizeof(struct superblock_smack),
>> +       .lbs_xattr_count = SMACK_INODE_INIT_XATTRS,
>>  };
>>
>>  static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>> --
>> 2.25.1

Powered by blists - more mailing lists