thr3ads.net - Ocfs2 devel - [Ocfs2-devel] [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init

If this information is useful, please help other people find it:
Share via:

Roberto Sassu

2023-Mar-24 13:25 UTC

[Ocfs2-devel] [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init_security hook

On Fri, 2023-03-24 at 11:18 +0100, Roberto Sassu wrote:> On Thu, 2023-03-23 at 20:09 -0400, Paul Moore wrote:
> > On Tue, Mar 14, 2023 at 4:19?AM Roberto Sassu
> > <roberto.sassu at huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu at 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 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 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. Update the documentation accordingly, and
fix the
> > > description of the xattr name, as it is not allocated anymore.
> > > 
> > > Since the LSM infrastructure, at initialization time, updates the
number of
> > > the requested xattrs provided by each LSM with a corresponding
offset in
> > > the security blob (in this case the xattr array), it makes
straightforward
> > > for an LSM to access the right position in the xattr array.
> > > 
> > > There is still the issue that an LSM might not fill the xattr,
even if it
> > > requests it (legitimate case, for example it might have been
loaded but not
> > > initialized with a policy). Since users of the xattr array (e.g.
the
> > > initxattrs() callbacks) detect the end of the xattr array by
checking if
> > > the xattr name is NULL, not filling an xattr would cause those
users to
> > > stop scanning xattrs prematurely.
> > > 
> > > Solve that issue by introducing
security_check_compact_filled_xattrs(),
> > > which does a basic check of the xattr array (if the xattr name is
filled,
> > > the xattr value should be too, and viceversa), and compacts the
xattr array
> > > by removing the holes.
> > > 
> > > An alternative solution would be to let users of the xattr array
know the
> > > number of elements of that array, so that they don't have to
check the
> > > termination. However, this seems more invasive, compared to a
simple move
> > > of few array elements.
> > > 
> > > security_check_compact_filled_xattrs() also determines how many
xattrs in
> > > the xattr array have been filled. If there is none, skip
> > > evm_inode_init_security() and initxattrs(). Skipping the former
also avoids
> > > EVM to crash the kernel, as it is expecting a filled xattr.
> > > 
> > > Finally, adapt both SELinux and Smack to use the new definition
of the
> > > inode_init_security hook, and to correctly fill the designated
slots in the
> > > xattr array. For Smack, reserve space for the other defined
xattrs although
> > > they are not set yet in smack_inode_init_security().
> > > 
> > > Reported-by: Nicolas Bouchinet <nicolas.bouchinet at
clip-os.org> (EVM crash)
> > > Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS at
archlinux/
> > > Signed-off-by: Roberto Sassu <roberto.sassu at huawei.com>
> > > Reviewed-by: Casey Schaufler <casey at schaufler-ca.com>
> > > Reviewed-by: Mimi Zohar <zohar at linux.ibm.com>
> > > ---
> > >  include/linux/lsm_hook_defs.h |   3 +-
> > >  include/linux/lsm_hooks.h     |   1 +
> > >  security/security.c           | 119
+++++++++++++++++++++++++++++-----
> > >  security/selinux/hooks.c      |  19 ++++--
> > >  security/smack/smack_lsm.c    |  33 ++++++----
> > >  5 files changed, 137 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/include/linux/lsm_hook_defs.h
b/include/linux/lsm_hook_defs.h
> > > index 6bb55e61e8e..b814955ae70 100644
> > > --- a/include/linux/lsm_hook_defs.h
> > > +++ b/include/linux/lsm_hook_defs.h
> > > @@ -112,8 +112,7 @@ LSM_HOOK(int, 0, path_notify, const struct
path *path, u64 mask,
> > >  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)
> > > +        struct inode *dir, const struct qstr *qstr, struct xattr
*xattrs)
> > >  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..75a2f85b49d 100644
> > > --- a/include/linux/lsm_hooks.h
> > > +++ b/include/linux/lsm_hooks.h
> > > @@ -63,6 +63,7 @@ struct lsm_blob_sizes {
> > >         int     lbs_ipc;
> > >         int     lbs_msg_msg;
> > >         int     lbs_task;
> > > +       int     lbs_xattr; /* number of xattr slots in new_xattrs
array */
> > 
> > No need for the comment, we don't do it for the other fields.
> > 
> > >  };
> > > 
> > >  /*
> > > diff --git a/security/security.c b/security/security.c
> > > index f4170efcddd..f1f5f62f7fa 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -1579,6 +1579,52 @@ int security_dentry_create_files_as(struct
dentry *dentry, int mode,
> > >  }
> > >  EXPORT_SYMBOL(security_dentry_create_files_as);
> > > 
> > > +/**
> > > + * security_check_compact_filled_xattrs - check xattrs and make
array contiguous
> > > + * @xattrs: xattr array filled by LSMs
> > > + * @num_xattrs: length of xattr array
> > > + * @num_filled_xattrs: number of already processed xattrs
> > > + *
> > > + * Ensure that each xattr slot is correctly filled and close the
gaps in the
> > > + * xattr array if an LSM didn't provide an xattr for which
it asked space
> > > + * (legitimate case, it might have been loaded but not
initialized). An LSM
> > > + * might request space in the xattr array for one or multiple
xattrs. The LSM
> > > + * infrastructure ensures that all requests by LSMs are
satisfied.
> > > + *
> > > + * Track the number of filled xattrs in @num_filled_xattrs, so
that it is easy
> > > + * to determine whether the currently processed xattr is fine in
its position
> > > + * (if all previous xattrs were filled) or it should be moved
after the last
> > > + * filled xattr.
> > > + *
> > > + * Return: zero if all xattrs are valid, -EINVAL otherwise.
> > > + */
> > > +static int security_check_compact_filled_xattrs(struct xattr
*xattrs,
> > > +                                               int num_xattrs,
> > > +                                               int
*num_filled_xattrs)
> > 
> > That is one long name :)
> > 
> > Since you're making some other changes to this patch, can you
rename
> > this to security_xattr_compact() or something like that?
> 
> Yes, definitely!
> 
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = *num_filled_xattrs; i < num_xattrs; i++) {
> > > +               if ((!xattrs[i].name && xattrs[i].value)
||
> > > +                   (xattrs[i].name && !xattrs[i].value))
> > > +                       return -EINVAL;
> > > +
> > > +               if (!xattrs[i].name)
> > > +                       continue;
> > > +
> > > +               if (i == *num_filled_xattrs) {
> > > +                       (*num_filled_xattrs)++;
> > > +                       continue;
> > > +               }
> > > +
> > > +               memcpy(xattrs + (*num_filled_xattrs)++, xattrs +
i,
> > > +                      sizeof(*xattrs));
> > > +               memset(xattrs + i, 0, sizeof(*xattrs));
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  /**
> > >   * security_inode_init_security() - Initialize an inode's
LSM context
> > >   * @inode: the inode
> > > @@ -1591,9 +1637,13 @@
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
> > > + * hooks called by the VFS.  The hook function is expected to
populate the
> > > + * @xattrs array, depending on how many xattrs have been
specified by the
> > > + * security module in the lbs_xattr field of the lsm_blob_sizes
structure.  For
> > > + * each array element, the hook function is expected to 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.
> > >   *
> > > @@ -1604,33 +1654,66 @@ 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;
> > > +       struct xattr *xattr;
> > > +       int ret = -EOPNOTSUPP, num_filled_xattrs = 0;
> > > 
> > >         if (unlikely(IS_PRIVATE(inode)))
> > >                 return 0;
> > > 
> > > +       if (!blob_sizes.lbs_xattr)
> > > +               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)
> > > +                                   dir, qstr, NULL);
> > > +       /* Allocate +1 for EVM and +1 as terminator. */
> > > +       new_xattrs = kcalloc(blob_sizes.lbs_xattr + 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);
> > > +               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 (ret == -EOPNOTSUPP)
> > > +                       continue;
> > > +               /*
> > > +                * As the number of xattrs reserved by LSMs is
not directly
> > > +                * available, directly use the total number
blob_sizes.lbs_xattr
> > > +                * to keep the code simple, while being not the
most efficient
> > > +                * way.
> > > +                */
> > 
> > Is there a good reason why the LSM can't return the number of
xattrs
> > it is adding to the xattr array?  It seems like it should be fairly
> > trivial for the individual LSMs to determine and it could save a lot
> > of work.  However, given we're at v8 on this patchset I'm sure
I'm
> > missing something obvious, can you help me understand why the idea
> > above is crazy stupid? ;)
Much simple answer. Yes, LSMs could return the number of xattrs set,
but security_check_compact_filled_xattrs() also needs to know from
which offset (the lbs_xattr of each LSM) it should start compacting.

