ima: Fallback to a ctime guard without i_version updates

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

Posted by Frederick Lawler 1 month, 1 week ago

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;
+
+	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,
-- 
Frederick Lawler <fred@cloudflare.com>

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

Posted by Jeff Layton 1 month ago

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?

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.

> +	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>

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

Posted by Frederick Lawler 1 month ago

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>

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

Posted by Frederick Lawler 1 month ago

On Tue, Jan 06, 2026 at 10:43:01AM -0600, Frederick Lawler wrote:
> 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.

Actually, I'm going to amend this. I think I did consider FSs without an
implementation. Where this is called at, it is often guarded by a
!IS_I_VERSION() || integrity_inode_attrs_change(). If I'm
understanding this correctly, the check call doesn't occur unless the inode
has i_version support.

It seems to me the suggestion then is to remove the IS_I_VERSION()
checks guarding the call sites, grab both ctime and cookie from stat,
and if IS_I_VERSION() use that, otherwise cookie, and compare
against the cached i_version with one of those values, and then fall
back to ctime?

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

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

Posted by Jeff Layton 1 month ago

On Tue, 2026-01-06 at 13:33 -0600, Frederick Lawler wrote:
> On Tue, Jan 06, 2026 at 10:43:01AM -0600, Frederick Lawler wrote:
> > 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.
> 
> Actually, I'm going to amend this. I think I did consider FSs without an
> implementation. Where this is called at, it is often guarded by a
> !IS_I_VERSION() || integrity_inode_attrs_change(). If I'm
> understanding this correctly, the check call doesn't occur unless the inode
> has i_version support.
> 


It depends on what you mean by i_version support:

That flag just tells the VFS that it needs to bump the i_version field
when updating timestamps. It's not a reliable indicator of whether the
i_version field is suitable for the purpose you want here.

The problem here and the one that we ultimately fixed with multigrain
timestamps is that XFS in particular will bump i_version on any change
to the log. That includes atime updates due to reads.

XFS still tracks the i_version the way it always has, but we've stopped
getattr() from reporting it because it's not suitable for the purpose
that nfsd (and IMA) need it for.

> It seems to me the suggestion then is to remove the IS_I_VERSION()
> checks guarding the call sites, grab both ctime and cookie from stat,
> and if IS_I_VERSION() use that, otherwise cookie, and compare
> against the cached i_version with one of those values, and then fall
> back to ctime?
> 

Not exactly.

You want to call getattr() for STATX_CHANGE_COOKIE|STATX_CTIME, and
then check the kstat->result_mask. If STATX_CHANGE_COOKIE is set, then
use that. If it's not then use the ctime.

The part I'm not sure about is whether it's actually safe to do this.
vfs_getattr_nosec() can block in some situations. Is it ok to do this
in any context where integrity_inode_attrs_changed() may be called? 

ISTR that this was an issue at one point, but maybe isn't now that IMA
is an LSM?

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

-- 
Jeff Layton <jlayton@kernel.org>

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

Posted by Mimi Zohar 3 weeks, 6 days ago

On Tue, 2026-01-06 at 14:50 -0500, Jeff Layton wrote:
> > > > > @@ -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.
> > 
> > Actually, I'm going to amend this. I think I did consider FSs without an
> > implementation. Where this is called at, it is often guarded by a
> > !IS_I_VERSION() || integrity_inode_attrs_change(). If I'm
> > understanding this correctly, the check call doesn't occur unless the inode
> > has i_version support.
> > 
> 
> 
> It depends on what you mean by i_version support:
> 
> That flag just tells the VFS that it needs to bump the i_version field
> when updating timestamps. It's not a reliable indicator of whether the
> i_version field is suitable for the purpose you want here.
> 
> The problem here and the one that we ultimately fixed with multigrain
> timestamps is that XFS in particular will bump i_version on any change
> to the log. That includes atime updates due to reads.
> 
> XFS still tracks the i_version the way it always has, but we've stopped
> getattr() from reporting it because it's not suitable for the purpose
> that nfsd (and IMA) need it for.
> 
> > It seems to me the suggestion then is to remove the IS_I_VERSION()
> > checks guarding the call sites, grab both ctime and cookie from stat,
> > and if IS_I_VERSION() use that, otherwise cookie, and compare
> > against the cached i_version with one of those values, and then fall
> > back to ctime?
> > 
> 
> Not exactly.
> 
> You want to call getattr() for STATX_CHANGE_COOKIE|STATX_CTIME, and
> then check the kstat->result_mask. If STATX_CHANGE_COOKIE is set, then
> use that. If it's not then use the ctime.
> 
> The part I'm not sure about is whether it's actually safe to do this.
> vfs_getattr_nosec() can block in some situations. Is it ok to do this
> in any context where integrity_inode_attrs_changed() may be called? 

