linux-kernel - Re: [RFC PATCH 2/2] integrity: double check iint

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ed2004413e0ac07c7bd6f10294d6b6fac6fdbf3.camel@linux.ibm.com>
Date:   Tue, 23 Mar 2021 10:47:45 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Dmitry Vyukov <dvyukov@...gle.com>
Cc:     linux-integrity@...r.kernel.org, James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Biggers <ebiggers@...nel.org>
Subject: Re: [RFC PATCH 2/2] integrity: double check iint_cache was
 initialized

On Tue, 2021-03-23 at 23:01 +0900, Tetsuo Handa wrote:
> On 2021/03/23 22:37, Tetsuo Handa wrote:
> > On 2021/03/23 21:09, Mimi Zohar wrote:
> >> 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.
> > 
> >> 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.

> > 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