* [PATCH -next] evm: Use __vfs_setxattr() to update security.evm
@ 2022-12-28 3:02 Xiu Jianfeng
2023-01-18 21:45 ` Mimi Zohar
0 siblings, 1 reply; 6+ messages in thread
From: Xiu Jianfeng @ 2022-12-28 3:02 UTC (permalink / raw)
To: zohar, dmitry.kasatkin, paul, jmorris, serge
Cc: linux-integrity, linux-security-module, linux-kernel
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.
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);
} 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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm 2022-12-28 3:02 [PATCH -next] evm: Use __vfs_setxattr() to update security.evm Xiu Jianfeng @ 2023-01-18 21:45 ` Mimi Zohar 2023-01-30 1:53 ` Guozihua (Scott) 0 siblings, 1 reply; 6+ messages in thread From: Mimi Zohar @ 2023-01-18 21:45 UTC (permalink / raw) To: Xiu Jianfeng, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel 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 > > 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); > } 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; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm 2023-01-18 21:45 ` Mimi Zohar @ 2023-01-30 1:53 ` Guozihua (Scott) 2023-01-31 11:31 ` Mimi Zohar 0 siblings, 1 reply; 6+ messages in thread From: Guozihua (Scott) @ 2023-01-30 1:53 UTC (permalink / raw) To: Mimi Zohar, Xiu Jianfeng, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel 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@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); >> } 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm 2023-01-30 1:53 ` Guozihua (Scott) @ 2023-01-31 11:31 ` Mimi Zohar 2023-02-01 6:42 ` xiujianfeng 2023-02-01 7:10 ` Guozihua (Scott) 0 siblings, 2 replies; 6+ messages in thread From: Mimi Zohar @ 2023-01-31 11:31 UTC (permalink / raw) To: Guozihua (Scott), Xiu Jianfeng, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel 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(). > >> } 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. > >> return rc; > >> } -- thanks, Mimi ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm 2023-01-31 11:31 ` Mimi Zohar @ 2023-02-01 6:42 ` xiujianfeng 2023-02-01 7:10 ` Guozihua (Scott) 1 sibling, 0 replies; 6+ messages in thread From: xiujianfeng @ 2023-02-01 6:42 UTC (permalink / raw) To: Mimi Zohar, Guozihua (Scott), dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel 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@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(). > >>>> } 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; >>>> } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH -next] evm: Use __vfs_setxattr() to update security.evm 2023-01-31 11:31 ` Mimi Zohar 2023-02-01 6:42 ` xiujianfeng @ 2023-02-01 7:10 ` Guozihua (Scott) 1 sibling, 0 replies; 6+ messages in thread From: Guozihua (Scott) @ 2023-02-01 7:10 UTC (permalink / raw) To: Mimi Zohar, Xiu Jianfeng, dmitry.kasatkin, paul, jmorris, serge Cc: linux-integrity, linux-security-module, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-01 7:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-12-28 3:02 [PATCH -next] evm: Use __vfs_setxattr() to update security.evm Xiu Jianfeng 2023-01-18 21:45 ` Mimi Zohar 2023-01-30 1:53 ` Guozihua (Scott) 2023-01-31 11:31 ` Mimi Zohar 2023-02-01 6:42 ` xiujianfeng 2023-02-01 7:10 ` Guozihua (Scott)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).