| 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: <aV66am9A5MmdNPbY@CMGLRV3>
Date: Wed, 7 Jan 2026 13:56:26 -0600
From: Frederick Lawler <fred@...udflare.com>
To: Jeff Layton <jlayton@...nel.org>
Cc: Mimi Zohar <zohar@...ux.ibm.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>, linux-kernel@...r.kernel.org,
linux-integrity@...r.kernel.org,
linux-security-module@...r.kernel.org, kernel-team@...udflare.com
Subject: Re: [PATCH RFC] ima: Fallback to a ctime guard without i_version
updates
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@...nel.org>
> > > > > Signed-off-by: Frederick Lawler <fred@...udflare.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@...nel.org>
>
> --
> Jeff Layton <jlayton@...nel.org>
Powered by blists - more mailing lists