[v10,2/4] security: Allow all LSMs to provide xattrs for inode_init_security hook

  From: Roberto Sassu <roberto.sassu@huawei.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@clip-os.org> (EVM crash)
Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.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(-)
  

On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.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@clip-os.org> (EVM crash)
> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.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?

>  #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
  

On 4/4/2023 11:54 AM, Paul Moore wrote:
> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
>> From: Roberto Sassu <roberto.sassu@huawei.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@clip-os.org> (EVM crash)
>> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.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
  

On 4/5/2023 4:08 AM, Casey Schaufler wrote:
> On 4/4/2023 11:54 AM, Paul Moore wrote:
>> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
>> <roberto.sassu@huaweicloud.com> wrote:
>>> From: Roberto Sassu <roberto.sassu@huawei.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@clip-os.org> (EVM crash)
>>> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.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.

If you like to set an additional xattr, that would be possible now. 
Since we reserve multiple xattrs, we can call lsm_get_xattr_slot() 
another time and set SMACK64_TRANSMUTE.

I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
EVM would protect SMACK64_TRANSMUTE too.

Roberto

>>>   #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
  

On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On 4/5/2023 4:08 AM, Casey Schaufler wrote:
> > On 4/4/2023 11:54 AM, Paul Moore wrote:
> >> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
> >> <roberto.sassu@huaweicloud.com> wrote:

...

> >>> 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.
>
> If you like to set an additional xattr, that would be possible now.
> Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
> another time and set SMACK64_TRANSMUTE.
>
> I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
> EVM would protect SMACK64_TRANSMUTE too.

Ooookay, but can someone explain to me how either the current, or
patched, smack_inode_init_security() function can return multiple
xattrs via the security_inode_init_security() LSM hook?  I'm hoping
I'm missing something really obvious, but I can only see a single
Smack xattr being returned ...
  

On 4/5/2023 12:59 PM, Paul Moore wrote:
> On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
>> On 4/5/2023 4:08 AM, Casey Schaufler wrote:
>>> On 4/4/2023 11:54 AM, Paul Moore wrote:
>>>> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
>>>> <roberto.sassu@huaweicloud.com> wrote:
> ..
>
>>>>> 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.
>> If you like to set an additional xattr, that would be possible now.
>> Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
>> another time and set SMACK64_TRANSMUTE.
>>
>> I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
>> EVM would protect SMACK64_TRANSMUTE too.
> Ooookay, but can someone explain to me how either the current, or
> patched, smack_inode_init_security() function can return multiple
> xattrs via the security_inode_init_security() LSM hook?

It can't.

>   I'm hoping
> I'm missing something really obvious, but I can only see a single
> Smack xattr being returned ...

Smack is setting the transmute attribute in smack_d_instantiate().
The exec and mmap attributes are always set explicitly.

I don't know how the "extra" Smack attributes were obtained by evm
before, and I haven't been looking at how they're doing it now.
I have assumed that CONFIG_EVM_EXTRA_SMACK_XATTRS does something
meaningful.
  

On Wed, Apr 5, 2023 at 4:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/5/2023 12:59 PM, Paul Moore wrote:
> > On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> >> On 4/5/2023 4:08 AM, Casey Schaufler wrote:
> >>> On 4/4/2023 11:54 AM, Paul Moore wrote:
> >>>> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
> >>>> <roberto.sassu@huaweicloud.com> wrote:
> > ..
> >
> >>>>> 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.
> >> If you like to set an additional xattr, that would be possible now.
> >> Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
> >> another time and set SMACK64_TRANSMUTE.
> >>
> >> I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
> >> EVM would protect SMACK64_TRANSMUTE too.
> >
> > Ooookay, but can someone explain to me how either the current, or
> > patched, smack_inode_init_security() function can return multiple
> > xattrs via the security_inode_init_security() LSM hook?
>
> It can't.

I didn't think so.

To be really specific, that's what we're talking about with this
patch: the number of xattrs that smack_inode_init_security() can
return to the LSM hook (and EVM, and the caller ...).  If it's only
ever going to be one, I think we can adjust the
'SMACK_INODE_INIT_XATTRS' down to '1' and save ourselves some
allocation space.
  

