Message-ID: <542D3A28.5000603@polito.it>
Date:	Thu, 02 Oct 2014 13:42:32 +0200
From:	Roberto Sassu <roberto.sassu@...ito.it>
To:	Dmitry Kasatkin <d.kasatkin@...sung.com>,
	Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
	zohar@...ux.vnet.ibm.com, linux-ima-devel@...ts.sourceforge.net,
	linux-security-module@...r.kernel.org
CC:	linux-kernel@...r.kernel.org
Subject: Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in
 the ima_file_free() hook

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@...ito.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@...sung.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@...ts.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@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

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

Powered by blists - more mailing lists