Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751653AbaJBJav (ORCPT ); Thu, 2 Oct 2014 05:30:51 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:62215 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750919AbaJBJat (ORCPT ); Thu, 2 Oct 2014 05:30:49 -0400 X-AuditID: cbfec7f5-b7f776d000003e54-ac-542d1b47e323 Message-id: <542D1B4B.7040904@samsung.com> Date: Thu, 02 Oct 2014 12:30:51 +0300 From: Dmitry Kasatkin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.2 MIME-version: 1.0 To: Roberto Sassu , Dmitry Kasatkin , zohar@linux.vnet.ibm.com, linux-ima-devel@lists.sourceforge.net, linux-security-module@vger.kernel.org Cc: linux-kernel@vger.kernel.org Subject: Re: [Linux-ima-devel] [PATCH v2 3/4] ima: check appraisal flag in the ima_file_free() hook References: <600aeb6a5b8eca7eb381392e142ca71484717e9c.1412188590.git.d.kasatkin@samsung.com> <542D0C33.2010700@polito.it> In-reply-to: <542D0C33.2010700@polito.it> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Originating-IP: [106.122.1.121] X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrBLMWRmVeSWpSXmKPExsVy+t/xa7ru0rohBtfuSVp8WVpn8XLGPHaL y7vmsFl86HnEZvFy1zd2i08rJjE7sHnsnHWX3ePBoc0sHrsXfGbyOL2y2OPzJrkA1igum5TU nMyy1CJ9uwSujNnbpAquaVS8ermRrYHxgGIXIweHhICJxMIlKl2MnECmmMSFe+vZuhi5OIQE ljJKnL17mx3CaWSS+Np9mQXCmcUo8e/WJUaQFl4BLYkFB7vYQWwWAVWJ6TevgsXZBPQkNjT/ AIuLCkRInLy7hx2iXlDix+R7LCCbRQSOMEpstAAJMwsoSPy6t4kVxBYWSJU4ePc+1BWrGSWe 77vBBJLgFNCUuLJjCjNILzPQ/PsXtSB65SU2r3nLDGILAZ3QvXYtG8Q3ihKnJ59jnsAoPAvJ 5lkI3bOQdC9gZF7FKJpamlxQnJSea6RXnJhbXJqXrpecn7uJERIbX3cwLj1mdYhRgINRiYf3 QJNOiBBrYllxZe4hRgkOZiUR3nwJ3RAh3pTEyqrUovz4otKc1OJDjEwcnFINjJEn5H5nK/4X vn/88YHEbaGrF8pX5OS99tDPupmk66PJXS8y8U9Y7pY3PCrmB3c+bToSl7WpyKfZtPbXdbY7 ek/8EqTPx26+w7N75+1vzZL/a24d2/vqgfb79L6fDSKvtnY4VM6uXchu8v+iIEtu/pn3W29d beP/8uuITunxHxyZSxnd3W9l2ymxFGckGmoxFxUnAgDDRBtBawIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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. 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 >> --- >> 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/