On 4/5/2023 1:49 PM, Paul Moore wrote:
> On Wed, Apr 5, 2023 at 4:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/5/2023 12:59 PM, Paul Moore wrote:
>>> On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
>>> <roberto.sassu@huaweicloud.com> wrote:
>>>> On 4/5/2023 4:08 AM, Casey Schaufler wrote:
>>>>> On 4/4/2023 11:54 AM, Paul Moore wrote:
>>>>>> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
>>>>>> <roberto.sassu@huaweicloud.com> wrote:
>>> ..
>>>
>>>>>>> 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.
>>>> If you like to set an additional xattr, that would be possible now.
>>>> Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
>>>> another time and set SMACK64_TRANSMUTE.
>>>>
>>>> I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
>>>> EVM would protect SMACK64_TRANSMUTE too.
>>> Ooookay, but can someone explain to me how either the current, or
>>> patched, smack_inode_init_security() function can return multiple
>>> xattrs via the security_inode_init_security() LSM hook?
>> It can't.
> I didn't think so.
>
> To be really specific, that's what we're talking about with this
> patch: the number of xattrs that smack_inode_init_security() can
> return to the LSM hook (and EVM, and the caller ...).  If it's only
> ever going to be one, I think we can adjust the
> 'SMACK_INODE_INIT_XATTRS' down to '1' and save ourselves some
> allocation space.

Does evm have an expectation that mumble_inode_init_security() is
going to report all the relevant attributes? It has to be getting
them somehow, which leads me to wonder if we might want to extend
smack_inode_init_security() to do so. Even if we did, the maximum
value would be '2', SMACK64 and SMACK64_TRANSMUTE. Now that would
require a whole lot of work in the calling filesystems, as setting
the transmute attribute would be moving out of smack_d_instantiate()
and into the callers. Or something like that.
  

On 4/5/2023 10:43 PM, Casey Schaufler wrote:
> On 4/5/2023 12:59 PM, Paul Moore wrote:
>> On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
>> <roberto.sassu@huaweicloud.com> wrote:
>>> On 4/5/2023 4:08 AM, Casey Schaufler wrote:
>>>> On 4/4/2023 11:54 AM, Paul Moore wrote:
>>>>> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
>>>>> <roberto.sassu@huaweicloud.com> wrote:
>> ..
>>
>>>>>> 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.
>>> If you like to set an additional xattr, that would be possible now.
>>> Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
>>> another time and set SMACK64_TRANSMUTE.
>>>
>>> I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
>>> EVM would protect SMACK64_TRANSMUTE too.
>> Ooookay, but can someone explain to me how either the current, or
>> patched, smack_inode_init_security() function can return multiple
>> xattrs via the security_inode_init_security() LSM hook?
> 
> It can't.
> 
>>    I'm hoping
>> I'm missing something really obvious, but I can only see a single
>> Smack xattr being returned ...
> 
> Smack is setting the transmute attribute in smack_d_instantiate().
> The exec and mmap attributes are always set explicitly.
> 
> I don't know how the "extra" Smack attributes were obtained by evm
> before, and I haven't been looking at how they're doing it now.
> I have assumed that CONFIG_EVM_EXTRA_SMACK_XATTRS does something
> meaningful.

EVM has a list of xattrs to protect. Without 
CONFIG_EVM_EXTRA_SMACK_XATTRS, EVM protects only security.SMACK64. With 
it, also the other Smack xattrs.

EVM calculates the HMAC of xattrs from that list at inode creation time 
(during the execution of security_inode_init_security(), after other 
LSMs) and during set/remove xattrs operations on the VFS.

Currently, Smack provides only security.SMACK64, so I agree with Paul 
that we should reserve as many xattr as we use. If in the future, we add 
security.SMACK_TRANSMUTE64, we can increase the number of reserved 
xattrs to 2.

Roberto
  

