Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <3ed2004413e0ac07c7bd6f10294d6b6fac6fdbf3.camel@linux.ibm.com>
Subject: Re: [RFC PATCH 2/2] integrity: double check iint_cache was
 initialized
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
        Dmitry Vyukov <dvyukov@google.com>
Cc:     linux-integrity@vger.kernel.org, James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>,
        linux-security-module <linux-security-module@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Eric Biggers <ebiggers@kernel.org>
Date:   Tue, 23 Mar 2021 10:47:45 -0400
In-Reply-To: <8a8763a7-eeeb-3578-d50c-c15919fbe1f9@i-love.sakura.ne.jp>
References: <20210319200358.22816-1-zohar@linux.ibm.com>
         <20210319200358.22816-2-zohar@linux.ibm.com>
         <8450c80a-104a-3f36-0963-0ae8fa69e0f2@i-love.sakura.ne.jp>
         <CACT4Y+bvakfNhVs29QvbY6Z8Pw0zmAUKGWM-DD5DcPZW5ny90A@mail.gmail.com>
         <1a2245c6-3cab-7085-83d3-55b083619303@i-love.sakura.ne.jp>
         <8039976be3df9bd07374fe4f1931b8ce28b89dab.camel@linux.ibm.com>
         <cde00350-2a18-1759-d53b-2e7489b6cc0e@i-love.sakura.ne.jp>
         <8a8763a7-eeeb-3578-d50c-c15919fbe1f9@i-love.sakura.ne.jp>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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