Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change detection

[Stefan Berger]

On 2/6/24 10:54, Jeff Layton wrote:
> On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:
> > On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
> > > process_measurement() will try to detect file content changes for not-yet-
> > > copied-up files on a stacked filesystem based on the i_version number of
> > > the real inode: !inode_eq_iversion(real_inode, iint->version)
> > > Therefore, take a snapshot of the i_version of the real file to be used
> > > for i_version number-based file content change detection by IMA in
> > > process_meassurements().
> > >
> > > In this case vfs_getattr_nosec() cannot be used since it will return the
> > > i_version number of the file on the overlay layer which will trigger more
> > > iint resets in process_measurements() than necessary since this i_version
> > > number represents different state than that of the real_inode (of a
> > > not-yet-copied up file).
> > >
> > > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > > ---
> > >   security/integrity/ima/ima_api.c | 28 +++++++++++++++-------------
> > >   1 file changed, 15 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > > index 597ea0c4d72f..530888cc481e 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -14,6 +14,7 @@
> > >   #include <linux/xattr.h>
> > >   #include <linux/evm.h>
> > >   #include <linux/fsverity.h>
> > > +#include <linux/iversion.h>
> > >
> > >   #include "ima.h"
> > >
> > > @@ -250,7 +251,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > >          int result = 0;
> > >          int length;
> > >          void *tmpbuf;
> > > -       u64 i_version = 0;
> > >
> > >          /*
> > >           * Always collect the modsig, because IMA might have already collected
> > > @@ -263,16 +263,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > >          if (iint->flags & IMA_COLLECTED)
> > >                  goto out;
> > >
> > > -       /*
> > > -        * Detecting file change is based on i_version. On filesystems
> > > -        * which do not support i_version, support was originally limited
> > > -        * to an initial measurement/appraisal/audit, but was modified to
> > > -        * assume the file changed.
> > > -        */
> > > -       result = vfs_getattr_nosec(&file->f_path, &stat, STATX_CHANGE_COOKIE,
> > > -                                  AT_STATX_SYNC_AS_STAT);
> > > -       if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> > > -               i_version = stat.change_cookie;
> > >          hash.hdr.algo = algo;
> > >          hash.hdr.length = hash_digest_size[algo];
> > >
> > > @@ -302,10 +292,22 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > >
> > >          iint->ima_hash = tmpbuf;
> > >          memcpy(iint->ima_hash, &hash, length);
> > > -       iint->version = i_version;
> > > -       if (real_inode != inode) {
> > > +       if (real_inode == inode) {
> > > +               /*
> > > +                * Detecting file change is based on i_version. On filesystems
> > > +                * which do not support i_version, support was originally limited
> > > +                * to an initial measurement/appraisal/audit, but was modified to
> > > +                * assume the file changed.
> > > +                */
> > > +               result = vfs_getattr_nosec(&file->f_path, &stat,
> > > +                                          STATX_CHANGE_COOKIE,
> > > +                                          AT_STATX_SYNC_AS_STAT);
> > > +               if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> > > +                       iint->version = stat.change_cookie;
> > > +       } else {
> > >                  iint->real_ino = real_inode->i_ino;
> > >                  iint->real_dev = real_inode->i_sb->s_dev;
> > > +               iint->version = inode_query_iversion(real_inode);
>
> You only want to do this if IS_I_VERSION(inode) is true. If the
> underlying filesystem is doing its own thing wrt the i_version field,
> calling inode_query_iversion on it may corrupt it.
>
>
> > >          }
> >
> > The commit that removed inode_query_iversion db1d1e8b9867 ("IMA: use
> > vfs_getattr_nosec to get the i_version") claimed to do that because
> > inode_query_iversion() did not work in overlayfs and now this commit
> > uses inode_query_iversion() only for overlayfs.

Following this patch inode_query_version() would only be used when 
real_inode != inode, such as when a copy-up has not occurred, yet. If 
real_inode == inode then this is the case for the 'overlay' layer of 
overlayfs as well as any other non-stacked filesystem that would then 
still use vfs_getattr_nosec(). So is vfs_getattr_nosec() NOT the more 
general approach for all filesystems to use here?

> > STATX_CHANGE_COOKIE does not seem to make much sense in this
> > code anymore, unless it is still needed, according to original commit to
> > "allow IMA to work properly with a broader class of filesystems in the future."
>
> I don't have a real opinion here. When I did the original patch that
> switched this over to to use vfs_getattr_nosec, I didn't consider that
> it could end up being called from an atomic context. Reverting that

Under what conditions do we have an atomic context here? I was/am not 
aware of this.

> seems like the correct thing to do if it's still broken.
>
> If you're fine with this only working on a subset of local filesystems,
> then doing something like this is probably fine:
>
> 	if (IS_I_VERSION(real_inode))
> 		iint->version = inode_query_iversion(real_inode);
>
> ...but it's not clear to me what you should do if IS_I_VERSION is false.
> I guess IMA just falls back to checking the ctime in that case?

It does not use ctime but assumes that something has changed.