integrity: few code cleanups

PATCH In review Dmitry Kasatkin 4 participants 13 messages Oct 1, 2014 – Oct 2, 2014

Subsystems (3)

Security (LSM) Integrity measurement architecture (IMA) Extended Verification Module (EVM)

lore

Share & tools

lore

Dmitry Kasatkin

author maintainer

2014-10-01 18:43 UTC #0 lore raw

v2 cover integrity: few code cleanups

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(-)

Signature · 1 line

1.9.1

Dmitry Kasatkin

author maintainer

2014-10-01 18:43 UTC #1 lore raw

PATCH v2 1/4 integrity: add missing '__init' keyword for integrity_init_keyring()

integrity_init_keyring() is used only from kernel '__init' functions. Add it there as well.

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 security/integrity/digsig.c    | 2 +-
 security/integrity/integrity.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

security/integrity/digsig.c +1 -1

index 8d4fbff..4f643d1 100644
@@ -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;

security/integrity/integrity.h +1 -1

index c0379d1..aafb468 100644
@@ -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,

Signature · 1 line

1.9.1

Dmitry Kasatkin

author maintainer

2014-10-01 18:44 UTC #2 lore raw

PATCH v2 2/4 evm: skip replacing EVM signature with HMAC on read-only filesystem

In reply to 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 <[email protected]>

---
 security/integrity/evm/evm_main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

security/integrity/evm/evm_main.c +8 -3

index 9685af3..b392fe6 100644
@@ -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:

Signature · 1 line

1.9.1

Dmitry Kasatkin

author maintainer

2014-10-01 18:43 UTC #3 lore raw

PATCH v2 3/4 ima: check appraisal flag in the ima_file_free() hook

In reply to 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 <[email protected]>

---
 security/integrity/iint.c         | 3 ---
 security/integrity/ima/ima_main.c | 2 +-
 security/integrity/integrity.h    | 3 ---
 3 files changed, 1 insertion(+), 7 deletions(-)

security/integrity/iint.c +0 -3

index a521edf..cc3eb4d 100644
@@ -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);

security/integrity/ima/ima_main.c +1 -1

index 62f59ec..87d7df7 100644
@@ -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);

security/integrity/integrity.h +0 -3

index aafb468..f51ad65 100644
@@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
 {
 }
 #endif
-
-/* set during initialization */
-extern int iint_initialized;

Signature · 1 line

1.9.1

Roberto Sassu

2014-10-02 08:26 UTC #4 lore raw

Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook

Quoted 7 lines · 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.

Quoted 5 lines

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

Quoted 65 lines

This patch uses lately introduced ima_policy_flag to test if appraisal was enabled by policy.

Changes in v2:
* 'iint_initialized' removal patch merged to this patch (requested by Mimi)

Signed-off-by Dmitry Kasatkin <[email protected]>

---
  security/integrity/iint.c         | 3 ---
  security/integrity/ima/ima_main.c | 2 +-
  security/integrity/integrity.h    | 3 ---
  3 files changed, 1 insertion(+), 7 deletions(-)

security/integrity/iint.c +0 -3

index a521edf..cc3eb4d 100644
@@ -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);

security/integrity/ima/ima_main.c +1 -1

index 62f59ec..87d7df7 100644
@@ -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);

security/integrity/integrity.h +0 -3

index aafb468..f51ad65 100644
@@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
  {
  }
  #endif
-
-/* set during initialization */
-extern int iint_initialized;

Dmitry Kasatkin

2014-10-02 09:22 UTC #5 lore raw

PATCH 1/1 ima: check ima_policy_flag in the ima_file_free() hook

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 <[email protected]>

---
 security/integrity/iint.c         | 3 ---
 security/integrity/ima/ima_main.c | 2 +-
 security/integrity/integrity.h    | 3 ---
 3 files changed, 1 insertion(+), 7 deletions(-)

security/integrity/iint.c +0 -3

index a521edf..cc3eb4d 100644
@@ -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);

security/integrity/ima/ima_main.c +1 -1

index 62f59ec..72faf0b 100644
@@ -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);

security/integrity/integrity.h +0 -3