On 4/5/2023 11:07 PM, Casey Schaufler wrote:
> On 4/5/2023 1:49 PM, Paul Moore wrote:
>> On Wed, Apr 5, 2023 at 4:43 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 4/5/2023 12:59 PM, Paul Moore wrote:
>>>> On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
>>>> <roberto.sassu@huaweicloud.com> wrote:
>>>>> On 4/5/2023 4:08 AM, Casey Schaufler wrote:
>>>>>> On 4/4/2023 11:54 AM, Paul Moore wrote:
>>>>>>> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
>>>>>>> <roberto.sassu@huaweicloud.com> wrote:
>>>> ..
>>>>
>>>>>>>> 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.
>>>>> If you like to set an additional xattr, that would be possible now.
>>>>> Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
>>>>> another time and set SMACK64_TRANSMUTE.
>>>>>
>>>>> I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
>>>>> EVM would protect SMACK64_TRANSMUTE too.
>>>> Ooookay, but can someone explain to me how either the current, or
>>>> patched, smack_inode_init_security() function can return multiple
>>>> xattrs via the security_inode_init_security() LSM hook?
>>> It can't.
>> I didn't think so.
>>
>> To be really specific, that's what we're talking about with this
>> patch: the number of xattrs that smack_inode_init_security() can
>> return to the LSM hook (and EVM, and the caller ...).  If it's only
>> ever going to be one, I think we can adjust the
>> 'SMACK_INODE_INIT_XATTRS' down to '1' and save ourselves some
>> allocation space.
> 
> Does evm have an expectation that mumble_inode_init_security() is
> going to report all the relevant attributes? It has to be getting
> them somehow, which leads me to wonder if we might want to extend
> smack_inode_init_security() to do so. Even if we did, the maximum
> value would be '2', SMACK64 and SMACK64_TRANSMUTE. Now that would
> require a whole lot of work in the calling filesystems, as setting
> the transmute attribute would be moving out of smack_d_instantiate()
> and into the callers. Or something like that.

After changing the inode_init_security hook definition to pass the full 
xattr array, this is not going to be a problem. EVM sees all xattrs that 
are going to be set when an inode is created, and adds its own too.

If you have enough information to set security.SMACK_TRANSMUTE64 in 
smack_inode_init_security(), this patch sets already allows to set both 
xattrs at the same time. We would just need to call lsm_get_xattr_slot() 
another time, assuming that we reserve two xattrs.

Roberto
  

On 4/6/2023 2:14 AM, Roberto Sassu wrote:
> On 4/5/2023 11:07 PM, Casey Schaufler wrote:
>> On 4/5/2023 1:49 PM, Paul Moore wrote:
>>> On Wed, Apr 5, 2023 at 4:43 PM Casey Schaufler
>>> <casey@schaufler-ca.com> wrote:
>>>> On 4/5/2023 12:59 PM, Paul Moore wrote:
>>>>> On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
>>>>> <roberto.sassu@huaweicloud.com> wrote:
>>>>>> On 4/5/2023 4:08 AM, Casey Schaufler wrote:
>>>>>>> On 4/4/2023 11:54 AM, Paul Moore wrote:
>>>>>>>> On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
>>>>>>>> <roberto.sassu@huaweicloud.com> wrote:
>>>>> ..
>>>>>
>>>>>>>>> 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.
>>>>>> If you like to set an additional xattr, that would be possible now.
>>>>>> Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
>>>>>> another time and set SMACK64_TRANSMUTE.
>>>>>>
>>>>>> I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
>>>>>> EVM would protect SMACK64_TRANSMUTE too.
>>>>> Ooookay, but can someone explain to me how either the current, or
>>>>> patched, smack_inode_init_security() function can return multiple
>>>>> xattrs via the security_inode_init_security() LSM hook?
>>>> It can't.
>>> I didn't think so.
>>>
>>> To be really specific, that's what we're talking about with this
>>> patch: the number of xattrs that smack_inode_init_security() can
>>> return to the LSM hook (and EVM, and the caller ...).  If it's only
>>> ever going to be one, I think we can adjust the
>>> 'SMACK_INODE_INIT_XATTRS' down to '1' and save ourselves some
>>> allocation space.
>>
>> Does evm have an expectation that mumble_inode_init_security() is
>> going to report all the relevant attributes? It has to be getting
>> them somehow, which leads me to wonder if we might want to extend
>> smack_inode_init_security() to do so. Even if we did, the maximum
>> value would be '2', SMACK64 and SMACK64_TRANSMUTE. Now that would
>> require a whole lot of work in the calling filesystems, as setting
>> the transmute attribute would be moving out of smack_d_instantiate()
>> and into the callers. Or something like that.
>
> After changing the inode_init_security hook definition to pass the
> full xattr array, this is not going to be a problem. EVM sees all
> xattrs that are going to be set when an inode is created, and adds its
> own too.
>
> If you have enough information to set security.SMACK_TRANSMUTE64 in
> smack_inode_init_security(),

