| 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: <aYOfenrieJLYQ9jR@CMGLRV3>
Date: Wed, 4 Feb 2026 13:35:22 -0600
From: Frederick Lawler <fred@...udflare.com>
To: Roberto Sassu <roberto.sassu@...weicloud.com>
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>,
Jeff Layton <jlayton@...nel.org>, linux-kernel@...r.kernel.org,
linux-integrity@...r.kernel.org,
linux-security-module@...r.kernel.org, kernel-team@...udflare.com
Subject: Re: [PATCH v5 2/3] ima: Make integrity_inode_attrs_changed() call
into vfs
On Wed, Feb 04, 2026 at 01:34:09PM +0100, Roberto Sassu wrote:
> On Fri, 2026-01-30 at 16:39 -0600, Frederick Lawler wrote:
> > Align integrity_inode_attrs_changed() to ima_check_last_writer()'s
> > semantics when detecting changes.
> >
> > For IMA, stacked file systems that do not set kstat.change_cookie,
> > integrity_inode_attrs_changed() will compare zero to zero, thus no
>
> I setup overlay with two xfs filesystems, kept the file I want to be
> audited in the lower filesystem.
>
> Without this patch set, if I modify the lower file, changes are
> detected, because actually the i_version is incremented.
Correct, but the test example in 00/03 demonstrates that there's no
modification. For workloads that only execute and not modify,
IMA shouldn't evaluate more than once, but that's what we're
observing at least for XFS.
>
> In which situation there is a comparison zero to zero?
My mistake. You're right, but for the wrong reason.
To be clear, these patches are about the STATX_CHANGE_COOKIE mechanic.
XFS updates the i_version regardless of the multigrain ctime changes.
You're correct in that with/without this patch there is no zero-zero
comparison for XFS, and that's because XFS isn't setting the
STATX_CHANGE_COOKIE in the result mask either for last writer check or
the attrs changed check, thus a change is always detected with
integrity_inode_attrs_stat_changed(), and thus maintains current
IMA behavior for XFS.
That said, should a file system set STATX_CHANGE_COOKIE in the result mask,
and not update the i_version (say its kept at zero), then it's always
zero-zero. I don't know how likely that scenario is.
I should reword this commit, but I am a bit hesitant to say "don't
squash this in with patch 3" due to that uncertainty.
>
> Thanks
>
> Roberto
>
> > change detected. This is not dissimilar to what
> > ima_check_last_writer() does.
> >
> > No logical change intended for EVM.
> >
> > Signed-off-by: Frederick Lawler <fred@...udflare.com>
> > ---
> > include/linux/integrity.h | 28 ++++++++++++++++++++++++----
> > security/integrity/evm/evm_main.c | 5 ++---
> > security/integrity/ima/ima_main.c | 5 ++---
> > 3 files changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> > index beb9ab19fa6257e79266b58bcb5f55b0c5445828..382c783f0fa3ae4a938cdf9559291ba1903a378e 100644
> > --- a/include/linux/integrity.h
> > +++ b/include/linux/integrity.h
> > @@ -9,6 +9,7 @@
> >
> > #include <linux/fs.h>
> > #include <linux/iversion.h>
> > +#include <linux/kernel.h>
> >
> > enum integrity_status {
> > INTEGRITY_PASS = 0,
> > @@ -62,14 +63,33 @@ integrity_inode_attrs_stat_changed
> >
> > /*
> > * 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,
> > + AT_STATX_SYNC_AS_STAT))
> > + return !IS_I_VERSION(inode) ||
> > + !inode_eq_iversion(inode, attrs->version);
> > +
> > + return integrity_inode_attrs_stat_changed(attrs, &stat);
> > }
> >
> >
> > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> > index 73d500a375cb37a54f295b0e1e93fd6e5d9ecddc..6a4e0e246005246d5700b1db590c1759242b9cb6 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -752,9 +752,8 @@ bool evm_metadata_changed(struct inode *inode, struct inode *metadata_inode)
> > bool ret = false;
> >
> > if (iint) {
> > - ret = (!IS_I_VERSION(metadata_inode) ||
> > - integrity_inode_attrs_changed(&iint->metadata_inode,
> > - metadata_inode));
> > + ret = integrity_inode_attrs_changed(&iint->metadata_inode,
> > + NULL, metadata_inode);
> > if (ret)
> > iint->evm_status = INTEGRITY_UNKNOWN;
> > }
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 6570ad10887b9ea1172c78274cf62482350e87ff..8cb17c9d446caaa5a98f5ec8f027c17ba7babca8 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -328,9 +328,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
> > real_inode = d_real_inode(file_dentry(file));
> > if (real_inode != inode &&
> > (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
> > - if (!IS_I_VERSION(real_inode) ||
> > - integrity_inode_attrs_changed(&iint->real_inode,
> > - real_inode)) {
> > + if (integrity_inode_attrs_changed(&iint->real_inode,
> > + file, real_inode)) {
> > iint->flags &= ~IMA_DONE_MASK;
> > iint->measured_pcrs = 0;
> > }
> >
>
Powered by blists - more mailing lists