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

On Mon, 2023-03-27 at 17:02 -0400, Paul Moore wrote:
> On Mon, Mar 27, 2023 at 3:30?AM Roberto Sassu
> <roberto.sassu at huaweicloud.com> wrote:
> > On Fri, 2023-03-24 at 17:39 -0400, Paul Moore wrote:
> > > 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?
> > 
> > Oh, unfortunately not. EVM needs to see all xattrs (when it is moved
to
> > the LSM infrastructure).
> 
> Okay, that's fair, but we could still pass the full xattrs array and a
> reference to the current count which could be both read and updated by
> the individual LSMs, right?
Yes, we could do.
> The issue is that the separate compaction stage is not something we
> want to have to do if we can avoid it.  Maybe we're stuck with it, but
> I'm not yet convinced that we can't make some minor changes to the
> LSMs to avoid the compaction step.
I liked more the idea that LSMs do what they are most familiar with,
get an offset in a security blob or, in this case, a starting slot in
the new_xattrs array, and write there.

v3 had the lsm_find_xattr_slot() helper, to get the starting slot, but
somehow I find it less intuitive.

Ok, if you prefer to avoid the compaction stage, I will rewrite this
patch.

Thanks

Roberto

On Tue, Mar 28, 2023 at 3:47?AM Roberto Sassu
<roberto.sassu at huaweicloud.com> wrote:
>
> On Mon, 2023-03-27 at 17:02 -0400, Paul Moore wrote:
> > On Mon, Mar 27, 2023 at 3:30?AM Roberto Sassu
> > <roberto.sassu at huaweicloud.com> wrote:
> > > On Fri, 2023-03-24 at 17:39 -0400, Paul Moore wrote:
> > > > 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>
...
> > Okay, that's fair, but we could still pass the full xattrs array
and a
> > reference to the current count which could be both read and updated by
> > the individual LSMs, right?
>
> Yes, we could do.
>
> > The issue is that the separate compaction stage is not something we
> > want to have to do if we can avoid it.  Maybe we're stuck with it,
but
> > I'm not yet convinced that we can't make some minor changes to
the
> > LSMs to avoid the compaction step.
>
> I liked more the idea that LSMs do what they are most familiar with,
> get an offset in a security blob or, in this case, a starting slot in
> the new_xattrs array, and write there.
>
> v3 had the lsm_find_xattr_slot() helper, to get the starting slot, but
> somehow I find it less intuitive.
>
> Ok, if you prefer to avoid the compaction stage, I will rewrite this
> patch.
My concern is having to look through the xattr array after each LSM
has been run and in at least one case having to then do a memcpy() to
keep the array packed.  There are some cases where there is no way to
avoid all that extra work, but here I think we have the LSMs do the
Right Thing with respect to packing the xattr array without overly
burdening the individual LSMs.

Does that make sense?  It basically comes down to being smart about
our abstractions and both selectively, and carefully, breaking them
when there is a reasonable performance gain to be had.

-- 
paul-moore.com