| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <42da6a2c72b1093b7aa5106cdce4d0efdbd785ee.camel@linux.ibm.com>
Date: Thu, 29 Jan 2026 21:32:17 -0500
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Frederick Lawler <fred@...udflare.com>,
Roberto Sassu
<roberto.sassu@...wei.com>,
Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
Eric Snowberg <eric.snowberg@...cle.com>,
Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>,
"Darrick J. Wong" <djwong@...nel.org>,
Christian Brauner
<brauner@...nel.org>,
Josef Bacik <josef@...icpanda.com>, Jeff Layton
<jlayton@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-integrity@...r.kernel.org,
linux-security-module@...r.kernel.org, kernel-team@...udflare.com
Subject: Re: [PATCH v4] ima: Fallback to ctime check for FS without
kstat.change_cookie
On Thu, 2026-01-29 at 12:07 -0600, Frederick Lawler wrote:
> Commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps")
> introduced a means to track change detection for an inode
> via ctime updates, opposed to setting kstat.change_cookie to
> an i_version when calling into xfs_vn_getattr().
>
> This introduced a regression for IMA such that an action
> performed on a LOWER inode on a stacked file systems always
> requires a re-evaluation if the LOWER file system does not
> leverage kstat.change_cookie to track inode i_version or lacks
> i_version support all together.
Please describe the change in behavior that needs to be fixed. Are there too
many, too few measurements, or both?
Examples are fine, but first describe the problem - not detecting file change on
xfs.
>
> In the case of stacking XFS on XFS, an action on either the LOWER or UPPER
> will require re-evaluation. Stacking TMPFS on XFS for instance, once the
> inode is UPPER is mutated, IMA resumes normal behavior because TMPFS
> leverages generic_fillattr() to update the change cookie.
This sounds like the same issue - not detecting file change on xfs. The problem
is simply manifesting itself on stacked filesystems.
>
> This is because IMA caches kstat.change_cookie to compare against an
> inode's i_version directly in integrity_inode_attrs_changed(), and thus
> could be out of date depending on how file systems set
> kstat.change_cookie.
>
> To address this, require integrity_inode_attrs_changed() to query
> vfs_getattr_nosec() to compare the cached version against
> kstat.change_cookie directly. This ensures that when updates occur,
> we're accessing the same changed inode version on changes, and fallback
> to compare against kstat.ctime when STATX_CHANGE_COOKIE is missing from
> result mask.
>
> Lastly, because EVM still relies on querying and caching a inode's
> i_version directly, the integrity_inode_attrs_changed() falls back to the
> original inode.i_version != cached comparison.
>
> Link: https://lore.kernel.org/all/aTspr4_h9IU4EyrR@CMGLRV3
> Fixes: 1cf7e834a6fb ("xfs: switch to multigrain timestamps")
> Suggested-by: Jeff Layton <jlayton@...nel.org>
> Reviewed-by: Jeff Layton <jlayton@...nel.org>
> Signed-off-by: Frederick Lawler <fred@...udflare.com>
> ---
> include/linux/integrity.h | 35 +++++++++++++++++++++++++++++++----
> security/integrity/evm/evm_main.c | 5 ++---
> security/integrity/ima/ima_api.c | 11 ++++++++---
> security/integrity/ima/ima_main.c | 17 ++++++++++-------
> 4 files changed, 51 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> index f5842372359be5341b6870a43b92e695e8fc78af..034f0a1ed48ca8c19c764e302bbfc555dad92cde 100644
> --- a/include/linux/integrity.h
> +++ b/include/linux/integrity.h
> @@ -9,6 +9,8 @@
>
> #include <linux/fs.h>
> #include <linux/iversion.h>
> +#include <linux/kernel.h>
> +#include <linux/time64.h>
>
> enum integrity_status {
> INTEGRITY_PASS = 0,
> @@ -51,14 +53,39 @@ integrity_inode_attrs_store(struct integrity_inode_attributes *attrs,
>
> /*
> * On stacked filesystems detect whether the inode or its content has changed.
> + *
> + * Must be called in process context.
> */
> 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;
> +
> + might_sleep();
> +
> + if (inode->i_sb->s_dev != attrs->dev || inode->i_ino != attrs->ino)
> + return true;
> +
> + /*
> + * EVM currently relies on backing inode i_version. While IS_I_VERSION
> + * is not a good indicator of i_version support, this still retains
> + * the logic such that a re-evaluation should still occur for EVM, and
> + * only for IMA if vfs_getattr_nosec() fails.
> + */
> + if (!file || vfs_getattr_nosec(&file->f_path, &stat,
> + STATX_CHANGE_COOKIE | STATX_CTIME,
> + AT_STATX_SYNC_AS_STAT))
> + return !IS_I_VERSION(inode) ||
> + !inode_eq_iversion(inode, attrs->version);
> +
> + if (stat.result_mask & STATX_CHANGE_COOKIE)
> + return stat.change_cookie != attrs->version;
> +
> + if (stat.result_mask & STATX_CTIME)
> + return timespec64_to_ns(&stat.ctime) != (s64)attrs->version;
> +
> + return true;
> }
>
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 5770cf691912aa912fc65280c59f5baac35dd725..8ac42b03740eb93bf23b15cb9039af6cd32aa999 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -28,6 +28,7 @@
> #include <linux/iversion.h>
> #include <linux/evm.h>
> #include <linux/crash_dump.h>
> +#include <linux/time64.h>
>
> #include "ima.h"
>
> @@ -199,10 +200,13 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
> &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) {
> + STATX_CHANGE_COOKIE | STATX_CTIME,
> + AT_STATX_SYNC_AS_STAT) ||
> + ((stat.result_mask & STATX_CHANGE_COOKIE) ?
> + stat.change_cookie != iint->real_inode.version :
> + (!(stat.result_mask & STATX_CTIME) ||
> + timespec64_to_ns(&stat.ctime) !=
> + (s64)iint->real_inode.version))) {
> iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> iint->measured_pcrs = 0;
> if (update)
The original i_version test was clear. This code has become really hard to
understand and needs to be cleaned up. Defining a helper function, will also
avoid code duplication.
Mimi
Powered by blists - more mailing lists