index aafb468..f51ad65 100644
@@ -169,6 +169,3 @@ static inline void integrity_audit_msg(int audit_msgno, struct inode *inode,
 {
 }
 #endif
-
-/* set during initialization */
-extern int iint_initialized;

Signature · 1 line

1.9.1

Dmitry Kasatkin

2014-10-02 09:30 UTC #6 lore raw

Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook

In reply to Roberto Sassu

Quoted 25 lines · Roberto Sassu

Quoted 7 lines · 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.
Quoted 5 lines
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.

Quoted 8 lines

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

Quoted 78 lines

Quoted 60 lines
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 <[email protected]>
---
  security/integrity/iint.c         | 3 ---
  security/integrity/ima/ima_main.c | 2 +-
  security/integrity/integrity.h    | 3 ---
  3 files changed, 1 insertion(+), 7 deletions(-)
security/integrity/iint.c +0 -3
index a521edf..cc3eb4d 100644
@@ -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);
security/integrity/ima/ima_main.c +1 -1
index 62f59ec..87d7df7 100644
@@ -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);
security/integrity/integrity.h +0 -0
index aafb468..f51ad65 100644
security/integrity/integrity.h static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, · line 174
Quoted 6 lines
security/integrity/integrity.h +0 -3
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-ima-devel

Roberto Sassu

2014-10-02 10:06 UTC #7 lore raw

Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook

Quoted 35 lines · Dmitry Kasatkin

Quoted 25 lines · Roberto Sassu
Quoted 7 lines · 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.
Quoted 5 lines
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.
Quoted 8 lines
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

Quoted 8 lines

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.

Quoted 6 lines

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

Quoted 82 lines

Thanks,
Dmitry
Quoted 78 lines
Quoted 60 lines
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 <[email protected]>
---
   security/integrity/iint.c         | 3 ---
   security/integrity/ima/ima_main.c | 2 +-
   security/integrity/integrity.h    | 3 ---
   3 files changed, 1 insertion(+), 7 deletions(-)
