Message-ID: <fe16a03a-102e-b3e1-cc3f-5bad3c28fad7@huaweicloud.com>
Date:   Fri, 18 Nov 2022 10:14:18 +0100
From:   Roberto Sassu <roberto.sassu@...weicloud.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>, dmitry.kasatkin@...il.com,
        paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
        stephen.smalley.work@...il.com, eparis@...isplace.org,
        casey@...aufler-ca.com
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        reiserfs-devel@...r.kernel.org, linux-kernel@...r.kernel.org,
        keescook@...omium.org, nicolas.bouchinet@...p-os.org,
        Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v4 3/5] security: Allow all LSMs to provide xattrs for
 inode_init_security hook

On 11/17/2022 5:05 PM, Mimi Zohar wrote:
> hOn Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu@...wei.com>
>>
>> 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.

EVM wouldn't notice whether it is the same LSM that provide multiple 
xattrs or multiple LSMs provided one xattr. As long as the xattr array 
contains consecutive xattrs, that would be fine. In the IMA/EVM test I 
included a test case where an LSM provides two xattrs (seems to work fine).

>> 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@...wei.com>
>> ---
> 
>> 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.

Ok, will improve it.

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

An LSM might reserve one or more xattrs, but not set it/them (for 
example because it is not initialized). In this case, removing the gaps 
is needed for all subsequent LSMs.

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

Ok.

Thanks

Roberto

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

Powered by blists - more mailing lists