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 <[email protected]>
> Signed-off-by: Frederick Lawler <[email protected]>
> ---
> 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 <[email protected]>
