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: <a19576f23e7f9d3e7c546672d1335b324bf9ca9f.camel@linux.ibm.com>
Subject: Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Roberto Sassu <roberto.sassu@huawei.com>,
        "mjg59@google.com" <mjg59@google.com>,
        John Johansen <john.johansen@canonical.com>
Cc:     "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-security-module@vger.kernel.org" 
        <linux-security-module@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Silviu Vlasceanu <Silviu.Vlasceanu@huawei.com>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Date:   Thu, 17 Sep 2020 13:47:52 -0400
In-Reply-To: <581966c47e94412ab3fd5b2ca9aacd3d@huawei.com>
References: <20200904092339.19598-1-roberto.sassu@huawei.com>
         <20200904092643.20013-3-roberto.sassu@huawei.com>
         <5bbf2169cfa38bb7a3d696e582c1de954a82d5c6.camel@linux.ibm.com>
         <581966c47e94412ab3fd5b2ca9aacd3d@huawei.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2020-09-17 at 17:36 +0000, Roberto Sassu wrote:
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -221,8 +221,15 @@ static enum integrity_status
> > evm_verify_hmac(struct dentry *dentry,
> > >  		evm_status = (rc == -ENODATA) ?
> > >  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> > >  out:
> > > -	if (iint)
> > > +	if (iint) {
> > > +		/*
> > > +		 * EVM_RESET_STATUS can be cleared only by
> > evm_verifyxattr()
> > > +		 * when EVM_ALLOW_METADATA_WRITES is set. This
> > guarantees that
> > > +		 * IMA sees the EVM_RESET_STATUS flag set before it is
> > cleared.
> > > +		 */
> > > +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > >  		iint->evm_status = evm_status;
> > 
> > True IMA is currently the only caller of evm_verifyxattr() in the
> > upstreamed kernel, but it is an exported function, which may be called
> > from elsewhere.  The previous version crossed the boundary between EVM
> > & IMA with EVM modifying the IMA flag directly.  This version assumes
> > that IMA will be the only caller.  Otherwise, I like this version.
> 
> Ok, I think it is better, as you suggested, to export a new EVM function
> that tells if evm_reset_status() will be executed in the EVM post hooks, and
> to call this function from IMA. IMA would then call ima_reset_appraise_flags()
> also depending on the result of the new EVM function.
> 
> ima_reset_appraise_flags() should be called in a post hook in IMA.
> Should I introduce it?

Yes, so any callers of evm_verifyxattr() will need to implement the
post hook as well.  As much as possible, please limit code duplication.

The last time I looked, there didn't seem to be a locking concern, but
please make sure.

thanks,

Mimi