Frederick, before making any changes, please describe the problem you're
actually seeing. From my limited testing, file change IS being detected. A major
change like Jeff is suggesting is not something that would or should be back
ported.  Remember, Jeff's interest is remote filesystems, not necessarily with
your particular XFS concern.

So again, what is the problem you're trying to address?

Mimi

> 
> ISTR that this was an issue at one point, but maybe isn't now that IMA
> is an LSM?

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

Posted by Frederick Lawler 3 weeks, 6 days ago

On Mon, Jan 12, 2026 at 09:02:02AM -0500, Mimi Zohar wrote:
> On Tue, 2026-01-06 at 14:50 -0500, Jeff Layton wrote:
> > > > > > @@ -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.
> > > 
> > > Actually, I'm going to amend this. I think I did consider FSs without an
> > > implementation. Where this is called at, it is often guarded by a
> > > !IS_I_VERSION() || integrity_inode_attrs_change(). If I'm
> > > understanding this correctly, the check call doesn't occur unless the inode
> > > has i_version support.
> > > 
> > 
> > 
> > It depends on what you mean by i_version support:
> > 
> > That flag just tells the VFS that it needs to bump the i_version field
> > when updating timestamps. It's not a reliable indicator of whether the
> > i_version field is suitable for the purpose you want here.
> > 
> > The problem here and the one that we ultimately fixed with multigrain
> > timestamps is that XFS in particular will bump i_version on any change
> > to the log. That includes atime updates due to reads.
> > 
> > XFS still tracks the i_version the way it always has, but we've stopped
> > getattr() from reporting it because it's not suitable for the purpose
> > that nfsd (and IMA) need it for.
> > 
> > > It seems to me the suggestion then is to remove the IS_I_VERSION()
> > > checks guarding the call sites, grab both ctime and cookie from stat,
> > > and if IS_I_VERSION() use that, otherwise cookie, and compare
> > > against the cached i_version with one of those values, and then fall
> > > back to ctime?
> > > 
> > 
> > Not exactly.
> > 
> > You want to call getattr() for STATX_CHANGE_COOKIE|STATX_CTIME, and
> > then check the kstat->result_mask. If STATX_CHANGE_COOKIE is set, then
> > use that. If it's not then use the ctime.
> > 
> > The part I'm not sure about is whether it's actually safe to do this.
> > vfs_getattr_nosec() can block in some situations. Is it ok to do this
> > in any context where integrity_inode_attrs_changed() may be called? 
> 
> Frederick, before making any changes, please describe the problem you're
> actually seeing. From my limited testing, file change IS being detected. A major
> change like Jeff is suggesting is not something that would or should be back
> ported.  Remember, Jeff's interest is remote filesystems, not necessarily with
> your particular XFS concern.
> 
> So again, what is the problem you're trying to address?

It's easier if I paste a simpler version of test I've been promising
for v1 to help show this (below).

In 6.12 the test snippet passes, for >= 6.13 we get an audit
evaluation on the each execution when there should only be 1.

The struct integrity_inode_attributes.version stays at zero for XFS
in the below test, as well as file systems that calls into
generic_fillattr() or otherwise that doesn't set the change cookie
request mask.

When file systems have a mutated file, the cookie is then updated,
but the compare against inode.i_version could be out of date depending
on file system implementation. Thus we see since 6.13, XFS an atime change
will cause another evaluation due to stale cache.

I'm not expecting a backport to 6.13 as there has been a lot of changes
in IMA/EVM, but I think to the 6.18 LTS is reasonable. With leaving
EVM alone, it's a small diff.

I have a updated patch that hopefully addresses all your concerns
from other responses in this thread. I want to point out that the updated
code is more EVM/IMA invariant mindful than this RFC. I'd
like to submit that, and then move discussion over there if possible.

Hopefully this helps,
Fred

