lkml.org 
[lkml]   [2020]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2 07/12] evm: Introduce EVM_RESET_STATUS atomic flag
Date
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, September 17, 2020 2:01 PM
> [Cc'ing John Johansen]
>
> Hi Roberto,
>
> On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> on
> > metadata. Its main purpose is to allow users to freely set metadata when
> > they are protected by a portable signature, until the HMAC key is loaded.
> >
> > However, IMA is not notified about metadata changes and, after the first
> > successful appraisal, always allows access to the files without checking
> > metadata again.
> >
> > This patch introduces the new atomic flag EVM_RESET_STATUS in
> > integrity_iint_cache that is set in the EVM post hooks and cleared in
> > evm_verify_hmac(). IMA checks the new flag in process_measurement()
> and if
> > it is set, it clears the appraisal flags.
> >
> > Although the flag could be cleared also by evm_inode_setxattr() and
> > evm_inode_setattr() before IMA sees it, this does not happen if
> > EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> > evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> > is cleared.
> >
> > This patch also adds a call to evm_reset_status() in
> > evm_inode_post_setattr() so that EVM won't return the cached status
> the
> > next time appraisal is performed.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> > security/integrity/evm/evm_main.c | 17 +++++++++++++++--
> > security/integrity/ima/ima_main.c | 8 ++++++--
> > security/integrity/integrity.h | 1 +
> > 3 files changed, 22 insertions(+), 4 deletions(-)
> >
> > 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?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> Mimi
>
> > + }
> > kfree(xattr_data);
> > return evm_status;
> > }
> > @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
> > struct integrity_iint_cache *iint;
> >
> > iint = integrity_iint_find(inode);
> > - if (iint)
> > + if (iint) {
> > + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> > + set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > +
> > iint->evm_status = INTEGRITY_UNKNOWN;
> > + }
> > }
> >
> > /**
> > @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
> > if (!evm_key_loaded())
> > return;
> >
> > + evm_reset_status(dentry->d_inode);
> > +
> > if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> > evm_update_evmxattr(dentry, NULL, NULL, 0);
> > }

\
 
 \ /
  Last update: 2020-09-17 19:37    [W:0.086 / U:0.916 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site