I think there's enough information to do that. I'm going to have to look
at your patch more closely.

> this patch sets already allows to set both xattrs at the same time. We
> would just need to call lsm_get_xattr_slot() another time, assuming
> that we reserve two xattrs.
>
> Roberto
>
  

Hi Roberto,

Sorry for the delay in responding...

The patch description reads as though support for per LSM multiple
xattrs is being added in this patch, though lsm_get_xattr_slot() only
ever is incremented once for each LSM.  To simplify review, it would be
nice to mention that lsm_get_xattr_slot() would be called multiple
times per LSM xattr.

On Fri, 2023-03-31 at 14:32 +0200, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.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.

The number of security xattrs permitted per LSM was discussed in the
second paragraph.  The first line of this paragraph needs to be updated
to reflect the current number of security xattrs used, though that is
more related to the new lsm_get_xattr_slot().  Or perhaps the entire
paragraph is unnecessary, a remnant from
security_check_compact_filled_xattrs(), and should be removed.  

> 
> 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().

This Smack comment should be moved to the previous paragraph and even
expanded explaining that lsm_get_xattr_slot() will be called for each
additional security xattr.

> 
> Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org> (EVM crash)
> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

> 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)++;

At some point, since lsm_get_xattr_slot() could be called multiple
times from the same LSM, shouldn't there be some sort of bounds
checking?
  

On Tue, 2023-04-11 at 03:22 -0400, Mimi Zohar wrote:
> Hi Roberto,
> 
> Sorry for the delay in responding...

Hi Mimi

no worries!

> The patch description reads as though support for per LSM multiple
> xattrs is being added in this patch, though lsm_get_xattr_slot() only
> ever is incremented once for each LSM.  To simplify review, it would be
> nice to mention that lsm_get_xattr_slot() would be called multiple
> times per LSM xattr.

Ok, I will mention it.

> On Fri, 2023-03-31 at 14:32 +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.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.
> 
> The number of security xattrs permitted per LSM was discussed in the
> second paragraph.  The first line of this paragraph needs to be updated
> to reflect the current number of security xattrs used, though that is
> more related to the new lsm_get_xattr_slot().  Or perhaps the entire
> paragraph is unnecessary, a remnant from
> security_check_compact_filled_xattrs(), and should be removed.  

I would probably say in that paragraph that the number specified for
the lbs_xattr_count field determines how many times an LSM can call
lsm_get_xattr_slot().

> > 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().
> 
> This Smack comment should be moved to the previous paragraph and even
> expanded explaining that lsm_get_xattr_slot() will be called for each
> additional security xattr.

From previous Paul's and Casey's comments, Smack will have just two
xattrs, assuming that security.SMACK_TRASMUTE64 can be set in
smack_inode_init_security(). I will change this part accordingly once
Casey can have a look at the function.

> > Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org> (EVM crash)
> > Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > 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)++;
> 
> At some point, since lsm_get_xattr_slot() could be called multiple
> times from the same LSM, shouldn't there be some sort of bounds
> checking?

From previous Paul's comments, I understood that he prefers to avoid
extra checks. It will be up to LSM developers to ensure that the API is
used correctly.

Thanks

Roberto
  

