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

[Roberto Sassu <roberto.sassu@polito.it>]

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