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

[Dmitry Kasatkin <d.kasatkin@samsung.com>]

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
>