Messages in this thread | | | Date | Thu, 02 Oct 2014 13:42:32 +0200 | From | Roberto Sassu <> | 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@polito.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@samsung.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@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 >> >
| |