security/integrity/iint.c +0 -3
index a521edf..cc3eb4d 100644
@@ -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);
security/integrity/ima/ima_main.c +1 -1
index 62f59ec..87d7df7 100644
@@ -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);
security/integrity/integrity.h +0 -0
index aafb468..f51ad65 100644
security/integrity/integrity.h static inline void integrity_audit_msg(int audit_msgno, struct inode *inode, · line 174
Quoted 6 lines
security/integrity/integrity.h +0 -3
@@ -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 [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-ima-devel

Dmitry Kasatkin

2014-10-02 10:43 UTC #8 lore raw

Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook

Quoted 45 lines · Roberto Sassu

Quoted 35 lines · Dmitry Kasatkin
Quoted 25 lines · Roberto Sassu
Quoted 7 lines · 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.
Quoted 5 lines
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.
Quoted 8 lines
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
Quoted 8 lines
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.

Quoted 7 lines

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

Quoted 115 lines

I noticed that James Morris pulled my previous patches, so also this one should be pulled after Mimi confirms that it is ok.
Quoted 6 lines
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
Quoted 90 lines
Thanks,
Dmitry
Quoted 86 lines
Quoted 64 lines
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 <[email protected]>
---
   security/integrity/iint.c         | 3 ---
   security/integrity/ima/ima_main.c | 2 +-
   security/integrity/integrity.h    | 3 ---
   3 files changed, 1 insertion(+), 7 deletions(-)
security/integrity/iint.c +0 -3
index a521edf..cc3eb4d 100644
@@ -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);
security/integrity/ima/ima_main.c +1 -1
index 62f59ec..87d7df7 100644
@@ -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);
security/integrity/integrity.h +0 -0
index aafb468..f51ad65 100644
security/integrity/integrity.h static inline void integrity_audit_msg(int · line 174
Quoted 7 lines
security/integrity/integrity.h +0 -3
@@ -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
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-ima-devel
Signature · 4 lines
To unsubscribe from this list: send the line "unsubscribe
linux-security-module" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Roberto Sassu

Acked-by

2014-10-02 11:42 UTC #9 lore raw

Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook

Quoted 48 lines · Dmitry Kasatkin

Quoted 47 lines · Roberto Sassu
Quoted 35 lines · Dmitry Kasatkin
Quoted 25 lines · Roberto Sassu
Quoted 7 lines · 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.
Quoted 5 lines
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.
Quoted 8 lines
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
Quoted 8 lines
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

Quoted 9 lines

Quoted 5 lines
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 <[email protected]>

Roberto Sassu

Quoted 118 lines

Dmitry
Quoted 115 lines
I noticed that James Morris pulled my previous patches, so also this one should be pulled after Mimi confirms that it is ok.
Quoted 6 lines
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
Quoted 90 lines
Thanks,
Dmitry
Quoted 86 lines
Quoted 64 lines
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 <[email protected]>
---
    security/integrity/iint.c         | 3 ---
    security/integrity/ima/ima_main.c | 2 +-
    security/integrity/integrity.h    | 3 ---
    3 files changed, 1 insertion(+), 7 deletions(-)
security/integrity/iint.c +0 -3
index a521edf..cc3eb4d 100644
@@ -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);
security/integrity/ima/ima_main.c +1 -1
index 62f59ec..87d7df7 100644
@@ -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);
security/integrity/integrity.h +0 -0
index aafb468..f51ad65 100644
security/integrity/integrity.h static inline void integrity_audit_msg(int · line 174
Quoted 7 lines
security/integrity/integrity.h +0 -3
@@ -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
[email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mimi Zohar

2014-10-02 13:03 UTC #10 lore raw

Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook

Quoted 56 lines · Roberto Sassu

Quoted 55 lines · Dmitry Kasatkin
Quoted 45 lines · Roberto Sassu
Quoted 35 lines · Dmitry Kasatkin
Quoted 25 lines · Roberto Sassu
Quoted 7 lines · 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.
Quoted 5 lines
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.
Quoted 8 lines
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
Quoted 8 lines
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.
Quoted 7 lines
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..

Quoted 8 lines

Quoted 2 lines
http://git.kernel.org/cgit/linux/kernel/git/kasatkin/linux-digsig.git/commit/?h=ima-next&id=cddb34c121434c71c69cb14069252c3c61c6ae11
Ok, thanks.
Acked-by Roberto Sassu <[email protected]>
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

Quoted 125 lines

Quoted 119 lines
Dmitry
Quoted 115 lines
I noticed that James Morris pulled my previous patches, so also this one should be pulled after Mimi confirms that it is ok.
Quoted 6 lines
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
Quoted 90 lines
Thanks,
Dmitry
Quoted 86 lines
Quoted 64 lines
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 <[email protected]>
---
    security/integrity/iint.c         | 3 ---
    security/integrity/ima/ima_main.c | 2 +-
    security/integrity/integrity.h    | 3 ---
    3 files changed, 1 insertion(+), 7 deletions(-)
security/integrity/iint.c +0 -3
index a521edf..cc3eb4d 100644
@@ -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);
security/integrity/ima/ima_main.c +1 -1
index 62f59ec..87d7df7 100644
@@ -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);
security/integrity/integrity.h +0 -0
index aafb468..f51ad65 100644
security/integrity/integrity.h static inline void integrity_audit_msg(int · line 174
Quoted 7 lines
security/integrity/integrity.h +0 -3
@@ -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
[email protected]
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 [email protected]
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dmitry Kasatkin

Acked-by Roberto Sassu

2014-10-02 13:12 UTC #11 lore raw

Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook

Quoted 4 lines · Mimi Zohar

Quoted 4 lines
Ok, thanks.
Quoted 3 lines
Acked-by Roberto Sassu <[email protected]>

Quoted 9 lines

Quoted 1 line
Quoted 1 line
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 <[email protected]>

Acked-by Roberto Sassu <[email protected]>

Dmitry Kasatkin

author maintainer

2014-10-01 18:43 UTC #12 lore raw

PATCH v2 4/4 ima: use path names cache

In reply to Dmitry Kasatkin

__getname() uses slab allocation which is faster than kmalloc. Make use of it.

Signed-off-by Dmitry Kasatkin <[email protected]>

---
 security/integrity/ima/ima_api.c  | 4 ++--
 security/integrity/ima/ima_main.c | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

security/integrity/ima/ima_api.c +2 -2

index 8688597..a99eb6d 100644
@@ -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;
 		}

security/integrity/ima/ima_main.c +2 -1

index 87d7df7..003d6b6 100644
@@ -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))

Signature · 1 line

1.9.1