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().
>
> Mimi
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.
>
>>
>> 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);
>> } 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);
>> return rc;
>> }
>>
>
>
--
Best
GUO Zihua
