Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for inode_init_security hook

[Mimi Zohar]

hOn Thu, 2022-11-10 at 10:46 +0100, 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 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.

Perhaps supporting per LSM multiple xattrs is a nice idea, but EVM
doesn't currently support it.  The LSM xattrs are hard coded in
evm_config_default_xattrnames[],  based on whether the LSM is
configured.  Additional security xattrs may be included in the
security.evm calculation, by extending the list via
security/integrity/evm/evm_xattrs.

> 
> 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.
> 
> 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_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 the xattr 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.
> 
> 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.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---

> diff --git a/security/security.c b/security/security.c
> index a0e9b4ce2341..b62f192de6da 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -30,8 +30,6 @@
>  #include <linux/msg.h>
>  #include <net/flow.h>
>  
> -#define MAX_LSM_EVM_XATTR	2
> -
>  /* How many LSMs were built into the kernel? */
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>  
> @@ -210,6 +208,7 @@ static void __init lsm_set_blob_sizes(struct lsm_blob_sizes *needed)
>  	lsm_set_blob_size(&needed->lbs_msg_msg, &blob_sizes.lbs_msg_msg);
>  	lsm_set_blob_size(&needed->lbs_superblock, &blob_sizes.lbs_superblock);
>  	lsm_set_blob_size(&needed->lbs_task, &blob_sizes.lbs_task);
> +	lsm_set_blob_size(&needed->lbs_xattr, &blob_sizes.lbs_xattr);
>  }
>  
>  /* Prepare LSM for initialization. */
> @@ -346,6 +345,7 @@ static void __init ordered_lsm_init(void)
>  	init_debug("msg_msg blob size    = %d\n", blob_sizes.lbs_msg_msg);
>  	init_debug("superblock blob size = %d\n", blob_sizes.lbs_superblock);
>  	init_debug("task blob size       = %d\n", blob_sizes.lbs_task);
> +	init_debug("xattr slots          = %d\n", blob_sizes.lbs_xattr);
>  
>  	/*
>  	 * Create any kmem_caches needed for blobs
> @@ -1100,34 +1100,78 @@ static int security_initxattrs(struct inode *inode, const struct xattr *xattrs,
>  	return 0;
>  }

> +static int security_check_compact_xattrs(struct xattr *xattrs,
> +					 int num_xattrs, int *checked_xattrs)

Perhaps the variable naming is off, making it difficult to read.   So
although this is a static function, which normally doesn't require a
comment, it's definitely needs one.

> +{
> +	int i;
> +
> +	for (i = *checked_xattrs; i < num_xattrs; i++) {

If the number of "checked" xattrs was kept up to date, removing the
empty xattr gaps wouldn't require a loop.  Is the purpose of this loop
to support multiple per LSM xattrs?

> +		if ((!xattrs[i].name && xattrs[i].value) ||
> +		    (xattrs[i].name && !xattrs[i].value))
> +			return -EINVAL;
> +
> +		if (!xattrs[i].name)
> +			continue;
> +
> +		if (i == *checked_xattrs) {
> +			(*checked_xattrs)++;
> +			continue;
> +		}
> +
> +		memcpy(xattrs + (*checked_xattrs)++, xattrs + i,
> +		       sizeof(*xattrs));
> +		memset(xattrs + i, 0, sizeof(*xattrs));
> +	}
> +
> +	return 0;
> +}
> +
>  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 = -EOPNOTSUPP;
> +	struct security_hook_list *P;
> +	struct xattr *new_xattrs;
> +	struct xattr *xattr;
> +	int ret = -EOPNOTSUPP, cur_xattrs = 0;
>  
>  	if (unlikely(IS_PRIVATE(inode)))
>  		goto out_exit;
>  
> +	if (!blob_sizes.lbs_xattr)
> +		goto out_exit;
> +
>  	if (!initxattrs ||
>  	    (initxattrs == &security_initxattrs && !fs_data)) {
>  		ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
> -				    dir, qstr, NULL, NULL, NULL);
> +				    dir, qstr, NULL);
>  		goto out_exit;
>  	}
> -	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)
> -		goto out;
> +	/* Allocate +1 for EVM and +1 as terminator. */
> +	new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2, sizeof(*new_xattrs),
> +			     GFP_NOFS);
> +	if (!new_xattrs) {
> +		ret = -ENOMEM;
> +		goto out_exit;
> +	}
> +	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;
> +		if (ret == -EOPNOTSUPP)
> +			continue;
> +		ret = security_check_compact_xattrs(new_xattrs,
> +						    blob_sizes.lbs_xattr,
> +						    &cur_xattrs);

Defining a variable named "cur_xattrs" to indicate the number of xattrs
compressed is off.  Perhaps use cur_num_xattrs?   Similarly,
"checked_xattrs" should be num_checked_xattrs.  Or change the existing
num_xattrs to max_num_xattrs and rename checked_xattrs to num_xattrs.

thanks,

Mimi

> +		if (ret < 0) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
> +	}
>  
> -	evm_xattr = lsm_xattr + 1;
> -	ret = evm_inode_init_security(inode, lsm_xattr, evm_xattr);
> +	ret = evm_inode_init_security(inode, new_xattrs,
> +				      new_xattrs + cur_xattrs);
>  	if (ret)
>  		goto out;
>  	ret = initxattrs(inode, new_xattrs, fs_data);
> @@ -1142,6 +1186,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  			continue;
>  		kfree(xattr->value);
>  	}
> +	kfree(new_xattrs);
>  out_exit:
>  	if (initxattrs == &security_initxattrs)
>  		return ret;