Re: [Linux-ima-devel] [PATCH] ima: Support filesystems without i_version support

[Sascha H. <s....@pe...>]

On Tue, Sep 26, 2017 at 12:07:23PM -0400, Mimi Zohar wrote:
> [Cc'ing lin...@vg...]
> 
> Hi Sascha,
> 
> Jarkko recently announced a new linux-integrity mailing list that
> we'll be using for both TPM and IMA discussions.
> 
> On Thu, 2017-09-21 at 11:44 +0200, Sascha Hauer wrote:
> > On Wed, Sep 20, 2017 at 08:06:27AM -0400, Mimi Zohar wrote:
> > > Hi Sascha,
> > > 
> > > On Wed, 2017-09-20 at 09:23 +0200, Sascha Hauer wrote:
> > > > Mimi,
> > > > 
> > > > On Wed, Sep 13, 2017 at 04:15:13PM +0200, Sascha Hauer wrote:
> > > > > IMA uses the inode's i_version field to detect changes on an inode.
> > > > > This seems to be an optimization for IMA and not strictly necessary.
> > > > > Just ignore the i_version field if it is zero and measure the file
> > > > > anyway. On filesystems which do not support i_version this may result
> > > > > in an unnecessary re-measurement of a file when it has been opened for
> > > > > writing without anything actually being written. For filesystems with
> > > > > i_version support the behaviour doesn't change.
> > > > > 
> > > > > Signed-off-by: Sascha Hauer <s....@pe...>
> > > > > ---
> > > > >  security/integrity/ima/ima_main.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > >  I'm not sure if this patch is appropriate, but even when it's not it
> > > > >  would be interesting to know why it isn't.
> > > > 
> > > > Any input to this one?
> > > 
> > > Sorry, I'm still thinking about it.  For filesystems that
> > > automatically enable i_version there would be no difference.  For
> > > filesystems that require a mount option to enable i_version, this
> > > changes the behavior.
> > > 
> > > This is slightly different than not caching the integrity results, in
> > > that the cache is only cleared if someone opens the file rw.
> > > 
> > > (Jeff Layton posted a patch that replaces the i_version checks with
> > > atime/mtime.)
> > 
> > Looking at that patch I think that using i_version only when
> > MS_I_VERSION is set is a useful change because otherwise IMA
> > ends up using i_version when it contains no sensible values.
> > 
> > I can't judge whether mtime is fine grained enough on all systems,
> > but I don't think it's necessary to use it.
> > 
> > My version of ima_should_update_iint() would look like:
> > 
> > static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> > 				struct inode *inode)
> > {
> > 	if (atomic_read(&inode->i_writecount) != 1)
> > 		return false;
> > 	if (iint->flags & IMA_NEW_FILE)
> > 		return true;
> > 	if (IS_I_VERSION(inode) && iint->version == inode->i_version)
> > 		return false;
> > 	return true;
> > }
> 
> The only reason for a new function would be to call it from multiple
> places. The other place would probably affect caching the integrity
> results.  This change you're proposing is simple enough to reason
> about.  If we decide at a later date that we need a new function,
> we'll refactor the code then.  For now, could you make this change in
> ima_check_last_writer()?

I liked the new function approach because I think it makes the code
slightly easier to read. Anyway, I just sent a new patch to the new
list.

Thanks
 Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |