Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm

["Guozihua (Scott)" <guozihua@huawei.com>]

On 2023/1/31 19:31, Mimi Zohar wrote:
> On Mon, 2023-01-30 at 09:53 +0800, Guozihua (Scott) wrote:
>> On 2023/1/19 5:45, Mimi Zohar wrote:
>>> On Wed, 2022-12-28 at 11:02 +0800, Xiu Jianfeng wrote:
>>>> Currently it uses __vfs_setxattr_noperm() to update "security.evm",
>>>> however there are two lsm hooks(inode_post_setxattr and inode_setsecurity)
>>>> being called inside this function, which don't make any sense for xattr
>>>> "security.evm", because the handlers of these two hooks, such as selinux
>>>> and smack, only care about their own xattr.
>>>
>>> Updating the security.ima hash triggers re-calculating and writing the
>>> security.evm HMAC.  Refer to evm_inode_post_setxattr().
>>
>> Hi Mimi,
>>
>> I believe what Jianfeng is trying to do is to avoid re-triggering
>> security_inode_post_setxattr if we are updating security.evm. I can't
>> think of any other xattr that could "absorb" security.evm.
> 
> I understand.  Comments below ...
>>>
>>>>
>>>> On the other hand, there is a literally rather than actually cyclical
>>>> callchain as follows:
>>>> security_inode_post_setxattr
>>>>   ->evm_inode_post_setxattr
>>>>     ->evm_update_evmxattr
>>>>       ->__vfs_setxattr_noperm
>>>>         ->security_inode_post_setxattr
>>>>
>>>> So use __vfs_setxattr() to update "security.evm".
>>>>
>>>> Signed-off-by: Xiu Jianfeng <xiujianfeng@huawei.com>
>>>> ---
>>>>  security/integrity/evm/evm_crypto.c   | 7 +++----
>>>>  security/integrity/ima/ima_appraise.c | 8 ++++----
>>>>  2 files changed, 7 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>> index fa5ff13fa8c9..d8275dfa49ef 100644
>>>> --- a/security/integrity/evm/evm_crypto.c
>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>> @@ -376,10 +376,9 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>>>>  			   xattr_value_len, &data);
>>>>  	if (rc == 0) {
>>>>  		data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
>>>> -		rc = __vfs_setxattr_noperm(&init_user_ns, dentry,
>>>> -					   XATTR_NAME_EVM,
>>>> -					   &data.hdr.xattr.data[1],
>>>> -					   SHA1_DIGEST_SIZE + 1, 0);
>>>> +		rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>>>> +				    XATTR_NAME_EVM, &data.hdr.xattr.data[1],
>>>> +				    SHA1_DIGEST_SIZE + 1, 0);
> 
> Although __vfs_setxattr_noperm() doesn't do any permission checking, it
> does other things - make sure the filesystem supports writing xattrs,
> calls fsnotify_xattr().

Thanks for the explanation Mimi, this makes sense.
> 
>>>>  	} else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>>>>  		rc = __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_EVM);
>>>>  	}
>>>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>>>> index ee6f7e237f2e..d2de9dc6c345 100644
>>>> --- a/security/integrity/ima/ima_appraise.c
>>>> +++ b/security/integrity/ima/ima_appraise.c
>>>> @@ -98,10 +98,10 @@ static int ima_fix_xattr(struct dentry *dentry,
>>>>  		iint->ima_hash->xattr.ng.type = IMA_XATTR_DIGEST_NG;
>>>>  		iint->ima_hash->xattr.ng.algo = algo;
>>>>  	}
>>>> -	rc = __vfs_setxattr_noperm(&init_user_ns, dentry, XATTR_NAME_IMA,
>>>> -				   &iint->ima_hash->xattr.data[offset],
>>>> -				   (sizeof(iint->ima_hash->xattr) - offset) +
>>>> -				   iint->ima_hash->length, 0);
>>>> +	rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>>>> +			    XATTR_NAME_IMA, &iint->ima_hash->xattr.data[offset],
>>>> +			    (sizeof(iint->ima_hash->xattr) - offset) +
>>>> +			    iint->ima_hash->length, 0);
> 
> To clarify, ima_fix_xattr() is either directly called when in "fix"
> mode or from ima_update_xattr().  With this change, the recalculated
> file hash would be written to security.ima, but security.evm would not
> be updated.

Sorry I missed this part. I agree that it is not a good idea to alter
ima_fix_xattr().
> 
>>>>  	return rc;
>>>>  }
> 

-- 
Best
GUO Zihua