RE: [PATCH v12 1/4] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Roberto Sassu <roberto.sassu@huawei.com>]

> From: Paul Moore [mailto:paul@paul-moore.com]
> Sent: Friday, July 7, 2023 3:44 AM
> On Jun 10, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote:
> >
> > Currently, the LSM infrastructure 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.
> >
> > Modify the inode_init_security hook definition, by passing the full
> > xattr array allocated in security_inode_init_security(), and the current
> > number of xattr slots in that array filled by LSMs. The first parameter
> > would allow EVM to access and calculate the HMAC on xattrs supplied by
> > other LSMs, the second to not leave gaps in the xattr array, when an LSM
> > requested but did not provide xattrs (e.g. if it is not initialized).
> >
> > Introduce lsm_get_xattr_slot(), which LSMs can call as many times as the
> > number specified in the lbs_xattr_count field of the lsm_blob_sizes
> > structure. During each call, lsm_get_xattr_slot() increments the number of
> > filled xattrs, so that at the next invocation it returns the next xattr
> > slot to fill.
> >
> > Cleanup security_inode_init_security(). Unify the !initxattrs and
> > initxattrs case by simply not allocating the new_xattrs array in the
> > former. Update the documentation to reflect the changes, and fix the
> > description of the xattr name, as it is not allocated anymore.
> >
> > Adapt both SELinux and Smack to use the new definition of the
> > inode_init_security hook, and to call lsm_get_xattr_slot() to obtain and
> > fill the reserved slots in the xattr array.
> >
> > Move the xattr->name assignment after the xattr->value one, so that it is
> > done only in case of successful memory allocation.
> >
> > Finally, change the default return value of the inode_init_security hook
> > from zero to -EOPNOTSUPP, so that BPF LSM correctly follows the hook
> > conventions.
> >
> > Reported-by: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org>
> > 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    | 25 ++++++------
> >  5 files changed, 92 insertions(+), 47 deletions(-)
> 
> Two *very* small suggestions below, but I can make those during the
> merge if you are okay with that Roberto?

Hi Paul

yes, sure, I'm ok with them. Please make them during the merge.

Thanks

Roberto

> I'm also going to assume that Casey is okay with the Smack portion of
> this patchset?  It looks fine to me, and considering his ACK on the
> other Smack patch in this patchset I'm assuming he is okay with this
> one as well ... ?
> 
> > diff --git a/security/security.c b/security/security.c
> > index ee4f1cc4902..d5ef7df1ce4 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -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
> 
> I think we want to change "@xattrs array" to just "xattrs array" as
> there is no function parameter named "xattrs" in the LSM/security_XXX
> hook itself, just in the 'inode_init_security' hook implementation.
> 
> I might also break the new text describing the hook implementation
> into a new paragraph.
> 
> > + * 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;
> 
> The above comments were nitpicky, this one is even more so ...
> convention within security/security.c is to call the
> security_hook_list pointer "hp", not "P" (although I recognize P is
> used in the macro).
> 
> > +	struct xattr *new_xattrs = NULL;
> > +	int ret = -EOPNOTSUPP, xattr_count = 0;
> 
> --
> paul-moore.com