Re: [PATCH v2 4/6] security: Support multiple LSMs implementing the inode_init_security hook

[Casey Schaufler]

On 4/21/2021 9:19 AM, Roberto Sassu wrote:
> The current implementation of security_inode_init_security() is capable 
of
> handling only one LSM providing an xattr to be set at inode creation. That
> xattr is then passed to EVM to calculate the HMAC.
>
> To support multiple LSMs, each providing an xattr, this patch makes the
> following modifications:
>
> security_inode_init_security():
> - dynamically allocates new_xattrs, based on the number of
>   inode_init_security hooks registered by LSMs;
> - replaces the call_int_hook() macro with its definition, to correctly
>   handle the case of an LSM returning -EOPNOTSUPP (the loop should not be
>   stopped).
>
> security_old_inode_init_security():
> - replaces the call_int_hook() macro with its definition, to stop the loop
>   at the first LSM providing an xattr, to avoid a memory leak (due to
>   overwriting the *value pointer).
>
> The modifications necessary for EVM to calculate the HMAC on all xattrs
> will be done in a separate patch.
>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
>  security/security.c | 93 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 82 insertions(+), 11 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 2c1fe1496069..2ab67fa4422e 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)
>  
> @@ -1028,9 +1026,10 @@ 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 *new_xattrs;
>  	struct xattr *lsm_xattr, *evm_xattr, *xattr;
> -	int ret;
> +	struct security_hook_list *P;
> +	int ret, max_new_xattrs = 0;
>  
>  	if (unlikely(IS_PRIVATE(inode)))
>  		return 0;
> @@ -1038,14 +1037,52 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  	if (!initxattrs)
>  		return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
>  				     dir, qstr, NULL, fs_data);
> -	memset(new_xattrs, 0, sizeof(new_xattrs));
> +
> +	/* Determine at run-time the max number of xattr structs to allocate. 
*/
> +	hlist_for_each_entry(P, &security_hook_heads.inode_init_security, list)
> +		max_new_xattrs++;

Don't do this here! You can count the number of modules providing inode_init_security
hooks when the modules register and save the value for use here. It's not 
going to
change.

> +
> +	if (!max_new_xattrs)
> +		return 0;
> +
> +	/* Allocate +1 for EVM and +1 as terminator. */
> +	new_xattrs = kcalloc(max_new_xattrs + 2, sizeof(*new_xattrs), GFP_NOFS);
> +	if (!new_xattrs)
> +		return -ENOMEM;
> +
>  	lsm_xattr = new_xattrs;
> -	ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
> -			    lsm_xattr, fs_data);
> -	if (ret)
> +	hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
> +			     list) {
> +		ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs,
> +						  fs_data);
> +		if (ret) {
> +			if (ret != -EOPNOTSUPP)
> +				goto out;
> +
> +			continue;
> +		}
> +
> +		/* LSM implementation error. */
> +		if (lsm_xattr->name == NULL || lsm_xattr->value == NULL) {
> +			WARN_ONCE(
> +				"LSM %s: ret = 0 but xattr name/value = NULL\n",
> +				P->lsm);
> +			ret = -ENOENT;
> +			goto out;
> +		}
> +
> +		lsm_xattr++;
> +
> +		if (!--max_new_xattrs)
> +			break;
> +	}
> +
> +	if (!new_xattrs->name) {
> +		ret = -EOPNOTSUPP;
>  		goto out;
> +	}
>  
> -	evm_xattr = lsm_xattr + 1;
> +	evm_xattr = lsm_xattr;
>  	ret = evm_inode_init_security(inode, new_xattrs, evm_xattr);

 +	ret = evm_inode_init_security(inode, new_xattrs, lsm_xattr);

then you don't need evm_xattr at all.

>  	if (ret)
>  		goto out;
> @@ -1053,6 +1090,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>  out:
>  	for (xattr = new_xattrs; xattr->value != NULL; xattr++)
>  		kfree(xattr->value);
> +	kfree(new_xattrs);
>  	return (ret == -EOPNOTSUPP) ? 0 : ret;
>  }
>  EXPORT_SYMBOL(security_inode_init_security);
> @@ -1071,11 +1109,44 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir,
>  {
>  	struct xattr xattr = { .name = NULL, .value = NULL, .value_len = 0 };
>  	struct xattr *lsm_xattr = (name && value && len) ? &xattr : NULL;
> +	struct security_hook_list *P;
> +	int ret;
>  
>  	if (unlikely(IS_PRIVATE(inode)))
>  		return -EOPNOTSUPP;
> -	return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
> -			     qstr, lsm_xattr, NULL);
> +
> +	hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
> +			     list) {
> +		ret = P->hook.inode_init_security(inode, dir, qstr, lsm_xattr,
> +						  NULL);
> +		if (ret) {
> +			if (ret != -EOPNOTSUPP)
> +				return ret;
> +
> +			continue;
> +		}
> +
> +		if (!lsm_xattr)
> +			continue;
> +
> +		/* LSM implementation error. */
> +		if (lsm_xattr->name == NULL || lsm_xattr->value == NULL) {
> +			WARN_ONCE(
> +				"LSM %s: ret = 0 but xattr name/value = NULL\n",
> +				P->lsm);
> +
> +			/* Callers should do the cleanup. */
> +			return -ENOENT;
> +		}
> +
> +		*name = lsm_xattr->name;
> +		*value = lsm_xattr->value;
> +		*len = lsm_xattr->value_len;
> +
> +		return ret;
> +	}
> +
> +	return -EOPNOTSUPP;
>  }
>  EXPORT_SYMBOL(security_old_inode_init_security);
>