Re: [PATCH v4 04/11] ima: Move ima_reset_appraise_flags() call to post hooks

[Mimi Zohar]

On Mon, 2021-05-03 at 07:41 +0000, Roberto Sassu wrote:
> 
> > > > diff --git a/security/integrity/ima/ima_appraise.c
> > > > b/security/integrity/ima/ima_appraise.c
> > > > index 565e33ff19d0..1f029e4c8d7f 100644
> > > > --- a/security/integrity/ima/ima_appraise.c
> > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > @@ -577,21 +577,40 @@ int ima_inode_setxattr(struct dentry *dentry,
> > const
> > > > char *xattr_name,
> > > >  	if (result == 1) {
> > > >  		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
> > > >  			return -EINVAL;
> > > > -		ima_reset_appraise_flags(d_backing_inode(dentry),
> > > > -			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> > > >  		result = 0;
> > > >  	}
> > > >  	return result;
> > > >  }
> > > >
> > > > +void ima_inode_post_setxattr(struct dentry *dentry, const char
> > > > *xattr_name,
> > > > +			     const void *xattr_value, size_t xattr_value_len)
> > > > +{
> > > > +	const struct evm_ima_xattr_data *xvalue = xattr_value;
> > > > +	int result;
> > > > +
> > > > +	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
> > > > +				   xattr_value_len);
> > > > +	if (result == 1)
> > > > +		ima_reset_appraise_flags(d_backing_inode(dentry),
> > >
> > > I found an issue in this patch.
> > >
> > > Moving ima_reset_appraise_flags() to the post hook causes this
> > > function to be executed also when __vfs_setxattr_noperm() is
> > > called.
> > >
> > > The problem is that at the end of a write IMA calls
> > > ima_collect_measurement() to recalculate the file digest and
> > > update security.ima. ima_collect_measurement() sets
> > > IMA_COLLECTED.
> > >
> > > However, after that __vfs_setxattr_noperm() causes
> > > IMA_COLLECTED to be reset, and to unnecessarily recalculate
> > > the file digest. This wouldn't happen if ima_reset_appraise_flags()
> > > is in the pre hook.
> > >
> > > I solved by replacing:
> > > 	iint->flags &= ~IMA_DONE_MASK;
> > > with:
> > > 	iint->flags &= ~(IMA_DONE_MASK & ~IMA_COLLECTED);
> > >
> > > just when the IMA_CHANGE_XATTR bit is set. It should
> > > not be a problem since setting an xattr does not influence
> > > the file content.
> > >
> > > Mimi, what do you think?
> > 
> > Thank yor for noticing this.
> > 
> > Without seeing the actual change it is hard to tell.   The only place
> > that "iint->flags &= ~IMA_DONE_MASK;" occurs is in neither of the above
> > functions, but in process_measurement().  There it is a part of a
> > compound "if" statement.  Perhaps it would be ok to change it for just
> > the IMA_CHANGE_XATTR test, but definitely not for the other conditions,
> > like untrusted mounts.
> 
> Ok. Should I include this change in this patch or in a separate patch?
> 	
> > Moving ima_reset_appraise_flags() to the post hooks is to minimize
> > resetting the flags unnecessarily.  That is really a performance fix,
> > not something necessary for making the EVM portable & immutable
> > signatures more usable.  As much as possible, please minimize the
> > changes to facilitate review and testing.
> 
> Ok.

I'm really sorry, but the more I'm looking at the patch set, the more
I'm realizing that a number of the changes are a result of this
"performance improvement".   Without this change, reviewing the code
would be simplified.  So yes, the patch set needs to be updated to
reflect only what is needed to support making EVM portable & immutable
signatures more usable.

thanks,

Mimi