Re: [PATCH] ima: Support filesystems without i_version support

Re: [PATCH] ima: Support filesystems without i_version support

[Mimi Zohar]

[Cc'ing linux-integrity@vger.kernel.org]

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.hauer@pengutronix.de>
> > > > ---
> > > >  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()?

> That is, when we don't know for sure that an inode has not changed, we
> must assume that it has changed and remeasure it. When in doubt we must
> make sure IMA is working as expected, everything else is performance
> optimization.

Agreed

thanks,

Mimi

Re: [PATCH] ima: Support filesystems without i_version support

[Sascha Hauer]

On Tue, Sep 26, 2017 at 12:07:23PM -0400, Mimi Zohar wrote:
> [Cc'ing linux-integrity@vger.kernel.org]
> 
> 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.hauer@pengutronix.de>
> > > > > ---
> > > > >  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 |