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