* [PATCH v2 0/4] integrity: few code cleanups
@ 2014-10-01 18:43 Dmitry Kasatkin
2014-10-01 18:43 ` [PATCH v2 1/4] integrity: add missing '__init' keyword for integrity_init_keyring() Dmitry Kasatkin
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Dmitry Kasatkin @ 2014-10-01 18:43 UTC (permalink / raw)
To: zohar, linux-ima-devel, linux-security-module
Cc: linux-kernel, Dmitry Kasatkin
Here is few simple code cleanups.
Please refer to the patch descriptions for details.
They previously were posted on linux-ima-devel mailing list
and feedback was addressed.
- Dmitry
Dmitry Kasatkin (4):
integrity: add missing '__init' keyword for integrity_init_keyring()
evm: skip replacing EVM signature with HMAC on read-only filesystem
ima: check appraisal flag in the ima_file_free() hook
ima: use path names cache
security/integrity/digsig.c | 2 +-
security/integrity/evm/evm_main.c | 11 ++++++++---
security/integrity/iint.c | 3 ---
security/integrity/ima/ima_api.c | 4 ++--
security/integrity/ima/ima_main.c | 5 +++--
security/integrity/integrity.h | 5 +----
6 files changed, 15 insertions(+), 15 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/4] integrity: add missing '__init' keyword for integrity_init_keyring() 2014-10-01 18:43 [PATCH v2 0/4] integrity: few code cleanups Dmitry Kasatkin @ 2014-10-01 18:43 ` Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 2/4] evm: skip replacing EVM signature with HMAC on read-only filesystem Dmitry Kasatkin ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-01 18:43 UTC (permalink / raw) To: zohar, linux-ima-devel, linux-security-module Cc: linux-kernel, Dmitry Kasatkin integrity_init_keyring() is used only from kernel '__init' functions. Add it there as well. Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> --- security/integrity/digsig.c | 2 +- security/integrity/integrity.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c index 8d4fbff..4f643d1 100644 --- a/security/integrity/digsig.c +++ b/security/integrity/digsig.c @@ -63,7 +63,7 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, return -EOPNOTSUPP; } -int integrity_init_keyring(const unsigned int id) +int __init integrity_init_keyring(const unsigned int id) { const struct cred *cred = current_cred(); int err = 0; diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index c0379d1..aafb468 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -129,7 +129,7 @@ struct integrity_iint_cache *integrity_iint_find(struct inode *inode); int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen, const char *digest, int digestlen); -int integrity_init_keyring(const unsigned int id); +int __init integrity_init_keyring(const unsigned int id); #else static inline int integrity_digsig_verify(const unsigned int id, -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] evm: skip replacing EVM signature with HMAC on read-only filesystem 2014-10-01 18:43 [PATCH v2 0/4] integrity: few code cleanups Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 1/4] integrity: add missing '__init' keyword for integrity_init_keyring() Dmitry Kasatkin @ 2014-10-01 18:43 ` Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 4/4] ima: use path names cache Dmitry Kasatkin 3 siblings, 0 replies; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-01 18:43 UTC (permalink / raw) To: zohar, linux-ima-devel, linux-security-module Cc: linux-kernel, Dmitry Kasatkin If filesystem is mounted read-only or file is immutable, updating xattr will fail. This is a usual case during early boot until filesystem is remount read-write. This patch verifies conditions to skip unnecessary attempt to calculate HMAC and set xattr. Changes in v2: * indention changed according to Lindent (requested by Mimi) Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> --- security/integrity/evm/evm_main.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 9685af3..b392fe6 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -162,9 +162,14 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, (const char *)xattr_data, xattr_len, calc.digest, sizeof(calc.digest)); if (!rc) { - /* we probably want to replace rsa with hmac here */ - evm_update_evmxattr(dentry, xattr_name, xattr_value, - xattr_value_len); + /* Replace RSA with HMAC if not mounted readonly and + * not immutable + */ + if (!IS_RDONLY(dentry->d_inode) && + !IS_IMMUTABLE(dentry->d_inode)) + evm_update_evmxattr(dentry, xattr_name, + xattr_value, + xattr_value_len); } break; default: -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-01 18:43 [PATCH v2 0/4] integrity: few code cleanups Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 1/4] integrity: add missing '__init' keyword for integrity_init_keyring() Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 2/4] evm: skip replacing EVM signature with HMAC on read-only filesystem Dmitry Kasatkin @ 2014-10-01 18:43 ` Dmitry Kasatkin 2014-10-02 8:26 ` [Linux-ima-devel] " Roberto Sassu 2014-10-01 18:43 ` [PATCH v2 4/4] ima: use path names cache Dmitry Kasatkin 3 siblings, 1 reply; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-01 18:43 UTC (permalink / raw) To: zohar, linux-ima-devel, linux-security-module Cc: linux-kernel, Dmitry Kasatkin ima_file_free() hook is only used by appraisal module to update hash when file was modified. When there were no integrity checks on inode, S_IMA flag is not set, integrity_iint_find() returns NULL and it quits. When inode is only measured/audited but not appraised, iint is allocated and it will cause calling ima_check_last_writer() which unnecessarily locks i_mutex. Currently ima_file_free() checks 'iint_initialized'. But it looks that it is a mistake and originally 'ima_appraise' had to be used instead. In fact usage of 'iint_initialized' is completely unnecessary because S_IMA flag would not be set if iint was not allocated. This patch uses lately introduced ima_policy_flag to test if appraisal was enabled by policy. With this change 'iint_initialized' is no longer used anywhere. Indeed, integrity cache is allocated with SLAB_PANIC and kernel will panic if allocation fails during kernel initialization. So this variable is unnecessary and thus this patch removes it. Changes in v2: * 'iint_initialized' removal patch merged to this patch (requested by Mimi) Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> --- security/integrity/iint.c | 3 --- security/integrity/ima/ima_main.c | 2 +- security/integrity/integrity.h | 3 --- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index a521edf..cc3eb4d 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; static DEFINE_RWLOCK(integrity_iint_lock); static struct kmem_cache *iint_cache __read_mostly; -int iint_initialized; - /* * __integrity_iint_find - return the iint associated with an inode */ @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) iint_cache = kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), 0, SLAB_PANIC, init_once); - iint_initialized = 1; return 0; } security_initcall(integrity_iintcache_init); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 62f59ec..87d7df7 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) struct inode *inode = file_inode(file); struct integrity_iint_cache *iint; - if (!iint_initialized || !S_ISREG(inode->i_mode)) + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) return; iint = integrity_iint_find(inode); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index aafb468..f51ad65 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, { } #endif - -/* set during initialization */ -extern int iint_initialized; -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-01 18:43 ` [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook Dmitry Kasatkin @ 2014-10-02 8:26 ` Roberto Sassu 2014-10-02 9:21 ` [PATCH 1/1] ima: check ima_policy_flag " Dmitry Kasatkin 2014-10-02 9:30 ` [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag " Dmitry Kasatkin 0 siblings, 2 replies; 13+ messages in thread From: Roberto Sassu @ 2014-10-02 8:26 UTC (permalink / raw) To: Dmitry Kasatkin, zohar, linux-ima-devel, linux-security-module Cc: linux-kernel On 10/01/2014 08:43 PM, Dmitry Kasatkin wrote: > ima_file_free() hook is only used by appraisal module to update hash > when file was modified. When there were no integrity checks on inode, > S_IMA flag is not set, integrity_iint_find() returns NULL and it > quits. When inode is only measured/audited but not appraised, iint > is allocated and it will cause calling ima_check_last_writer() which > unnecessarily locks i_mutex. > > Currently ima_file_free() checks 'iint_initialized'. But it looks that > it is a mistake and originally 'ima_appraise' had to be used instead. > In fact usage of 'iint_initialized' is completely unnecessary because > S_IMA flag would not be set if iint was not allocated. > Hi Dmitry sorry, I think there are two mistakes here. First, ima_file_free() is not used by the appraisal module only because, if a file has been written, also the IMA_MEASURED and IMA_AUDITED flags are cleared. Your patch prevents further measurements/audits if a file is modified. Second, the lock of i_mutex is necessary, as it protects the access to the fields of the 'integrity_iint_cache' structure. My suggestion is to provide a patch that fixes the commit a756024e of Mimi's 'next' branch. The patch should just replace the check of 'iint_initialized' with 'ima_policy_flag'. The removal of 'iint_initialized' could be done in a separate patch. Thanks Roberto Sassu > This patch uses lately introduced ima_policy_flag to test if appraisal > was enabled by policy. > > With this change 'iint_initialized' is no longer used anywhere. Indeed, > integrity cache is allocated with SLAB_PANIC and kernel will panic if > allocation fails during kernel initialization. So this variable is > unnecessary and thus this patch removes it. > > Changes in v2: > * 'iint_initialized' removal patch merged to this patch (requested > by Mimi) > > Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> > --- > security/integrity/iint.c | 3 --- > security/integrity/ima/ima_main.c | 2 +- > security/integrity/integrity.h | 3 --- > 3 files changed, 1 insertion(+), 7 deletions(-) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index a521edf..cc3eb4d 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; > static DEFINE_RWLOCK(integrity_iint_lock); > static struct kmem_cache *iint_cache __read_mostly; > > -int iint_initialized; > - > /* > * __integrity_iint_find - return the iint associated with an inode > */ > @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) > iint_cache = > kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), > 0, SLAB_PANIC, init_once); > - iint_initialized = 1; > return 0; > } > security_initcall(integrity_iintcache_init); > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 62f59ec..87d7df7 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) > struct inode *inode = file_inode(file); > struct integrity_iint_cache *iint; > > - if (!iint_initialized || !S_ISREG(inode->i_mode)) > + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) > return; > > iint = integrity_iint_find(inode); > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h > index aafb468..f51ad65 100644 > --- a/security/integrity/integrity.h > +++ b/security/integrity/integrity.h > @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, > { > } > #endif > - > -/* set during initialization */ > -extern int iint_initialized; > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/1] ima: check ima_policy_flag in the ima_file_free() hook 2014-10-02 8:26 ` [Linux-ima-devel] " Roberto Sassu @ 2014-10-02 9:21 ` Dmitry Kasatkin 2014-10-02 9:30 ` [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag " Dmitry Kasatkin 1 sibling, 0 replies; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-02 9:21 UTC (permalink / raw) To: zohar, linux-ima-devel, linux-security-module, roberto.sassu Cc: linux-kernel, dmitry.kasatkin, Dmitry Kasatkin ima_file_free() checks 'iint_initialized' unnecessarily, because S_IMA flag would not be set if iint was not allocated. At the same time integrity cache is allocated with SLAB_PANIC and kernel will panic if allocation fails during kernel initialization. So on running system iint_initialized is always true and can be removed. This patch uses lately introduced ima_policy_flag to test if IMA is enabled by policy. Changes in v3: * not limiting test to IMA_APPRAISE (spotted by Roberto Sassu) Changes in v2: * 'iint_initialized' removal patch merged to this patch (requested by Mimi) Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> --- security/integrity/iint.c | 3 --- security/integrity/ima/ima_main.c | 2 +- security/integrity/integrity.h | 3 --- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/security/integrity/iint.c b/security/integrity/iint.c index a521edf..cc3eb4d 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; static DEFINE_RWLOCK(integrity_iint_lock); static struct kmem_cache *iint_cache __read_mostly; -int iint_initialized; - /* * __integrity_iint_find - return the iint associated with an inode */ @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) iint_cache = kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), 0, SLAB_PANIC, init_once); - iint_initialized = 1; return 0; } security_initcall(integrity_iintcache_init); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 62f59ec..72faf0b 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) struct inode *inode = file_inode(file); struct integrity_iint_cache *iint; - if (!iint_initialized || !S_ISREG(inode->i_mode)) + if (!ima_policy_flag || !S_ISREG(inode->i_mode)) return; iint = integrity_iint_find(inode); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index aafb468..f51ad65 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, { } #endif - -/* set during initialization */ -extern int iint_initialized; -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-02 8:26 ` [Linux-ima-devel] " Roberto Sassu 2014-10-02 9:21 ` [PATCH 1/1] ima: check ima_policy_flag " Dmitry Kasatkin @ 2014-10-02 9:30 ` Dmitry Kasatkin 2014-10-02 10:06 ` Roberto Sassu 1 sibling, 1 reply; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-02 9:30 UTC (permalink / raw) To: Roberto Sassu, Dmitry Kasatkin, zohar, linux-ima-devel, linux-security-module Cc: linux-kernel On 02/10/14 11:26, Roberto Sassu wrote: > On 10/01/2014 08:43 PM, Dmitry Kasatkin wrote: >> ima_file_free() hook is only used by appraisal module to update hash >> when file was modified. When there were no integrity checks on inode, >> S_IMA flag is not set, integrity_iint_find() returns NULL and it >> quits. When inode is only measured/audited but not appraised, iint >> is allocated and it will cause calling ima_check_last_writer() which >> unnecessarily locks i_mutex. >> >> Currently ima_file_free() checks 'iint_initialized'. But it looks that >> it is a mistake and originally 'ima_appraise' had to be used instead. >> In fact usage of 'iint_initialized' is completely unnecessary because >> S_IMA flag would not be set if iint was not allocated. >> > Hi Dmitry > > sorry, I think there are two mistakes here. > > First, ima_file_free() is not used by the appraisal module only > because, if a file has been written, also the IMA_MEASURED > and IMA_AUDITED flags are cleared. Your patch prevents further > measurements/audits if a file is modified. > > Second, the lock of i_mutex is necessary, as it protects the > access to the fields of the 'integrity_iint_cache' structure. > > My suggestion is to provide a patch that fixes the commit a756024e > of Mimi's 'next' branch. The patch should just replace the check > of 'iint_initialized' with 'ima_policy_flag'. The removal of > 'iint_initialized' could be done in a separate patch. > > Thanks > > Roberto Sassu Hi Roberto, You are right. Clearing flags slipped out from my eyes. In such case we need to test entire flag as we do in other places. But in such case the patch is more about remove iint_initialzed, because S_IMA flag would not be set anyway. I posted modified version. This patch and other patches were posted a week ago on linux-ima-devel mailing list. I appreciate you would review patches when they come there so we could spot more issues before they come to lsm mailing list. Thanks, Dmitry > >> This patch uses lately introduced ima_policy_flag to test if appraisal >> was enabled by policy. >> >> With this change 'iint_initialized' is no longer used anywhere. Indeed, >> integrity cache is allocated with SLAB_PANIC and kernel will panic if >> allocation fails during kernel initialization. So this variable is >> unnecessary and thus this patch removes it. >> >> Changes in v2: >> * 'iint_initialized' removal patch merged to this patch (requested >> by Mimi) >> >> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> >> --- >> security/integrity/iint.c | 3 --- >> security/integrity/ima/ima_main.c | 2 +- >> security/integrity/integrity.h | 3 --- >> 3 files changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/security/integrity/iint.c b/security/integrity/iint.c >> index a521edf..cc3eb4d 100644 >> --- a/security/integrity/iint.c >> +++ b/security/integrity/iint.c >> @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; >> static DEFINE_RWLOCK(integrity_iint_lock); >> static struct kmem_cache *iint_cache __read_mostly; >> >> -int iint_initialized; >> - >> /* >> * __integrity_iint_find - return the iint associated with an inode >> */ >> @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) >> iint_cache = >> kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), >> 0, SLAB_PANIC, init_once); >> - iint_initialized = 1; >> return 0; >> } >> security_initcall(integrity_iintcache_init); >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >> index 62f59ec..87d7df7 100644 >> --- a/security/integrity/ima/ima_main.c >> +++ b/security/integrity/ima/ima_main.c >> @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) >> struct inode *inode = file_inode(file); >> struct integrity_iint_cache *iint; >> >> - if (!iint_initialized || !S_ISREG(inode->i_mode)) >> + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) >> return; >> >> iint = integrity_iint_find(inode); >> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h >> index aafb468..f51ad65 100644 >> --- a/security/integrity/integrity.h >> +++ b/security/integrity/integrity.h >> @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, >> { >> } >> #endif >> - >> -/* set during initialization */ >> -extern int iint_initialized; >> > > ------------------------------------------------------------------------------ > Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer > Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports > Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper > Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer > http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk > _______________________________________________ > Linux-ima-devel mailing list > Linux-ima-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-ima-devel > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-02 9:30 ` [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag " Dmitry Kasatkin @ 2014-10-02 10:06 ` Roberto Sassu 2014-10-02 10:43 ` Dmitry Kasatkin 0 siblings, 1 reply; 13+ messages in thread From: Roberto Sassu @ 2014-10-02 10:06 UTC (permalink / raw) To: Dmitry Kasatkin, Dmitry Kasatkin, zohar, linux-ima-devel, linux-security-module Cc: linux-kernel On 10/02/2014 11:30 AM, Dmitry Kasatkin wrote: > On 02/10/14 11:26, Roberto Sassu wrote: >> On 10/01/2014 08:43 PM, Dmitry Kasatkin wrote: >>> ima_file_free() hook is only used by appraisal module to update hash >>> when file was modified. When there were no integrity checks on inode, >>> S_IMA flag is not set, integrity_iint_find() returns NULL and it >>> quits. When inode is only measured/audited but not appraised, iint >>> is allocated and it will cause calling ima_check_last_writer() which >>> unnecessarily locks i_mutex. >>> >>> Currently ima_file_free() checks 'iint_initialized'. But it looks that >>> it is a mistake and originally 'ima_appraise' had to be used instead. >>> In fact usage of 'iint_initialized' is completely unnecessary because >>> S_IMA flag would not be set if iint was not allocated. >>> >> Hi Dmitry >> >> sorry, I think there are two mistakes here. >> >> First, ima_file_free() is not used by the appraisal module only >> because, if a file has been written, also the IMA_MEASURED >> and IMA_AUDITED flags are cleared. Your patch prevents further >> measurements/audits if a file is modified. >> >> Second, the lock of i_mutex is necessary, as it protects the >> access to the fields of the 'integrity_iint_cache' structure. >> >> My suggestion is to provide a patch that fixes the commit a756024e >> of Mimi's 'next' branch. The patch should just replace the check >> of 'iint_initialized' with 'ima_policy_flag'. The removal of >> 'iint_initialized' could be done in a separate patch. >> >> Thanks >> >> Roberto Sassu > > Hi Roberto, > > You are right. Clearing flags slipped out from my eyes. > In such case we need to test entire flag as we do in other places. > But in such case the patch is more about remove iint_initialzed, because > S_IMA flag would not be set anyway. > I posted modified version. > Hi Dmitry thanks for the updated version. I would slightly modify the patch description by saying that your patch completes the switching to the 'ima_policy_flag' variable in the checks at the beginning of IMA functions, started with the commit a756024e. I noticed that James Morris pulled my previous patches, so also this one should be pulled after Mimi confirms that it is ok. > > This patch and other patches were posted a week ago on linux-ima-devel > mailing list. > I appreciate you would review patches when they come there so we could > spot more issues before they come to lsm mailing list. > Ok, sure. Thanks Roberto Sassu > Thanks, > Dmitry > >> >>> This patch uses lately introduced ima_policy_flag to test if appraisal >>> was enabled by policy. >>> >>> With this change 'iint_initialized' is no longer used anywhere. Indeed, >>> integrity cache is allocated with SLAB_PANIC and kernel will panic if >>> allocation fails during kernel initialization. So this variable is >>> unnecessary and thus this patch removes it. >>> >>> Changes in v2: >>> * 'iint_initialized' removal patch merged to this patch (requested >>> by Mimi) >>> >>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> >>> --- >>> security/integrity/iint.c | 3 --- >>> security/integrity/ima/ima_main.c | 2 +- >>> security/integrity/integrity.h | 3 --- >>> 3 files changed, 1 insertion(+), 7 deletions(-) >>> >>> diff --git a/security/integrity/iint.c b/security/integrity/iint.c >>> index a521edf..cc3eb4d 100644 >>> --- a/security/integrity/iint.c >>> +++ b/security/integrity/iint.c >>> @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; >>> static DEFINE_RWLOCK(integrity_iint_lock); >>> static struct kmem_cache *iint_cache __read_mostly; >>> >>> -int iint_initialized; >>> - >>> /* >>> * __integrity_iint_find - return the iint associated with an inode >>> */ >>> @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) >>> iint_cache = >>> kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), >>> 0, SLAB_PANIC, init_once); >>> - iint_initialized = 1; >>> return 0; >>> } >>> security_initcall(integrity_iintcache_init); >>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >>> index 62f59ec..87d7df7 100644 >>> --- a/security/integrity/ima/ima_main.c >>> +++ b/security/integrity/ima/ima_main.c >>> @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) >>> struct inode *inode = file_inode(file); >>> struct integrity_iint_cache *iint; >>> >>> - if (!iint_initialized || !S_ISREG(inode->i_mode)) >>> + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) >>> return; >>> >>> iint = integrity_iint_find(inode); >>> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h >>> index aafb468..f51ad65 100644 >>> --- a/security/integrity/integrity.h >>> +++ b/security/integrity/integrity.h >>> @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, >>> { >>> } >>> #endif >>> - >>> -/* set during initialization */ >>> -extern int iint_initialized; >>> >> >> ------------------------------------------------------------------------------ >> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer >> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports >> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper >> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer >> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk >> _______________________________________________ >> Linux-ima-devel mailing list >> Linux-ima-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-02 10:06 ` Roberto Sassu @ 2014-10-02 10:43 ` Dmitry Kasatkin 2014-10-02 11:42 ` Roberto Sassu 0 siblings, 1 reply; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-02 10:43 UTC (permalink / raw) To: Roberto Sassu, Dmitry Kasatkin, zohar, linux-ima-devel, linux-security-module Cc: linux-kernel On 02/10/14 13:06, Roberto Sassu wrote: > On 10/02/2014 11:30 AM, Dmitry Kasatkin wrote: >> On 02/10/14 11:26, Roberto Sassu wrote: >>> On 10/01/2014 08:43 PM, Dmitry Kasatkin wrote: >>>> ima_file_free() hook is only used by appraisal module to update hash >>>> when file was modified. When there were no integrity checks on inode, >>>> S_IMA flag is not set, integrity_iint_find() returns NULL and it >>>> quits. When inode is only measured/audited but not appraised, iint >>>> is allocated and it will cause calling ima_check_last_writer() which >>>> unnecessarily locks i_mutex. >>>> >>>> Currently ima_file_free() checks 'iint_initialized'. But it looks that >>>> it is a mistake and originally 'ima_appraise' had to be used instead. >>>> In fact usage of 'iint_initialized' is completely unnecessary because >>>> S_IMA flag would not be set if iint was not allocated. >>>> >>> Hi Dmitry >>> >>> sorry, I think there are two mistakes here. >>> >>> First, ima_file_free() is not used by the appraisal module only >>> because, if a file has been written, also the IMA_MEASURED >>> and IMA_AUDITED flags are cleared. Your patch prevents further >>> measurements/audits if a file is modified. >>> >>> Second, the lock of i_mutex is necessary, as it protects the >>> access to the fields of the 'integrity_iint_cache' structure. >>> >>> My suggestion is to provide a patch that fixes the commit a756024e >>> of Mimi's 'next' branch. The patch should just replace the check >>> of 'iint_initialized' with 'ima_policy_flag'. The removal of >>> 'iint_initialized' could be done in a separate patch. >>> >>> Thanks >>> >>> Roberto Sassu >> >> Hi Roberto, >> >> You are right. Clearing flags slipped out from my eyes. >> In such case we need to test entire flag as we do in other places. >> But in such case the patch is more about remove iint_initialzed, because >> S_IMA flag would not be set anyway. >> I posted modified version. >> > > Hi Dmitry > > thanks for the updated version. I would slightly modify the > patch description by saying that your patch completes the switching > to the 'ima_policy_flag' variable in the checks at the beginning > of IMA functions, started with the commit a756024e. > Updated in my tree.. http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/commit/?h=ima-next&id=cddb34c121434c71c69cb14069252c3c61c6ae11 Dmitry > I noticed that James Morris pulled my previous patches, so also > this one should be pulled after Mimi confirms that it is ok. > > >> >> This patch and other patches were posted a week ago on linux-ima-devel >> mailing list. >> I appreciate you would review patches when they come there so we could >> spot more issues before they come to lsm mailing list. >> > > Ok, sure. > > Thanks > > Roberto Sassu > > >> Thanks, >> Dmitry >> >>> >>>> This patch uses lately introduced ima_policy_flag to test if appraisal >>>> was enabled by policy. >>>> >>>> With this change 'iint_initialized' is no longer used anywhere. >>>> Indeed, >>>> integrity cache is allocated with SLAB_PANIC and kernel will panic if >>>> allocation fails during kernel initialization. So this variable is >>>> unnecessary and thus this patch removes it. >>>> >>>> Changes in v2: >>>> * 'iint_initialized' removal patch merged to this patch (requested >>>> by Mimi) >>>> >>>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> >>>> --- >>>> security/integrity/iint.c | 3 --- >>>> security/integrity/ima/ima_main.c | 2 +- >>>> security/integrity/integrity.h | 3 --- >>>> 3 files changed, 1 insertion(+), 7 deletions(-) >>>> >>>> diff --git a/security/integrity/iint.c b/security/integrity/iint.c >>>> index a521edf..cc3eb4d 100644 >>>> --- a/security/integrity/iint.c >>>> +++ b/security/integrity/iint.c >>>> @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; >>>> static DEFINE_RWLOCK(integrity_iint_lock); >>>> static struct kmem_cache *iint_cache __read_mostly; >>>> >>>> -int iint_initialized; >>>> - >>>> /* >>>> * __integrity_iint_find - return the iint associated with an inode >>>> */ >>>> @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) >>>> iint_cache = >>>> kmem_cache_create("iint_cache", sizeof(struct >>>> integrity_iint_cache), >>>> 0, SLAB_PANIC, init_once); >>>> - iint_initialized = 1; >>>> return 0; >>>> } >>>> security_initcall(integrity_iintcache_init); >>>> diff --git a/security/integrity/ima/ima_main.c >>>> b/security/integrity/ima/ima_main.c >>>> index 62f59ec..87d7df7 100644 >>>> --- a/security/integrity/ima/ima_main.c >>>> +++ b/security/integrity/ima/ima_main.c >>>> @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) >>>> struct inode *inode = file_inode(file); >>>> struct integrity_iint_cache *iint; >>>> >>>> - if (!iint_initialized || !S_ISREG(inode->i_mode)) >>>> + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) >>>> return; >>>> >>>> iint = integrity_iint_find(inode); >>>> diff --git a/security/integrity/integrity.h >>>> b/security/integrity/integrity.h >>>> index aafb468..f51ad65 100644 >>>> --- a/security/integrity/integrity.h >>>> +++ b/security/integrity/integrity.h >>>> @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int >>>> audit_msgno, struct inode *inode, >>>> { >>>> } >>>> #endif >>>> - >>>> -/* set during initialization */ >>>> -extern int iint_initialized; >>>> >>> >>> ------------------------------------------------------------------------------ >>> >>> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer >>> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS >>> Reports >>> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper >>> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer >>> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk >>> >>> _______________________________________________ >>> Linux-ima-devel mailing list >>> Linux-ima-devel@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel >>> >> > > -- > To unsubscribe from this list: send the line "unsubscribe > linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-02 10:43 ` Dmitry Kasatkin @ 2014-10-02 11:42 ` Roberto Sassu 2014-10-02 13:03 ` Mimi Zohar 0 siblings, 1 reply; 13+ messages in thread From: Roberto Sassu @ 2014-10-02 11:42 UTC (permalink / raw) To: Dmitry Kasatkin, Dmitry Kasatkin, zohar, linux-ima-devel, linux-security-module Cc: linux-kernel On 10/02/2014 12:43 PM, Dmitry Kasatkin wrote: > On 02/10/14 13:06, Roberto Sassu wrote: >> On 10/02/2014 11:30 AM, Dmitry Kasatkin wrote: >>> On 02/10/14 11:26, Roberto Sassu wrote: >>>> On 10/01/2014 08:43 PM, Dmitry Kasatkin wrote: >>>>> ima_file_free() hook is only used by appraisal module to update hash >>>>> when file was modified. When there were no integrity checks on inode, >>>>> S_IMA flag is not set, integrity_iint_find() returns NULL and it >>>>> quits. When inode is only measured/audited but not appraised, iint >>>>> is allocated and it will cause calling ima_check_last_writer() which >>>>> unnecessarily locks i_mutex. >>>>> >>>>> Currently ima_file_free() checks 'iint_initialized'. But it looks that >>>>> it is a mistake and originally 'ima_appraise' had to be used instead. >>>>> In fact usage of 'iint_initialized' is completely unnecessary because >>>>> S_IMA flag would not be set if iint was not allocated. >>>>> >>>> Hi Dmitry >>>> >>>> sorry, I think there are two mistakes here. >>>> >>>> First, ima_file_free() is not used by the appraisal module only >>>> because, if a file has been written, also the IMA_MEASURED >>>> and IMA_AUDITED flags are cleared. Your patch prevents further >>>> measurements/audits if a file is modified. >>>> >>>> Second, the lock of i_mutex is necessary, as it protects the >>>> access to the fields of the 'integrity_iint_cache' structure. >>>> >>>> My suggestion is to provide a patch that fixes the commit a756024e >>>> of Mimi's 'next' branch. The patch should just replace the check >>>> of 'iint_initialized' with 'ima_policy_flag'. The removal of >>>> 'iint_initialized' could be done in a separate patch. >>>> >>>> Thanks >>>> >>>> Roberto Sassu >>> >>> Hi Roberto, >>> >>> You are right. Clearing flags slipped out from my eyes. >>> In such case we need to test entire flag as we do in other places. >>> But in such case the patch is more about remove iint_initialzed, because >>> S_IMA flag would not be set anyway. >>> I posted modified version. >>> >> >> Hi Dmitry >> >> thanks for the updated version. I would slightly modify the >> patch description by saying that your patch completes the switching >> to the 'ima_policy_flag' variable in the checks at the beginning >> of IMA functions, started with the commit a756024e. >> > Updated in my tree.. > > http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/commit/?h=ima-next&id=cddb34c121434c71c69cb14069252c3c61c6ae11 > Ok, thanks. Acked-by: Roberto Sassu <roberto.sassu@polito.it> Roberto Sassu > Dmitry > >> I noticed that James Morris pulled my previous patches, so also >> this one should be pulled after Mimi confirms that it is ok. >> >> >>> >>> This patch and other patches were posted a week ago on linux-ima-devel >>> mailing list. >>> I appreciate you would review patches when they come there so we could >>> spot more issues before they come to lsm mailing list. >>> >> >> Ok, sure. >> >> Thanks >> >> Roberto Sassu >> >> >>> Thanks, >>> Dmitry >>> >>>> >>>>> This patch uses lately introduced ima_policy_flag to test if appraisal >>>>> was enabled by policy. >>>>> >>>>> With this change 'iint_initialized' is no longer used anywhere. >>>>> Indeed, >>>>> integrity cache is allocated with SLAB_PANIC and kernel will panic if >>>>> allocation fails during kernel initialization. So this variable is >>>>> unnecessary and thus this patch removes it. >>>>> >>>>> Changes in v2: >>>>> * 'iint_initialized' removal patch merged to this patch (requested >>>>> by Mimi) >>>>> >>>>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> >>>>> --- >>>>> security/integrity/iint.c | 3 --- >>>>> security/integrity/ima/ima_main.c | 2 +- >>>>> security/integrity/integrity.h | 3 --- >>>>> 3 files changed, 1 insertion(+), 7 deletions(-) >>>>> >>>>> diff --git a/security/integrity/iint.c b/security/integrity/iint.c >>>>> index a521edf..cc3eb4d 100644 >>>>> --- a/security/integrity/iint.c >>>>> +++ b/security/integrity/iint.c >>>>> @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; >>>>> static DEFINE_RWLOCK(integrity_iint_lock); >>>>> static struct kmem_cache *iint_cache __read_mostly; >>>>> >>>>> -int iint_initialized; >>>>> - >>>>> /* >>>>> * __integrity_iint_find - return the iint associated with an inode >>>>> */ >>>>> @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) >>>>> iint_cache = >>>>> kmem_cache_create("iint_cache", sizeof(struct >>>>> integrity_iint_cache), >>>>> 0, SLAB_PANIC, init_once); >>>>> - iint_initialized = 1; >>>>> return 0; >>>>> } >>>>> security_initcall(integrity_iintcache_init); >>>>> diff --git a/security/integrity/ima/ima_main.c >>>>> b/security/integrity/ima/ima_main.c >>>>> index 62f59ec..87d7df7 100644 >>>>> --- a/security/integrity/ima/ima_main.c >>>>> +++ b/security/integrity/ima/ima_main.c >>>>> @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) >>>>> struct inode *inode = file_inode(file); >>>>> struct integrity_iint_cache *iint; >>>>> >>>>> - if (!iint_initialized || !S_ISREG(inode->i_mode)) >>>>> + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) >>>>> return; >>>>> >>>>> iint = integrity_iint_find(inode); >>>>> diff --git a/security/integrity/integrity.h >>>>> b/security/integrity/integrity.h >>>>> index aafb468..f51ad65 100644 >>>>> --- a/security/integrity/integrity.h >>>>> +++ b/security/integrity/integrity.h >>>>> @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int >>>>> audit_msgno, struct inode *inode, >>>>> { >>>>> } >>>>> #endif >>>>> - >>>>> -/* set during initialization */ >>>>> -extern int iint_initialized; >>>>> >>>> >>>> ------------------------------------------------------------------------------ >>>> >>>> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer >>>> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS >>>> Reports >>>> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper >>>> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer >>>> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk >>>> >>>> _______________________________________________ >>>> Linux-ima-devel mailing list >>>> Linux-ima-devel@lists.sourceforge.net >>>> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel >>>> >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe >> linux-security-module" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-02 11:42 ` Roberto Sassu @ 2014-10-02 13:03 ` Mimi Zohar 2014-10-02 13:12 ` Dmitry Kasatkin 0 siblings, 1 reply; 13+ messages in thread From: Mimi Zohar @ 2014-10-02 13:03 UTC (permalink / raw) To: Roberto Sassu Cc: Dmitry Kasatkin, Dmitry Kasatkin, linux-ima-devel, linux-security-module, linux-kernel On Thu, 2014-10-02 at 13:42 +0200, Roberto Sassu wrote: > On 10/02/2014 12:43 PM, Dmitry Kasatkin wrote: > > On 02/10/14 13:06, Roberto Sassu wrote: > >> On 10/02/2014 11:30 AM, Dmitry Kasatkin wrote: > >>> On 02/10/14 11:26, Roberto Sassu wrote: > >>>> On 10/01/2014 08:43 PM, Dmitry Kasatkin wrote: > >>>>> ima_file_free() hook is only used by appraisal module to update hash > >>>>> when file was modified. When there were no integrity checks on inode, > >>>>> S_IMA flag is not set, integrity_iint_find() returns NULL and it > >>>>> quits. When inode is only measured/audited but not appraised, iint > >>>>> is allocated and it will cause calling ima_check_last_writer() which > >>>>> unnecessarily locks i_mutex. > >>>>> > >>>>> Currently ima_file_free() checks 'iint_initialized'. But it looks that > >>>>> it is a mistake and originally 'ima_appraise' had to be used instead. > >>>>> In fact usage of 'iint_initialized' is completely unnecessary because > >>>>> S_IMA flag would not be set if iint was not allocated. > >>>>> > >>>> Hi Dmitry > >>>> > >>>> sorry, I think there are two mistakes here. > >>>> > >>>> First, ima_file_free() is not used by the appraisal module only > >>>> because, if a file has been written, also the IMA_MEASURED > >>>> and IMA_AUDITED flags are cleared. Your patch prevents further > >>>> measurements/audits if a file is modified. > >>>> > >>>> Second, the lock of i_mutex is necessary, as it protects the > >>>> access to the fields of the 'integrity_iint_cache' structure. > >>>> > >>>> My suggestion is to provide a patch that fixes the commit a756024e > >>>> of Mimi's 'next' branch. The patch should just replace the check > >>>> of 'iint_initialized' with 'ima_policy_flag'. The removal of > >>>> 'iint_initialized' could be done in a separate patch. > >>>> > >>>> Thanks > >>>> > >>>> Roberto Sassu > >>> > >>> Hi Roberto, > >>> > >>> You are right. Clearing flags slipped out from my eyes. > >>> In such case we need to test entire flag as we do in other places. > >>> But in such case the patch is more about remove iint_initialzed, because > >>> S_IMA flag would not be set anyway. > >>> I posted modified version. > >>> > >> > >> Hi Dmitry > >> > >> thanks for the updated version. I would slightly modify the > >> patch description by saying that your patch completes the switching > >> to the 'ima_policy_flag' variable in the checks at the beginning > >> of IMA functions, started with the commit a756024e. > >> > > Updated in my tree.. > > > > http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/commit/?h=ima-next&id=cddb34c121434c71c69cb14069252c3c61c6ae11 > > > > Ok, thanks. > > Acked-by: Roberto Sassu <roberto.sassu@polito.it> > > Roberto Sassu Thanks, Dmitry, Roberto. The patch and update description looks good. Please post the updated patch inline here on the mailing list. thanks, Mimi > > > Dmitry > > > >> I noticed that James Morris pulled my previous patches, so also > >> this one should be pulled after Mimi confirms that it is ok. > >> > >> > >>> > >>> This patch and other patches were posted a week ago on linux-ima-devel > >>> mailing list. > >>> I appreciate you would review patches when they come there so we could > >>> spot more issues before they come to lsm mailing list. > >>> > >> > >> Ok, sure. > >> > >> Thanks > >> > >> Roberto Sassu > >> > >> > >>> Thanks, > >>> Dmitry > >>> > >>>> > >>>>> This patch uses lately introduced ima_policy_flag to test if appraisal > >>>>> was enabled by policy. > >>>>> > >>>>> With this change 'iint_initialized' is no longer used anywhere. > >>>>> Indeed, > >>>>> integrity cache is allocated with SLAB_PANIC and kernel will panic if > >>>>> allocation fails during kernel initialization. So this variable is > >>>>> unnecessary and thus this patch removes it. > >>>>> > >>>>> Changes in v2: > >>>>> * 'iint_initialized' removal patch merged to this patch (requested > >>>>> by Mimi) > >>>>> > >>>>> Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> > >>>>> --- > >>>>> security/integrity/iint.c | 3 --- > >>>>> security/integrity/ima/ima_main.c | 2 +- > >>>>> security/integrity/integrity.h | 3 --- > >>>>> 3 files changed, 1 insertion(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/security/integrity/iint.c b/security/integrity/iint.c > >>>>> index a521edf..cc3eb4d 100644 > >>>>> --- a/security/integrity/iint.c > >>>>> +++ b/security/integrity/iint.c > >>>>> @@ -25,8 +25,6 @@ static struct rb_root integrity_iint_tree = RB_ROOT; > >>>>> static DEFINE_RWLOCK(integrity_iint_lock); > >>>>> static struct kmem_cache *iint_cache __read_mostly; > >>>>> > >>>>> -int iint_initialized; > >>>>> - > >>>>> /* > >>>>> * __integrity_iint_find - return the iint associated with an inode > >>>>> */ > >>>>> @@ -166,7 +164,6 @@ static int __init integrity_iintcache_init(void) > >>>>> iint_cache = > >>>>> kmem_cache_create("iint_cache", sizeof(struct > >>>>> integrity_iint_cache), > >>>>> 0, SLAB_PANIC, init_once); > >>>>> - iint_initialized = 1; > >>>>> return 0; > >>>>> } > >>>>> security_initcall(integrity_iintcache_init); > >>>>> diff --git a/security/integrity/ima/ima_main.c > >>>>> b/security/integrity/ima/ima_main.c > >>>>> index 62f59ec..87d7df7 100644 > >>>>> --- a/security/integrity/ima/ima_main.c > >>>>> +++ b/security/integrity/ima/ima_main.c > >>>>> @@ -143,7 +143,7 @@ void ima_file_free(struct file *file) > >>>>> struct inode *inode = file_inode(file); > >>>>> struct integrity_iint_cache *iint; > >>>>> > >>>>> - if (!iint_initialized || !S_ISREG(inode->i_mode)) > >>>>> + if (!(ima_policy_flag & IMA_APPRAISE) || !S_ISREG(inode->i_mode)) > >>>>> return; > >>>>> > >>>>> iint = integrity_iint_find(inode); > >>>>> diff --git a/security/integrity/integrity.h > >>>>> b/security/integrity/integrity.h > >>>>> index aafb468..f51ad65 100644 > >>>>> --- a/security/integrity/integrity.h > >>>>> +++ b/security/integrity/integrity.h > >>>>> @@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int > >>>>> audit_msgno, struct inode *inode, > >>>>> { > >>>>> } > >>>>> #endif > >>>>> - > >>>>> -/* set during initialization */ > >>>>> -extern int iint_initialized; > >>>>> > >>>> > >>>> ------------------------------------------------------------------------------ > >>>> > >>>> Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer > >>>> Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS > >>>> Reports > >>>> Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper > >>>> Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer > >>>> http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk > >>>> > >>>> _______________________________________________ > >>>> Linux-ima-devel mailing list > >>>> Linux-ima-devel@lists.sourceforge.net > >>>> https://lists.sourceforge.net/lists/listinfo/linux-ima-devel > >>>> > >>> > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe > >> linux-security-module" in > >> the body of a message to majordomo@vger.kernel.org > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook 2014-10-02 13:03 ` Mimi Zohar @ 2014-10-02 13:12 ` Dmitry Kasatkin 0 siblings, 0 replies; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-02 13:12 UTC (permalink / raw) To: Mimi Zohar, Roberto Sassu Cc: Dmitry Kasatkin, linux-ima-devel, linux-security-module, linux-kernel On 02/10/14 16:03, Mimi Zohar wrote: >> Ok, thanks. >> > >> > Acked-by: Roberto Sassu <roberto.sassu@polito.it> >> > >> > Roberto Sassu > Thanks, Dmitry, Roberto. The patch and update description looks good. > Please post the updated patch inline here on the mailing list. > > thanks, > > Mimi > > Mimi, patch is the same what I posted 9:21 GMT and what Roberto acked. Patch description updated based on Roberto's and Your comments ima: check ima_policy_flag in the ima_file_free() hook This patch completes the switching to the 'ima_policy_flag' variable in the checks at the beginning of IMA functions, starting with the commit a756024e. Checking 'iint_initialized' is completely unnecessary, because S_IMA flag is unset if iint was not allocated. At the same time the integrity cache is allocated with SLAB_PANIC and the kernel will panic if the allocation fails during kernel initialization. So on a running system iint_initialized is always true and can be removed. Changes in v3: * not limiting test to IMA_APPRAISE (spotted by Roberto Sassu) Changes in v2: * 'iint_initialized' removal patch merged to this patch (requested by Mimi) Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> Acked-by: Roberto Sassu <roberto.sassu@polito.it> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 4/4] ima: use path names cache 2014-10-01 18:43 [PATCH v2 0/4] integrity: few code cleanups Dmitry Kasatkin ` (2 preceding siblings ...) 2014-10-01 18:43 ` [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook Dmitry Kasatkin @ 2014-10-01 18:43 ` Dmitry Kasatkin 3 siblings, 0 replies; 13+ messages in thread From: Dmitry Kasatkin @ 2014-10-01 18:43 UTC (permalink / raw) To: zohar, linux-ima-devel, linux-security-module Cc: linux-kernel, Dmitry Kasatkin __getname() uses slab allocation which is faster than kmalloc. Make use of it. Signed-off-by: Dmitry Kasatkin <d.kasatkin@samsung.com> --- security/integrity/ima/ima_api.c | 4 ++-- security/integrity/ima/ima_main.c | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 8688597..a99eb6d 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -325,11 +325,11 @@ const char *ima_d_path(struct path *path, char **pathbuf) { char *pathname = NULL; - *pathbuf = kmalloc(PATH_MAX, GFP_KERNEL); + *pathbuf = __getname(); if (*pathbuf) { pathname = d_absolute_path(path, *pathbuf, PATH_MAX); if (IS_ERR(pathname)) { - kfree(*pathbuf); + __putname(*pathbuf); *pathbuf = NULL; pathname = NULL; } diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 87d7df7..003d6b6 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -246,7 +246,8 @@ out_digsig: rc = -EACCES; kfree(xattr_value); out_free: - kfree(pathbuf); + if (pathbuf) + __putname(pathbuf); out: mutex_unlock(&inode->i_mutex); if ((rc && must_appraise) && (ima_appraise & IMA_APPRAISE_ENFORCE)) -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-10-02 13:12 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-10-01 18:43 [PATCH v2 0/4] integrity: few code cleanups Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 1/4] integrity: add missing '__init' keyword for integrity_init_keyring() Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 2/4] evm: skip replacing EVM signature with HMAC on read-only filesystem Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook Dmitry Kasatkin 2014-10-02 8:26 ` [Linux-ima-devel] " Roberto Sassu 2014-10-02 9:21 ` [PATCH 1/1] ima: check ima_policy_flag " Dmitry Kasatkin 2014-10-02 9:30 ` [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag " Dmitry Kasatkin 2014-10-02 10:06 ` Roberto Sassu 2014-10-02 10:43 ` Dmitry Kasatkin 2014-10-02 11:42 ` Roberto Sassu 2014-10-02 13:03 ` Mimi Zohar 2014-10-02 13:12 ` Dmitry Kasatkin 2014-10-01 18:43 ` [PATCH v2 4/4] ima: use path names cache Dmitry Kasatkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.