_fragment.config_
CONFIG_XFS_FS=y
CONFIG_OVERLAY_FS=y
CONFIG_IMA=y
CONFIG_IMA_WRITE_POLICY=y
CONFIG_IMA_READ_POLICY=y

_./test.sh_
#!/bin/bash -e

IMA_POLICY="/sys/kernel/security/ima/policy"
TEST_BIN="/bin/date"
MNT_BASE="/tmp/ima_test_root"

mkdir -p "$MNT_BASE"
mount -t tmpfs tmpfs "$MNT_BASE"
mkdir -p "$MNT_BASE"/{xfs_disk,upper,work,ovl}

dd if=/dev/zero of="$MNT_BASE/xfs.img" bs=1M count=300
mkfs.xfs -q "$MNT_BASE/xfs.img"
mount "$MNT_BASE/xfs.img" "$MNT_BASE/xfs_disk"
cp "$TEST_BIN" "$MNT_BASE/xfs_disk/test_prog"

mount -t overlay overlay -o \
"lowerdir=$MNT_BASE/xfs_disk,upperdir=$MNT_BASE/upper,workdir=$MNT_BASE/work" \
"$MNT_BASE/ovl"

echo "audit func=BPRM_CHECK uid=$(id -u nobody)" > "$IMA_POLICY"

target_prog="$MNT_BASE/ovl/test_prog"
setpriv --reuid nobody "$target_prog"
setpriv --reuid nobody "$target_prog"
setpriv --reuid nobody "$target_prog"

audit_count=$(dmesg | grep -c "file=\"$target_prog\"")

if [[ "$audit_count" -eq 1 ]]; then
	echo "PASS: Found exactly 1 audit event."
else
	echo "FAIL: Expected 1 audit event, but found $audit_count."
	exit 1
fi

> 
> Mimi
> 
> > 
> > ISTR that this was an issue at one point, but maybe isn't now that IMA
> > is an LSM?
>

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

Posted by Frederick Lawler 1 month ago

On Tue, Jan 06, 2026 at 02:50:31PM -0500, Jeff Layton wrote:
> On Tue, 2026-01-06 at 13:33 -0600, Frederick Lawler wrote:
> > On Tue, Jan 06, 2026 at 10:43:01AM -0600, Frederick Lawler wrote:
> > > 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.
> > 
> > Actually, I'm going to amend this. I think I did consider FSs without an
> > implementation. Where this is called at, it is often guarded by a
> > !IS_I_VERSION() || integrity_inode_attrs_change(). If I'm
> > understanding this correctly, the check call doesn't occur unless the inode
> > has i_version support.
> > 
> 
> 
> It depends on what you mean by i_version support:
> 
> That flag just tells the VFS that it needs to bump the i_version field
> when updating timestamps. It's not a reliable indicator of whether the
> i_version field is suitable for the purpose you want here.
>

So, it would make sense then to also remove those guards at the callsite
then if the intention is to compare against the cookie & ctime regardless?

> The problem here and the one that we ultimately fixed with multigrain
> timestamps is that XFS in particular will bump i_version on any change
> to the log. That includes atime updates due to reads.
> 
> XFS still tracks the i_version the way it always has, but we've stopped
> getattr() from reporting it because it's not suitable for the purpose
> that nfsd (and IMA) need it for.
> 
> > It seems to me the suggestion then is to remove the IS_I_VERSION()
> > checks guarding the call sites, grab both ctime and cookie from stat,
> > and if IS_I_VERSION() use that, otherwise cookie, and compare
> > against the cached i_version with one of those values, and then fall
> > back to ctime?
> > 
> 
> Not exactly.
> 
> You want to call getattr() for STATX_CHANGE_COOKIE|STATX_CTIME, and
> then check the kstat->result_mask. If STATX_CHANGE_COOKIE is set, then
> use that. If it's not then use the ctime.

Ok, I think I understand. To reiterate my understanding, ignore calling
into inode_eq_iversion() all together.

	return other_checks || ((mask & cookie) ? cache->i_version == cookie_val :
	compare_ctime())

> 
> The part I'm not sure about is whether it's actually safe to do this.
> vfs_getattr_nosec() can block in some situations. Is it ok to do this
> in any context where integrity_inode_attrs_changed() may be called? 
> 
> ISTR that this was an issue at one point, but maybe isn't now that IMA
> is an LSM?

