Hi,
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@xxxxxxxxxx>
>>>> ---
>>>> 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().
>
>>>> } 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.
Thanks for you explanation, I will drop this patch.
>
>>>> return rc;
>>>> }
>
