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

[Casey Schaufler]

On 7/6/2023 6:43 PM, Paul Moore wrote:
> On Jun 10, 2023 Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
>> Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/
>> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>> ---
>>  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?
>
> 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 ... ?

Yes, please feel free to add my Acked-by as needed.

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