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

From: Roberto Sassu
Date: Thu Oct 02 2014 - 06:06:44 EST

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@xxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/linux-ima-devel



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/