| 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
| ||
|
Message-ID: <542D2398.8000300@polito.it>
Date: Thu, 02 Oct 2014 12:06:16 +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 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@...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-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