Example: suppose that you have three LSMs with:

LSM#1: lbs_xattr 1
LSM#2: lbs_xattr 2 (disabled)
LSM#3: lbs_xattr 1

The current compaction interval is: already compacted xattrs - end of
new_xattr array.

When the security_inode_init_security() loop calls LSM#3, the
compaction interval is: 1 - 2 (LSM#2 returns 0), which clearly isn't
right. The correct compaction interval should be: 3 - 4.

Going to the end of new_xattrs is an approximation, but it ensures
that security_check_compact_filled_xattrs() reaches the xattr set by
LSM#3.

The alternative I was mentioning of passing num_filled_xattrs to LSMs
goes again in the direction of doing on-the-fly compaction, while LSMs
are more familiar with using the lbs_* fields.

I suggest to keep this part as it is, if you agree.

Thanks

Roberto
> Ok, I looked back at what I did for v3.
> 
> Moving from v3 to v4, I decided to put less burden on LSMs, and to make
> all the processing from the LSM infrastructure side.
> 
> v3 had some safeguards to prevent some programming mistakes by LSMs,
> which maybe made the code less understandable.
> 
> However, if we say we keep things as simple as possible and assume that
> LSMs implement this correctly, we can just pass num_filled_xattrs to
> them and they simply increment it.
> 
> The EVM bug should not arise (accessing xattr->name = NULL), even if
> BPF LSM alone returns zero, due to the check of num_filled_xattrs
> before calling evm_inode_init_security().
> 
> Patch 6 (at the end) will prevent the bug from arising when EVM is
> moved to the LSM infrastructure (no num_filled_xattrs check anymore).
> There is a loop that stops if xattr->name is NULL, so
> evm_protected_xattr() will not be called.
> 
> Or, like you suggested, we just return a positive value from LSMs and
> we keep num_filled_xattrs in security_inode_init_security().
> 
> Ok, I'll go for your proposal.
> 
> > > +               ret =
security_check_compact_filled_xattrs(new_xattrs,
> > > +                                                         
blob_sizes.lbs_xattr,
> > > +                                                         
&num_filled_xattrs);
> > > +               if (ret < 0) {
> > > +                       ret = -ENOMEM;
> > > +                       goto out;
> > > +               }
> > > +       }
> > > +
> > > +       if (!num_filled_xattrs)
> > >                 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 +
num_filled_xattrs);
> > >         if (ret)
> > >                 goto out;
> > >         ret = initxattrs(inode, new_xattrs, fs_data);
> > >  out:
> > >         for (xattr = new_xattrs; xattr->value != NULL;
xattr++)
> > >                 kfree(xattr->value);
> > > +       kfree(new_xattrs);
> > >         return (ret == -EOPNOTSUPP) ? 0 : ret;
> > >  }
> > >  EXPORT_SYMBOL(security_inode_init_security);
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 9a5bdfc2131..3e4308dd336 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -104,6 +104,8 @@
> > >  #include "audit.h"
> > >  #include "avc_ss.h"
> > > 
> > > +#define SELINUX_INODE_INIT_XATTRS 1
> > > +
> > >  struct selinux_state selinux_state;
> > > 
> > >  /* SECMARK reference count */
> > > @@ -2868,11 +2870,11 @@ static int
selinux_dentry_create_files_as(struct dentry *dentry, int mode,
> > > 
> > >  static int selinux_inode_init_security(struct inode *inode,
struct inode *dir,
> > >                                        const struct qstr *qstr,
> > > -                                      const char **name,
> > > -                                      void **value, size_t *len)
> > > +                                      struct xattr *xattrs)
> > >  {
> > >         const struct task_security_struct *tsec =
selinux_cred(current_cred());
> > >         struct superblock_security_struct *sbsec;
> > > +       struct xattr *xattr = NULL;
> > >         u32 newsid, clen;
> > >         int rc;
> > >         char *context;
> > > @@ -2899,16 +2901,18 @@ static int
selinux_inode_init_security(struct inode *inode, struct inode *dir,
> > >             !(sbsec->flags & SBLABEL_MNT))
> > >                 return -EOPNOTSUPP;
> > > 
> > > -       if (name)
> > > -               *name = XATTR_SELINUX_SUFFIX;
> > > +       if (xattrs)
> > > +               xattr = xattrs + selinux_blob_sizes.lbs_xattr;
> > 
> > Please abstract that away to an inline function similar to
> > selinux_cred(), selinux_file(), selinux_inode(), etc.
> 
> Ok.
> 
> > > +       if (xattr) {
> > > +               xattr->name = XATTR_SELINUX_SUFFIX;
> > 
> > I'm guessing the xattr->name assignment is always done,
regardless of
> > if security_sid_to_context_force() is successful, due to the -EINVAL
> > check in security_check_compact_filled_xattrs()?  If yes, it would be
> > good to make note of that here in the code.  If not, it would be nice
> > to move this down the function to go with the other xattr->XXX
> > assignments, unless there is another reason for its placement that
I'm
> > missing.
> 
> Uhm, if an LSM returns an error, security_inode_init_security() stops
> and does the cleanup. It should not matter if xattr->name was set.
> 
> Thanks
> 
> Roberto
> 
> > > -       if (value && len) {
> > >                 rc =
security_sid_to_context_force(&selinux_state, newsid,
> > >                                                    &context,
&clen);
> > >                 if (rc)
> > >                         return rc;
> > > -               *value = context;
> > > -               *len = clen;
> > > +               xattr->value = context;
> > > +               xattr->value_len = clen;
> > >         }
> > > 
> > >         return 0;
> > > @@ -6918,6 +6922,7 @@ struct lsm_blob_sizes selinux_blob_sizes
__lsm_ro_after_init = {
> > >         .lbs_ipc = sizeof(struct ipc_security_struct),
> > >         .lbs_msg_msg = sizeof(struct msg_security_struct),
> > >         .lbs_superblock = sizeof(struct
superblock_security_struct),
> > > +       .lbs_xattr = SELINUX_INODE_INIT_XATTRS,
> > >  };
> > > 
> > >  #ifdef CONFIG_PERF_EVENTS
> > > diff --git a/security/smack/smack_lsm.c
b/security/smack/smack_lsm.c
> > > index cfcbb748da2..c8cf8df268b 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,
> > > + * SMACK64MMAP - controls library loading,
> > > + * SMACK64TRANSMUTE - label initialization,
> > > + * Not saved on files - SMACK64IPIN and SMACK64IPOUT
> > > + */
> > > +#define SMACK_INODE_INIT_XATTRS 4
> > > +
> > >  #ifdef SMACK_IPV6_PORT_LABELING
> > >  static DEFINE_MUTEX(smack_ipv6_lock);
> > >  static LIST_HEAD(smk_ipv6_port_list);
> > > @@ -939,26 +948,27 @@ 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
> > >   *
> > >   * 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)
> > >  {
> > >         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 = NULL;
> > >         int may;
> > > 
> > > -       if (name)
> > > -               *name = XATTR_SMACK_SUFFIX;
> > > +       if (xattrs)
> > > +               xattr = xattrs + smack_blob_sizes.lbs_xattr;
> > > +
> > > +       if (xattr) {
> > > +               xattr->name = XATTR_SMACK_SUFFIX;
> > > 
> > > -       if (value && len) {
> > >                 rcu_read_lock();
> > >                 may = smk_access_entry(skp->smk_known,
dsp->smk_known,
> > >                                        &skp->smk_rules);
> > > @@ -976,11 +986,11 @@ 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);
> > >         }
> > > 
> > >         return 0;
> > > @@ -4854,6 +4864,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 = SMACK_INODE_INIT_XATTRS,
> > >  };
> > > 
> > >  static struct security_hook_list smack_hooks[]
__lsm_ro_after_init = {
> > > --
> > > 2.25.1

Paul Moore

2023-Mar-24 21:39 UTC

head link

[Ocfs2-devel] [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init_security hook

On Fri, Mar 24, 2023 at 9:26?AM Roberto Sassu
<roberto.sassu at huaweicloud.com> wrote:>
> On Fri, 2023-03-24 at 11:18 +0100, Roberto Sassu wrote:
> > On Thu, 2023-03-23 at 20:09 -0400, Paul Moore wrote:
> > > On Tue, Mar 14, 2023 at 4:19?AM Roberto Sassu
> > > <roberto.sassu at huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu at 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
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 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. Update the documentation accordingly,
and fix the
> > > > description of the xattr name, as it is not allocated
anymore.
> > > >
> > > > Since the LSM infrastructure, at initialization time,
updates the number of
> > > > the requested xattrs provided by each LSM with a
corresponding offset in
> > > > the security blob (in this case the xattr array), it makes
straightforward
> > > > for an LSM to access the right position in the xattr array.
> > > >
> > > > There is still the issue that an LSM might not fill the
xattr, even if it
> > > > requests it (legitimate case, for example it might have been
loaded but not
> > > > initialized with a policy). Since users of the xattr array
(e.g. the
> > > > initxattrs() callbacks) detect the end of the xattr array by
checking if
> > > > the xattr name is NULL, not filling an xattr would cause
those users to
> > > > stop scanning xattrs prematurely.
> > > >
> > > > Solve that issue by introducing
security_check_compact_filled_xattrs(),
> > > > which does a basic check of the xattr array (if the xattr
name is filled,
> > > > the xattr value should be too, and viceversa), and compacts
the xattr array
> > > > by removing the holes.
> > > >
> > > > An alternative solution would be to let users of the xattr
array know the
> > > > number of elements of that array, so that they don't
have to check the
> > > > termination. However, this seems more invasive, compared to
a simple move
> > > > of few array elements.
> > > >
> > > > security_check_compact_filled_xattrs() also determines how
many xattrs in
> > > > the xattr array have been filled. If there is none, skip
> > > > evm_inode_init_security() and initxattrs(). Skipping the
former also avoids
> > > > EVM to crash the kernel, as it is expecting a filled xattr.
> > > >
> > > > Finally, adapt both SELinux and Smack to use the new
definition of the
> > > > inode_init_security hook, and to correctly fill the
designated slots in the
> > > > xattr array. For Smack, reserve space for the other defined
xattrs although
> > > > they are not set yet in smack_inode_init_security().
> > > >
> > > > Reported-by: Nicolas Bouchinet <nicolas.bouchinet at
clip-os.org> (EVM crash)
> > > > Link:
https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS at archlinux/
> > > > Signed-off-by: Roberto Sassu <roberto.sassu at
huawei.com>
> > > > Reviewed-by: Casey Schaufler <casey at
schaufler-ca.com>
> > > > Reviewed-by: Mimi Zohar <zohar at linux.ibm.com>
> > > > ---
> > > >  include/linux/lsm_hook_defs.h |   3 +-
> > > >  include/linux/lsm_hooks.h     |   1 +
> > > >  security/security.c           | 119
+++++++++++++++++++++++++++++-----
> > > >  security/selinux/hooks.c      |  19 ++++--
> > > >  security/smack/smack_lsm.c    |  33 ++++++----
> > > >  5 files changed, 137 insertions(+), 38 deletions(-)
...
> > > > @@ -1604,33 +1654,66 @@ 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;
> > > > +       struct xattr *xattr;
> > > > +       int ret = -EOPNOTSUPP, num_filled_xattrs = 0;
> > > >
> > > >         if (unlikely(IS_PRIVATE(inode)))
> > > >                 return 0;
> > > >
> > > > +       if (!blob_sizes.lbs_xattr)
> > > > +               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)
> > > > +                                   dir, qstr, NULL);
> > > > +       /* Allocate +1 for EVM and +1 as terminator. */
> > > > +       new_xattrs = kcalloc(blob_sizes.lbs_xattr + 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);
> > > > +               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 (ret == -EOPNOTSUPP)
> > > > +                       continue;
> > > > +               /*
> > > > +                * As the number of xattrs reserved by LSMs
is not directly
> > > > +                * available, directly use the total number
blob_sizes.lbs_xattr
> > > > +                * to keep the code simple, while being not
the most efficient
> > > > +                * way.
> > > > +                */
> > >
> > > Is there a good reason why the LSM can't return the number of
xattrs
> > > it is adding to the xattr array?  It seems like it should be
fairly
> > > trivial for the individual LSMs to determine and it could save a
lot
> > > of work.  However, given we're at v8 on this patchset I'm
sure I'm
> > > missing something obvious, can you help me understand why the
idea
> > > above is crazy stupid? ;)
>
> Much simple answer. Yes, LSMs could return the number of xattrs set,
> but security_check_compact_filled_xattrs() also needs to know from
> which offset (the lbs_xattr of each LSM) it should start compacting.
>
> Example: suppose that you have three LSMs with:
>
> LSM#1: lbs_xattr 1
> LSM#2: lbs_xattr 2 (disabled)
> LSM#3: lbs_xattr 1
>
> The current compaction interval is: already compacted xattrs - end of
> new_xattr array.
>
> When the security_inode_init_security() loop calls LSM#3, the
> compaction interval is: 1 - 2 (LSM#2 returns 0), which clearly isn't
> right. The correct compaction interval should be: 3 - 4.
>
> Going to the end of new_xattrs is an approximation, but it ensures
> that security_check_compact_filled_xattrs() reaches the xattr set by
> LSM#3.
>
> The alternative I was mentioning of passing num_filled_xattrs to LSMs
> goes again in the direction of doing on-the-fly compaction, while LSMs
> are more familiar with using the lbs_* fields.
I guess I was thinking of the case where the LSM layer, i.e.
security_inode_init_security(), allocates an xattr array like it does
now based on the maximum number of xattrs possible using the
lsm_blob_sizes values and passes a pointer to the individual LSMs
which is incremented based on how many xattrs are created by the
individual LSMs.  Here is some *very* rough pseudo code:

int security_inode_init_security(...)
{

  /* allocate an xattr array */
  xattrs = kcalloc(blob_sizes, sizeof(*xattrs), GFP_BLAH);

  /* loop on the lsms */
  xa_cnt = 0;
  while (lsm_hooks) {
    rc = call_hook(lsm_hook, &xattrs[xa_cnt]);
    if (rc > 0)
      xa_cnt += rc;
  }

  /* evm magic */
  evm_inode_init_security(...)
}

Does that work?  Am I missing something?

-- 
paul-moore.com

Possibly Parallel Threads

Search for more possibly parallel threads

Ocfs2 devel - Mar 2023 - [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Ocfs2-devel] [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Ocfs2-devel] [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init_security hook

Possibly Parallel Threads