Message-ID: <754c1617c27660eff17bc0d7f4921c59d2471603.camel@kernel.org>
Date: Tue, 06 Feb 2024 10:54:49 -0500
From: Jeff Layton <jlayton@...nel.org>
To: Amir Goldstein <amir73il@...il.com>, Stefan Berger
 <stefanb@...ux.ibm.com>
Cc: linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org, 
 linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 paul@...l-moore.com,  jmorris@...ei.org, serge@...lyn.com,
 zohar@...ux.ibm.com, roberto.sassu@...wei.com,  brauner@...nel.org,
 miklos@...redi.hu
Subject: Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change
 detection

On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:
> On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@...ux.ibm.com> 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@...ux.ibm.com>
> > ---
> >  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.
> 
> 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
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?
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists