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

[Mimi Z. <zo...@li...>]

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.)

Mimi 

> Sascha
> 
> >  
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index ac66680689d3..931773049a09 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -123,7 +123,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> >  
> >  	inode_lock(inode);
> >  	if (atomic_read(&inode->i_writecount) == 1) {
> > -		if ((iint->version != inode->i_version) ||
> > +		if (!inode->i_version || (iint->version != inode->i_version) ||
> >  		    (iint->flags & IMA_NEW_FILE)) {
> >  			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> >  			iint->measured_pcrs = 0;
> > -- 
> > 2.11.0
> > 
> > 
>