On 4/11/2023 12:53 AM, Roberto Sassu wrote:
> On Tue, 2023-04-11 at 03:22 -0400, Mimi Zohar wrote:
>> Hi Roberto,
>>
>> Sorry for the delay in responding...
> Hi Mimi
>
> no worries!
>
>> The patch description reads as though support for per LSM multiple
>> xattrs is being added in this patch, though lsm_get_xattr_slot() only
>> ever is incremented once for each LSM.  To simplify review, it would be
>> nice to mention that lsm_get_xattr_slot() would be called multiple
>> times per LSM xattr.
> Ok, I will mention it.
>
>> On Fri, 2023-03-31 at 14:32 +0200, Roberto Sassu wrote:
>>> From: Roberto Sassu <roberto.sassu@huawei.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.
>> The number of security xattrs permitted per LSM was discussed in the
>> second paragraph.  The first line of this paragraph needs to be updated
>> to reflect the current number of security xattrs used, though that is
>> more related to the new lsm_get_xattr_slot().  Or perhaps the entire
>> paragraph is unnecessary, a remnant from
>> security_check_compact_filled_xattrs(), and should be removed.  
> I would probably say in that paragraph that the number specified for
> the lbs_xattr_count field determines how many times an LSM can call
> lsm_get_xattr_slot().
>
>>> 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().
>> This Smack comment should be moved to the previous paragraph and even
>> expanded explaining that lsm_get_xattr_slot() will be called for each
>> additional security xattr.
> >From previous Paul's and Casey's comments, Smack will have just two
> xattrs, assuming that security.SMACK_TRASMUTE64 can be set in
> smack_inode_init_security(). I will change this part accordingly once
> Casey can have a look at the function.

To be clear, Smack may use two xattrs from smack_inode_init_security(),
SMACK64 and SMACK64_TRANSMUTE. SMACK64_TRANSMUTE is only set on directories.
SMACK64_MMAP and SMACK64_EXEC can be set on files, but they have to be
set explicitly. A file may have three xattrs, but only one from
smack_inode_init_security().

I'm looking at the existing Smack function, and it includes checking for
the transmute attribute. Your patch seems to have dropped this important
behavior. That needs to be restored in any case. You can tell that you need
to include the SMACK64_TRANSMUTE xattr if setting it is detected.

>
>>> Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org> (EVM crash)
>>> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>> 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)++;
>> At some point, since lsm_get_xattr_slot() could be called multiple
>> times from the same LSM, shouldn't there be some sort of bounds
>> checking?
> >From previous Paul's comments, I understood that he prefers to avoid
> extra checks. It will be up to LSM developers to ensure that the API is
> used correctly.
>
> Thanks
>
> Roberto
>
  

On Tue, 2023-04-11 at 09:42 -0700, Casey Schaufler wrote:
> On 4/11/2023 12:53 AM, Roberto Sassu wrote:
> > On Tue, 2023-04-11 at 03:22 -0400, Mimi Zohar wrote:
> > > Hi Roberto,
> > > 
> > > Sorry for the delay in responding...
> > Hi Mimi
> > 
> > no worries!
> > 
> > > The patch description reads as though support for per LSM multiple
> > > xattrs is being added in this patch, though lsm_get_xattr_slot() only
> > > ever is incremented once for each LSM.  To simplify review, it would be
> > > nice to mention that lsm_get_xattr_slot() would be called multiple
> > > times per LSM xattr.
> > Ok, I will mention it.
> > 
> > > On Fri, 2023-03-31 at 14:32 +0200, Roberto Sassu wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.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.
> > > The number of security xattrs permitted per LSM was discussed in the
> > > second paragraph.  The first line of this paragraph needs to be updated
> > > to reflect the current number of security xattrs used, though that is
> > > more related to the new lsm_get_xattr_slot().  Or perhaps the entire
> > > paragraph is unnecessary, a remnant from
> > > security_check_compact_filled_xattrs(), and should be removed.  
> > I would probably say in that paragraph that the number specified for
> > the lbs_xattr_count field determines how many times an LSM can call
> > lsm_get_xattr_slot().
> > 
> > > > 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().
> > > This Smack comment should be moved to the previous paragraph and even
> > > expanded explaining that lsm_get_xattr_slot() will be called for each
> > > additional security xattr.
> > > From previous Paul's and Casey's comments, Smack will have just two
> > xattrs, assuming that security.SMACK_TRASMUTE64 can be set in
> > smack_inode_init_security(). I will change this part accordingly once
> > Casey can have a look at the function.
> 
> To be clear, Smack may use two xattrs from smack_inode_init_security(),
> SMACK64 and SMACK64_TRANSMUTE. SMACK64_TRANSMUTE is only set on directories.
> SMACK64_MMAP and SMACK64_EXEC can be set on files, but they have to be
> set explicitly. A file may have three xattrs, but only one from
> smack_inode_init_security().
> 
> I'm looking at the existing Smack function, and it includes checking for
> the transmute attribute. Your patch seems to have dropped this important
> behavior. That needs to be restored in any case. You can tell that you need
> to include the SMACK64_TRANSMUTE xattr if setting it is detected.