Poking around, callers to integrity_inode_attrs_changed() are currently behind
mutex_lock(&iinit->mutex) (similar to vfs_getattr_nosec() calls), and currently
only called from process_measurement().

While I can't say for certain this will always be the case for future use
use cases, would it be helpful to include a may_sleep() in
integrity_inode_attrs_changed() to drive this point and make a comment?

> 
> > > 
> > > > 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>
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

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

Posted by Jeff Layton 1 month ago

On Wed, 2026-01-07 at 13:56 -0600, Frederick Lawler wrote:
> On Tue, Jan 06, 2026 at 02:50:31PM -0500, Jeff Layton wrote:
> > On Tue, 2026-01-06 at 13:33 -0600, Frederick Lawler wrote:
> > > On Tue, Jan 06, 2026 at 10:43:01AM -0600, Frederick Lawler wrote:
> > > > 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.
> > > 
> > > Actually, I'm going to amend this. I think I did consider FSs without an
> > > implementation. Where this is called at, it is often guarded by a
> > > !IS_I_VERSION() || integrity_inode_attrs_change(). If I'm
> > > understanding this correctly, the check call doesn't occur unless the inode
> > > has i_version support.
> > > 
> > 
> > 
> > It depends on what you mean by i_version support:
> > 
> > That flag just tells the VFS that it needs to bump the i_version field
> > when updating timestamps. It's not a reliable indicator of whether the
> > i_version field is suitable for the purpose you want here.
> > 
> 
> So, it would make sense then to also remove those guards at the callsite
> then if the intention is to compare against the cookie & ctime regardless?
> 

Yes, I'd say so. IS_I_VERSION() is not a reliable indicator of whether
the filesystem can provide a proper change attribute. For instance,
cephfs and NFS don't set that flag, but they can (sometimes) provide
STATX_CHANGE_COOKIE. Going through getattr() is a much more reliable
method.

> > The problem here and the one that we ultimately fixed with multigrain
> > timestamps is that XFS in particular will bump i_version on any change
> > to the log. That includes atime updates due to reads.
> > 
> > XFS still tracks the i_version the way it always has, but we've stopped
> > getattr() from reporting it because it's not suitable for the purpose
> > that nfsd (and IMA) need it for.
> > 
> > > It seems to me the suggestion then is to remove the IS_I_VERSION()
> > > checks guarding the call sites, grab both ctime and cookie from stat,
> > > and if IS_I_VERSION() use that, otherwise cookie, and compare
> > > against the cached i_version with one of those values, and then fall
> > > back to ctime?
> > > 
> > 
> > Not exactly.
> > 
> > You want to call getattr() for STATX_CHANGE_COOKIE|STATX_CTIME, and
> > then check the kstat->result_mask. If STATX_CHANGE_COOKIE is set, then
> > use that. If it's not then use the ctime.
> 
> Ok, I think I understand. To reiterate my understanding, ignore calling
> into inode_eq_iversion() all together.
> 
> 	return other_checks || ((mask & cookie) ? cache->i_version == cookie_val :
> 	compare_ctime())
> 

Yes, that psuedocode looks about right. IS_I_VERSION(),
inode_eq_iversion(), etc. are really filesystem-internal APIs and not
something IMA should be relying on as a fstype-agnostic LSM.


> > 
> > The part I'm not sure about is whether it's actually safe to do this.
> > vfs_getattr_nosec() can block in some situations. Is it ok to do this
> > in any context where integrity_inode_attrs_changed() may be called? 
> > 
> > ISTR that this was an issue at one point, but maybe isn't now that IMA
> > is an LSM?
> 
> Poking around, callers to integrity_inode_attrs_changed() are currently behind
> mutex_lock(&iinit->mutex) (similar to vfs_getattr_nosec() calls), and currently
> only called from process_measurement().
> 
> While I can't say for certain this will always be the case for future use
> use cases, would it be helpful to include a may_sleep() in
> integrity_inode_attrs_changed() to drive this point and make a comment?
> 

Those kind of annotations do tend to be useful.

> 
-- 
Jeff Layton <jlayton@kernel.org>

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

Posted by Mimi Zohar 1 month ago

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.

Thanks, Frederick.

Instead of using the new function name integrity_ctime_guard() to describe the
change, please describe the change in words.  Perhaps something like: rely on
the inode's ctime to detect a file data or metadata change.

