linux-kernel - Re: [PATCH v2 07/12] evm: Introduce EVM_RESET

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] [day] [month] [year] [list]

Message-ID: <a19576f23e7f9d3e7c546672d1335b324bf9ca9f.camel@linux.ibm.com>
Date:   Thu, 17 Sep 2020 13:47:52 -0400
From:   Mimi Zohar <zohar@...ux.ibm.com>
To:     Roberto Sassu <roberto.sassu@...wei.com>,
        "mjg59@...gle.com" <mjg59@...gle.com>,
        John Johansen <john.johansen@...onical.com>
Cc:     "linux-integrity@...r.kernel.org" <linux-integrity@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Silviu Vlasceanu <Silviu.Vlasceanu@...wei.com>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag

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