Uhm, I think it is simply omitted in the patch, not deleted.

I just sent a draft of the modifications required to set
SMACK64_TRANSMUTE in smack_inode_init_security().

Roberto

> > > > Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org> (EVM crash)
> > > > Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > > 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)++;
> > > At some point, since lsm_get_xattr_slot() could be called multiple
> > > times from the same LSM, shouldn't there be some sort of bounds
> > > checking?
> > > From previous Paul's comments, I understood that he prefers to avoid
> > extra checks. It will be up to LSM developers to ensure that the API is
> > used correctly.
> > 
> > Thanks
> > 
> > Roberto
> >
  

On 4/11/2023 10:25 AM, Roberto Sassu wrote:
> On Tue, 2023-04-11 at 09:42 -0700, Casey Schaufler wrote:
>> On 4/11/2023 12:53 AM, Roberto Sassu wrote:
>>> On Tue, 2023-04-11 at 03:22 -0400, Mimi Zohar wrote:
>>>> Hi Roberto,
>>>>
>>>> Sorry for the delay in responding...
>>> Hi Mimi
>>>
>>> no worries!
>>>
>>>> The patch description reads as though support for per LSM multiple
>>>> xattrs is being added in this patch, though lsm_get_xattr_slot() only
>>>> ever is incremented once for each LSM.  To simplify review, it would be
>>>> nice to mention that lsm_get_xattr_slot() would be called multiple
>>>> times per LSM xattr.
>>> Ok, I will mention it.
>>>
>>>> On Fri, 2023-03-31 at 14:32 +0200, Roberto Sassu wrote:
>>>>> From: Roberto Sassu <roberto.sassu@huawei.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.
>>>> The number of security xattrs permitted per LSM was discussed in the
>>>> second paragraph.  The first line of this paragraph needs to be updated
>>>> to reflect the current number of security xattrs used, though that is
>>>> more related to the new lsm_get_xattr_slot().  Or perhaps the entire
>>>> paragraph is unnecessary, a remnant from
>>>> security_check_compact_filled_xattrs(), and should be removed.  
>>> I would probably say in that paragraph that the number specified for
>>> the lbs_xattr_count field determines how many times an LSM can call
>>> lsm_get_xattr_slot().
>>>
>>>>> 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().
>>>> This Smack comment should be moved to the previous paragraph and even
>>>> expanded explaining that lsm_get_xattr_slot() will be called for each
>>>> additional security xattr.
>>>> From previous Paul's and Casey's comments, Smack will have just two
>>> xattrs, assuming that security.SMACK_TRASMUTE64 can be set in
>>> smack_inode_init_security(). I will change this part accordingly once
>>> Casey can have a look at the function.
>> To be clear, Smack may use two xattrs from smack_inode_init_security(),
>> SMACK64 and SMACK64_TRANSMUTE. SMACK64_TRANSMUTE is only set on directories.
>> SMACK64_MMAP and SMACK64_EXEC can be set on files, but they have to be
>> set explicitly. A file may have three xattrs, but only one from
>> smack_inode_init_security().
>>
>> I'm looking at the existing Smack function, and it includes checking for
>> the transmute attribute. Your patch seems to have dropped this important
>> behavior. That needs to be restored in any case. You can tell that you need
>> to include the SMACK64_TRANSMUTE xattr if setting it is detected.
> Uhm, I think it is simply omitted in the patch, not deleted.

Oops. Right you are.

>
> I just sent a draft of the modifications required to set
> SMACK64_TRANSMUTE in smack_inode_init_security().

Yup. I'll comment on that.

>
> Roberto
>
>>>>> Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org> (EVM crash)
>>>>> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>> ---
>>>>> 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)++;
>>>> At some point, since lsm_get_xattr_slot() could be called multiple
>>>> times from the same LSM, shouldn't there be some sort of bounds
>>>> checking?
>>>> From previous Paul's comments, I understood that he prefers to avoid
>>> extra checks. It will be up to LSM developers to ensure that the API is
>>> used correctly.
>>>
>>> Thanks
>>>
>>> Roberto
>>>