| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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