Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752148AbaJBLm4 (ORCPT ); Thu, 2 Oct 2014 07:42:56 -0400 Received: from fm1nodo1.polito.it ([130.192.180.11]:36364 "EHLO antispam.polito.it" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751176AbaJBLmy (ORCPT ); Thu, 2 Oct 2014 07:42:54 -0400 X-ExtScanner: Niversoft's FindAttachments (free) Message-ID: <542D3A28.5000603@polito.it> Date: Thu, 02 Oct 2014 13:42:32 +0200 From: Roberto Sassu Organization: Politecnico di Torino User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Dmitry Kasatkin , 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> <542D1B4B.7040904@samsung.com> <542D2398.8000300@polito.it> <542D2C56.9040505@samsung.com> In-Reply-To: <542D2C56.9040505@samsung.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-FEAS-SYSTEM-WL: 130.192.180.41 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 >>>>> --- >>>>> 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 >> > -- 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/