[PATCH v2 0/4] integrity: few code cleanups

[PATCH v2 0/4] integrity: few code cleanups

[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

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

[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

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

[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

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

[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

[PATCH v2 4/4] ima: use path names cache

[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

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

[Roberto Sassu]

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;
>

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

[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

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

[Dmitry Kasatkin]

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
>

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

[Roberto Sassu]

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

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

[Dmitry Kasatkin]

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
>

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

[Roberto Sassu]

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

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

[Mimi Zohar]

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
> 

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

[Dmitry Kasatkin]

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>