Re: [PATCH RFC] ima: Fallback to a ctime guard without i_version updates

[Frederick Lawler <fred@cloudflare.com>]

Hi Jeff,

On Tue, Jan 06, 2026 at 07:01:08AM -0500, Jeff Layton wrote:
> On Mon, 2025-12-29 at 11:52 -0600, Frederick Lawler wrote:
> > Since commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps"), IMA
> > is no longer able to correctly track inode.i_version due to the struct
> > kstat.change_cookie no longer containing an updated i_version.
> > 
> > Introduce a fallback mechanism for IMA that instead tracks a
> > integrity_ctime_guard() in absence of or outdated i_version
> > for stacked file systems.
> > 
> > EVM is left alone since it mostly cares about the backing inode.
> > 
> > Link: https://lore.kernel.org/all/aTspr4_h9IU4EyrR@CMGLRV3
> > Fixes: 1cf7e834a6fb ("xfs: switch to multigrain timestamps")
> > Suggested-by: Jeff Layton <jlayton@kernel.org>
> > Signed-off-by: Frederick Lawler <fred@cloudflare.com>
> > ---
> > The motivation behind this was that file systems that use the
> > cookie to set the i_version for stacked file systems may still do so.
> > Then add in the ctime_guard as a fallback if there's a detected change.
> > The assumption is that the ctime will be different if the i_version is
> > different anyway for non-stacked file systems.
> > 
> > I'm not too pleased with passing in struct file* to
> > integrity_inode_attrs_changed() since EVM doesn't currently use
> > that for now, but I couldn't come up with another idea to get the
> > stat without coming up with a new stat function to accommodate just
> > the file path, fully separate out IMA/EVM checks, or lastly add stacked
> > file system support to EVM (which doesn't make much sense to me
> > at the moment).
> > 
> > I plan on adding in self test infrastructure for the v1, but I would
> > like to get some early feedback on the approach first.
> > ---
> >  include/linux/integrity.h           | 29 ++++++++++++++++++++++++-----
> >  security/integrity/evm/evm_crypto.c |  2 +-
> >  security/integrity/evm/evm_main.c   |  2 +-
> >  security/integrity/ima/ima_api.c    | 21 +++++++++++++++------
> >  security/integrity/ima/ima_main.c   | 17 ++++++++++-------
> >  5 files changed, 51 insertions(+), 20 deletions(-)
> > 
> > diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> > index f5842372359be5341b6870a43b92e695e8fc78af..4964c0f2bbda0ca450d135b9b738bc92256c375a 100644
> > --- a/include/linux/integrity.h
> > +++ b/include/linux/integrity.h
> > @@ -31,19 +31,27 @@ static inline void integrity_load_keys(void)
> >  
> >  /* An inode's attributes for detection of changes */
> >  struct integrity_inode_attributes {
> > +	u64 ctime_guard;
> >  	u64 version;		/* track inode changes */
> >  	unsigned long ino;
> >  	dev_t dev;
> >  };
> >  
> > +static inline u64 integrity_ctime_guard(struct kstat stat)
> > +{
> > +	return stat.ctime.tv_sec ^ stat.ctime.tv_nsec;
> > +}
> > +
> >  /*
> >   * On stacked filesystems the i_version alone is not enough to detect file data
> >   * or metadata change. Additional metadata is required.
> >   */
> >  static inline void
> >  integrity_inode_attrs_store(struct integrity_inode_attributes *attrs,
> > -			    u64 i_version, const struct inode *inode)
> > +			    u64 i_version, u64 ctime_guard,
> > +			    const struct inode *inode)
> >  {
> > +	attrs->ctime_guard = ctime_guard;
> >  	attrs->version = i_version;
> >  	attrs->dev = inode->i_sb->s_dev;
> >  	attrs->ino = inode->i_ino;
> > @@ -54,11 +62,22 @@ integrity_inode_attrs_store(struct integrity_inode_attributes *attrs,
> >   */
> >  static inline bool
> >  integrity_inode_attrs_changed(const struct integrity_inode_attributes *attrs,
> > -			      const struct inode *inode)
> > +			      struct file *file, struct inode *inode)
> >  {
> > -	return (inode->i_sb->s_dev != attrs->dev ||
> > -		inode->i_ino != attrs->ino ||
> > -		!inode_eq_iversion(inode, attrs->version));
> > +	struct kstat stat;
> > +
> > +	if (inode->i_sb->s_dev != attrs->dev ||
> > +	    inode->i_ino != attrs->ino)
> > +		return true;
> > +
> > +	if (inode_eq_iversion(inode, attrs->version))
> > +		return false;
> > +
> > +	if (!file || vfs_getattr_nosec(&file->f_path, &stat, STATX_CTIME,
> > +				       AT_STATX_SYNC_AS_STAT))
> > +		return true;
> > +
> 
> This is rather odd. You're sampling the i_version field directly, but
> if it's not equal then you go through ->getattr() to get the ctime.
> 
> It's particularly odd since you don't know whether the i_version field
> is even implemented on the fs. On filesystems where it isn't, the
> i_version field generally stays at 0, so won't this never fall through
> to do the vfs_getattr_nosec() call on those filesystems?
>

You're totally right. I didn't consider FS's caching the value at zero.

> Ideally, you should just call vfs_getattr_nosec() early on with
> STATX_CHANGE_COOKIE|STATX_CTIME to get both at once, and only trust
> STATX_CHANGE_COOKIE if it's set in the returned mask.
> 

Yes, that makes sense.

I'll spin that in v1, thanks!

> > +	return attrs->ctime_guard != integrity_ctime_guard(stat);
> >  }
> >  
> >  
> > diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> > index a5e730ffda57fbc0a91124adaa77b946a12d08b4..2d89c0e8d9360253f8dad52d2a8168127bb4d3b8 100644
> > --- a/security/integrity/evm/evm_crypto.c
> > +++ b/security/integrity/evm/evm_crypto.c
> > @@ -300,7 +300,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> >  		if (IS_I_VERSION(inode))
> >  			i_version = inode_query_iversion(inode);
> >  		integrity_inode_attrs_store(&iint->metadata_inode, i_version,
> > -					    inode);
> > +					    0, inode);
> >  	}
> >  
> >  	/* Portable EVM signatures must include an IMA hash */
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 73d500a375cb37a54f295b0e1e93fd6e5d9ecddc..0712802628fd6533383f9855687e19bef7b771c7 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -754,7 +754,7 @@ bool evm_metadata_changed(struct inode *inode, struct inode *metadata_inode)
> >  	if (iint) {
> >  		ret = (!IS_I_VERSION(metadata_inode) ||
> >  		       integrity_inode_attrs_changed(&iint->metadata_inode,
> > -						     metadata_inode));
> > +			       NULL, metadata_inode));
> >  		if (ret)
> >  			iint->evm_status = INTEGRITY_UNKNOWN;
> >  	}
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index c35ea613c9f8d404ba4886e3b736c3bab29d1668..72bba8daa588a0f4e45e4249276edb54ca3d77ef 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -254,6 +254,7 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> >  	int length;
> >  	void *tmpbuf;
> >  	u64 i_version = 0;
> > +	u64 ctime_guard = 0;
> >  
> >  	/*
> >  	 * Always collect the modsig, because IMA might have already collected
> > @@ -272,10 +273,16 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> >  	 * 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,
> > +	result = vfs_getattr_nosec(&file->f_path, &stat,
> > +				   STATX_CHANGE_COOKIE | STATX_CTIME,
> >  				   AT_STATX_SYNC_AS_STAT);
> > -	if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
> > -		i_version = stat.change_cookie;
> > +	if (!result) {
> > +		if (stat.result_mask & STATX_CHANGE_COOKIE)
> > +			i_version = stat.change_cookie;
> > +
> > +		if (stat.result_mask & STATX_CTIME)
> > +			ctime_guard = integrity_ctime_guard(stat);
> > +	}
> >  	hash.hdr.algo = algo;
> >  	hash.hdr.length = hash_digest_size[algo];
> >  
> > @@ -305,11 +312,13 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
> >  
> >  	iint->ima_hash = tmpbuf;
> >  	memcpy(iint->ima_hash, &hash, length);
> > -	if (real_inode == inode)
> > +	if (real_inode == inode) {
> >  		iint->real_inode.version = i_version;
> > -	else
> > +		iint->real_inode.ctime_guard = ctime_guard;
> > +	} else {
> >  		integrity_inode_attrs_store(&iint->real_inode, i_version,
> > -					    real_inode);
> > +				ctime_guard, real_inode);
> > +	}
> >  
> >  	/* Possibly temporary failure due to type of read (eg. O_DIRECT) */
> >  	if (!result)
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 5770cf691912aa912fc65280c59f5baac35dd725..6051ea4a472fc0b0dd7b4e81da36eff8bd048c62 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/mount.h>
> >  #include <linux/mman.h>
> >  #include <linux/slab.h>
> > +#include <linux/stat.h>
> >  #include <linux/xattr.h>
> >  #include <linux/ima.h>
> >  #include <linux/fs.h>
> > @@ -185,6 +186,7 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
> >  {
> >  	fmode_t mode = file->f_mode;
> >  	bool update;
> > +	int ret;
> >  
> >  	if (!(mode & FMODE_WRITE))
> >  		return;
> > @@ -197,12 +199,13 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
> >  
> >  		update = test_and_clear_bit(IMA_UPDATE_XATTR,
> >  					    &iint->atomic_flags);
> > -		if ((iint->flags & IMA_NEW_FILE) ||
> > -		    vfs_getattr_nosec(&file->f_path, &stat,
> > -				      STATX_CHANGE_COOKIE,
> > -				      AT_STATX_SYNC_AS_STAT) ||
> > -		    !(stat.result_mask & STATX_CHANGE_COOKIE) ||
> > -		    stat.change_cookie != iint->real_inode.version) {
> > +		ret = vfs_getattr_nosec(&file->f_path, &stat,
> > +					STATX_CHANGE_COOKIE | STATX_CTIME,
> > +					AT_STATX_SYNC_AS_STAT);
> > +		if ((iint->flags & IMA_NEW_FILE) || ret ||
> > +		    (!ret && stat.change_cookie != iint->real_inode.version) ||
> > +		    (!ret && integrity_ctime_guard(stat) !=
> > +		     iint->real_inode.ctime_guard)) {
> >  			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> >  			iint->measured_pcrs = 0;
> >  			if (update)
> > @@ -330,7 +333,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
> >  	    (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
> >  		if (!IS_I_VERSION(real_inode) ||
> >  		    integrity_inode_attrs_changed(&iint->real_inode,
> > -						  real_inode)) {
> > +						  file, real_inode)) {
> >  			iint->flags &= ~IMA_DONE_MASK;
> >  			iint->measured_pcrs = 0;
> >  		}
> > 
> > ---
> > base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
> > change-id: 20251212-xfs-ima-fixup-931780a62c2c
> > 
> > Best regards,
> 
> -- 
> Jeff Layton <jlayton@kernel.org>