The purpose of generating a ctime guard value, as opposed to using the tv_sec
and tv_nsec, I assume is to minimize the amount of memory being saved in the
iint.

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

Agreed. This patch inverts the i_version test to return immediately if the
i_version hasn't changed and then checks the ctime guard value.  Is the ctime
guard value test simply a performance improvement?

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

I really appreciate your adding a self test.

thanks,

Mimi

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

Posted by Frederick Lawler 1 month ago

Hi Mimi,

On Mon, Jan 05, 2026 at 05:15:15PM -0500, Mimi Zohar 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.
> 
> Thanks, Frederick.
> 
> Instead of using the new function name integrity_ctime_guard() to describe the
> change, please describe the change in words.  Perhaps something like: rely on
> the inode's ctime to detect a file data or metadata change.
> 

Sure thing, I'll change for the v1.

> The purpose of generating a ctime guard value, as opposed to using the tv_sec
> and tv_nsec, I assume is to minimize the amount of memory being saved in the
> iint.

This was Jeff's suggestion. It does serve the purpose on saving
some memory, but also has some value in the event nsec or sec is zero'd.
It just needs to be different enough from the last cache'd evaluation.

> 
> > 
> > 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.
> 
> Agreed. This patch inverts the i_version test to return immediately if the
> i_version hasn't changed and then checks the ctime guard value.  Is the ctime
> guard value test simply a performance improvement?
> 

Kinda. The thought was to make already-audited files that have an
unchanged i_version since last audit succeed early.

Stacking tempfs on XFS for instance, would incur the stat cost each
evaluation, since the cookie that used to set the i_version on
evaluation with XFS on 6.12, is now always setting it to zero since 6.13.

> > 
> > 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.
> 
> I really appreciate your adding a self test.
>

I was poking around last week at some testing platforms, and instead of
adding to kernel sefltests & setup a VM etc..., to instead add to 
Linux Test Project (LTP) if that's fine?

In any case, I can add my test snippet to v1, so you have something
to review with the patch. That snippet works the same on 6.12 as it
does on 6.19 with this patch.

> thanks,
> 
> Mimi

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

Posted by Mimi Zohar 1 month ago

On Mon, 2026-01-05 at 17:09 -0600, Frederick Lawler wrote:
> Hi Mimi,
> 
> On Mon, Jan 05, 2026 at 05:15:15PM -0500, Mimi Zohar 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.
> > 
> > Thanks, Frederick.
> > 
> > Instead of using the new function name integrity_ctime_guard() to describe the
> > change, please describe the change in words.  Perhaps something like: rely on
> > the inode's ctime to detect a file data or metadata change.
> > 
> 
> Sure thing, I'll change for the v1.
> 
> > The purpose of generating a ctime guard value, as opposed to using the tv_sec
> > and tv_nsec, I assume is to minimize the amount of memory being saved in the
> > iint.
> 
> This was Jeff's suggestion. It does serve the purpose on saving
> some memory, but also has some value in the event nsec or sec is zero'd.
> It just needs to be different enough from the last cache'd evaluation.

All of this needs to be described in the patch description and, probably, a
comment before the function.
> 
> > 
> > > 
> > > 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.
> > 
> > Agreed. This patch inverts the i_version test to return immediately if the
> > i_version hasn't changed and then checks the ctime guard value.  Is the ctime
> > guard value test simply a performance improvement?
> > 
> 
> Kinda. The thought was to make already-audited files that have an
> unchanged i_version since last audit succeed early.
> 
> Stacking tempfs on XFS for instance, would incur the stat cost each
> evaluation, since the cookie that used to set the i_version on
> evaluation with XFS on 6.12, is now always setting it to zero since 6.13.

So without this patch, there aren't any missing measurements, verifications, or
audit records.  Please clarify in the patch description that this is a
performance improvement.

> 
> > > 
> > > 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.
> > 
> > I really appreciate your adding a self test.
> > 
> 
> I was poking around last week at some testing platforms, and instead of
> adding to kernel sefltests & setup a VM etc..., to instead add to 
> Linux Test Project (LTP) if that's fine?

That's perfect.

> 
> In any case, I can add my test snippet to v1, so you have something
> to review with the patch. That snippet works the same on 6.12 as it
> does on 6.19 with this patch.

Thanks!