Re: [PATCH v8 4/6] security: Allow all LSMs to provide xattrs for inode_init_security hook
From: Roberto Sassu
Date: Mon Mar 27 2023 - 03:37:18 EST
On Fri, 2023-03-24 at 17:19 -0400, Paul Moore wrote:
> On Fri, Mar 24, 2023 at 6:18 AM Roberto Sassu
> <roberto.sassu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
> > > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx> (EVM crash)
> > > > Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > Reviewed-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > > > Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> > > > ---
> > > > 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? ;)
> >
> > 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.
>
> As a general rule I think it's a good goal to keep the LSM layer as
> small as possible; I believe it allows us to be more flexible with the
> LSMs and it keeps the LSM as simple as possible. I mean less code,
> less bugs, amirite? ... ;)
>
> > 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().
>
> I like the idea of individual LSMs simply reporting the number of
> xattrs they've generated instead of incrementing the num_filled_xattrs
> variable.
>
> It seems like returning the xattr count as a positive return value
> should work just fine, leaving negative values for errors, but if you
> run into problems you can always pass the value back in a new
> parameter pointer if needed.
>
> > > > @@ -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.
>
> Okay, I thought I might be missing something during the review. Since
> there is no special reason for putting the xattr->name assignment up
> there, please move it down below with the other xattr->XXX
> assignments.
Ok, will do.
Thanks
Roberto