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

[Mimi Zohar]

Hi Roberto,

Sorry for the delay in responding...

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.

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.  

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

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

-- 
thanks,

Mimi