Re: [RFC PATCH 2/2] integrity: double check iint_cache was initialized
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: 2021-03-23 14:02:35
Also in:
linux-security-module, lkml
Subsystem:
extended verification module (evm), integrity measurement architecture (ima), security subsystem, the rest · Maintainers:
Mimi Zohar, Roberto Sassu, Dmitry Kasatkin, Paul Moore, James Morris, "Serge E. Hallyn", Linus Torvalds
On 2021/03/23 22:37, Tetsuo Handa wrote:
On 2021/03/23 21:09, Mimi Zohar wrote:quoted
Please take a look at the newer version of this patch. Do you want to add any tags?Oh, I didn't know that you already posted the newer version.quoted
diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 1d20003243c3..0ba01847e836 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c@@ -98,6 +98,14 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) struct rb_node *node, *parent = NULL; struct integrity_iint_cache *iint, *test_iint; + /* + * The integrity's "iint_cache" is initialized at security_init(), + * unless it is not included in the ordered list of LSMs enabled + * on the boot command line. + */ + if (!iint_cache) + panic("%s: lsm=integrity required.\n", __func__); +This looks strange. If "lsm=" parameter must include "integrity", it implies that nobody is allowed to disable "integrity" at boot. Then, why not unconditionally call integrity_iintcache_init() by not counting on DEFINE_LSM(integrity) declaration?
Or, I think below one is also possible.
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 1d20003243c3..37afc5168891 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c@@ -19,6 +19,7 @@ #include <linux/uaccess.h> #include <linux/security.h> #include <linux/lsm_hooks.h> +#include <linux/sched/mm.h> #include "integrity.h" static struct rb_root integrity_iint_tree = RB_ROOT;
@@ -85,6 +86,20 @@ static void iint_free(struct integrity_iint_cache *iint) kmem_cache_free(iint_cache, iint); } +static void init_once(void *foo) +{ + struct integrity_iint_cache *iint = foo; + + memset(iint, 0, sizeof(*iint)); + iint->ima_file_status = INTEGRITY_UNKNOWN; + iint->ima_mmap_status = INTEGRITY_UNKNOWN; + iint->ima_bprm_status = INTEGRITY_UNKNOWN; + iint->ima_read_status = INTEGRITY_UNKNOWN; + iint->ima_creds_status = INTEGRITY_UNKNOWN; + iint->evm_status = INTEGRITY_UNKNOWN; + mutex_init(&iint->mutex); +} + /** * integrity_inode_get - find or allocate an iint associated with an inode * @inode: pointer to the inode
@@ -102,6 +117,18 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) if (iint) return iint; + if (!iint_cache) { + static DEFINE_MUTEX(lock); + unsigned int flags = memalloc_nofs_save(); + + mutex_lock(&lock); + if (!iint_cache) + iint_cache = kmem_cache_create("iint_cache", + sizeof(struct integrity_iint_cache), + 0, SLAB_PANIC, init_once); + mutex_unlock(&lock); + memalloc_nofs_restore(flags); + } iint = kmem_cache_alloc(iint_cache, GFP_NOFS); if (!iint) return NULL;
@@ -150,25 +177,8 @@ void integrity_inode_free(struct inode *inode) iint_free(iint); } -static void init_once(void *foo) -{ - struct integrity_iint_cache *iint = foo; - - memset(iint, 0, sizeof(*iint)); - iint->ima_file_status = INTEGRITY_UNKNOWN; - iint->ima_mmap_status = INTEGRITY_UNKNOWN; - iint->ima_bprm_status = INTEGRITY_UNKNOWN; - iint->ima_read_status = INTEGRITY_UNKNOWN; - iint->ima_creds_status = INTEGRITY_UNKNOWN; - iint->evm_status = INTEGRITY_UNKNOWN; - mutex_init(&iint->mutex); -} - static int __init integrity_iintcache_init(void) { - iint_cache = - kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), - 0, SLAB_PANIC, init_once); return 0; } DEFINE_LSM(integrity) = {