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

[Casey Schaufler]

On 4/11/2023 12:53 AM, Roberto Sassu wrote:
> On Tue, 2023-04-11 at 03:22 -0400, Mimi Zohar wrote:
>> Hi Roberto,
>>
>> Sorry for the delay in responding...
> Hi Mimi
>
> no worries!
>
>> The patch description reads as though support for per LSM multiple
>> xattrs is being added in this patch, though lsm_get_xattr_slot() only
>> ever is incremented once for each LSM.  To simplify review, it would be
>> nice to mention that lsm_get_xattr_slot() would be called multiple
>> times per LSM xattr.
> Ok, I will mention it.
>
>> On Fri, 2023-03-31 at 14:32 +0200, Roberto Sassu 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_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.
>>>
>>> Dynamically allocate the new_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. Unify the !initxattrs and initxattrs case, simply
>>> don't allocate the new_xattrs array in the former.
>>>
>>> Also, pass to the hook the number of xattrs filled by each LSM, so that
>>> there are no gaps when the next LSM fills the array. Gaps might occur
>>> because an LSM can legitimately request xattrs to the LSM infrastructure,
>>> but not fill the reserved slots, if it was not initialized.
>> The number of security xattrs permitted per LSM was discussed in the
>> second paragraph.  The first line of this paragraph needs to be updated
>> to reflect the current number of security xattrs used, though that is
>> more related to the new lsm_get_xattr_slot().  Or perhaps the entire
>> paragraph is unnecessary, a remnant from
>> security_check_compact_filled_xattrs(), and should be removed.  
> I would probably say in that paragraph that the number specified for
> the lbs_xattr_count field determines how many times an LSM can call
> lsm_get_xattr_slot().
>
>>> Update the documentation of security_inode_init_security() to reflect the
>>> changes, and fix the description of the xattr name, as it is not allocated
>>> anymore.
>>>
>>> Finally, adapt both SELinux and Smack to use the new definition of the
>>> inode_init_security hook, and to fill the reserved slots in the xattr
>>> array. Introduce the lsm_get_xattr_slot() helper to retrieve an available
>>> slot to fill, and to increment the number of filled slots.
>>>
>>> Move the xattr->name assignment after the xattr->value one, so that it is
>>> done only in case of successful memory allocation. For Smack, also reserve
>>> space for the other defined xattrs although they are not set yet in
>>> smack_inode_init_security().
>> This Smack comment should be moved to the previous paragraph and even
>> expanded explaining that lsm_get_xattr_slot() will be called for each
>> additional security xattr.
> >From previous Paul's and Casey's comments, Smack will have just two
> xattrs, assuming that security.SMACK_TRASMUTE64 can be set in
> smack_inode_init_security(). I will change this part accordingly once
> Casey can have a look at the function.

To be clear, Smack may use two xattrs from smack_inode_init_security(),
SMACK64 and SMACK64_TRANSMUTE. SMACK64_TRANSMUTE is only set on directories.
SMACK64_MMAP and SMACK64_EXEC can be set on files, but they have to be
set explicitly. A file may have three xattrs, but only one from
smack_inode_init_security().

I'm looking at the existing Smack function, and it includes checking for
the transmute attribute. Your patch seems to have dropped this important
behavior. That needs to be restored in any case. You can tell that you need
to include the SMACK64_TRANSMUTE xattr if setting it is detected.

>
>>> 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>
>>> ---
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index c2be66c669a..9eb9b686493 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -28,6 +28,7 @@
>>>  #include <linux/security.h>
>>>  #include <linux/init.h>
>>>  #include <linux/rculist.h>
>>> +#include <linux/xattr.h>
>>>  
>>>  union security_list_options {
>>>  	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
>>> @@ -63,8 +64,27 @@ struct lsm_blob_sizes {
>>>  	int	lbs_ipc;
>>>  	int	lbs_msg_msg;
>>>  	int	lbs_task;
>>> +	int	lbs_xattr_count; /* number of xattr slots in new_xattrs array */
>>>  };
>>>  
>>> +/**
>>> + * lsm_get_xattr_slot - Return the next available slot and increment the index
>>> + * @xattrs: array storing LSM-provided xattrs
>>> + * @xattr_count: number of already stored xattrs (updated)
>>> + *
>>> + * Retrieve the first available slot in the @xattrs array to fill with an xattr,
>>> + * and increment @xattr_count.
>>> + *
>>> + * Return: The slot to fill in @xattrs if non-NULL, NULL otherwise.
>>> + */
>>> +static inline struct xattr *lsm_get_xattr_slot(struct xattr *xattrs,
>>> +					       int *xattr_count)
>>> +{
>>> +	if (unlikely(!xattrs))
>>> +		return NULL;
>>> +	return xattrs + (*xattr_count)++;
>> At some point, since lsm_get_xattr_slot() could be called multiple
>> times from the same LSM, shouldn't there be some sort of bounds
>> checking?
> >From previous Paul's comments, I understood that he prefers to avoid
> extra checks. It will be up to LSM developers to ensure that the API is
> used correctly.
>
> Thanks
>
> Roberto
>