On Tue, 2021-03-23 at 23:01 +0900, Tetsuo Handa wrote:

On 2021/03/23 22:37, Tetsuo Handa wrote:

quoted

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.

Integrity isn't always required.  Only when something tries to use it,
does it need to be enabled.  Since both integrity and the integrity
caller are runtime dependent, it is up to the user/admin to specify
"integrity" as an "lsm=" option.

quoted

Then, why not unconditionally call integrity_iintcache_init() by
not counting on DEFINE_LSM(integrity) declaration?

Initially I also questioned making "integrity" an LSM.  Perhaps it's
time to reconsider.   For now, it makes sense to just fix the NULL
pointer dereferencing.

Mimi