From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ot1-f45.google.com (mail-ot1-f45.google.com [209.85.210.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8EFD72C029F for ; Thu, 15 Jan 2026 18:10:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.210.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768500642; cv=none; b=XKBanNHtSWzL6y/CqxYKq7AQo5iWHeLdnt4C8sCmCp56ZlLHXBBq3ZgpwurhYGxMxTUJpMFmf+HSunH71PJpBrSwJv/JVkSCWokXjMEE9crsj4eNe3AkIQjlVJx0MqAY0SEC6a1/IDFLua5t1EuehQWHgJpaShAqnAR96ZufGXo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768500642; c=relaxed/simple; bh=/8rDZ++wjiwDxMZuAHQpg75MVGHaHMBkLzd5tdnTz4w=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dqYC72mLlP7lMYBv17pXJkuvGyODvXSfM+oo96p+oO9Y8Wdd7Jj8UoDLd3Ynij7BxU81UVvlo3PIP7/A1XKODdJqoVIiz2S6EoS2DKDe0mN+7/H4go3v/HQu69zw5t+WE5J77yh6g92NdojYchLQ2Wl7wPkGsFEQO/gSzaejzGQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=cloudflare.com; spf=pass smtp.mailfrom=cloudflare.com; dkim=pass (2048-bit key) header.d=cloudflare.com header.i=@cloudflare.com header.b=fy7RkU3C; arc=none smtp.client-ip=209.85.210.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=cloudflare.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=cloudflare.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=cloudflare.com header.i=@cloudflare.com header.b="fy7RkU3C" Received: by mail-ot1-f45.google.com with SMTP id 46e09a7af769-7cfcbf34124so813069a34.0 for ; Thu, 15 Jan 2026 10:10:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloudflare.com; s=google09082023; t=1768500638; x=1769105438; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=8BemoNEWUh+R2I1lFJL4SATcppPiP2aVt64nnFLdo8s=; b=fy7RkU3CL0+HX/QcLn0rNYsR4WHkLHXlH6F2JIaWtWbKFEaoAJuak5TIb/Wt8v4wGY QjTWWTCSH7Qv/C5r/ptN/f6LB2JXxW5TWyZlULdJn1GtlK0ppmAS5KpmSWIshxQ8hUE+ iGZTd6iCNfra3C97s9KEpPHsQ4t5KJFZJamArM37c13uDUMIHwRkF08h199T0IOyayhR nmv5xV4en+9c7yfK1lrAWk+uYi/iCsN0C57PjDI2FV/0kPOPLazJR06gjPfFvBGGGqY/ HJMgZ7XBwA2zokdz8K9gTvd++n58hf6/Ph/CKGmqtdcCt/3CitqvJ4sqQXKNAgdDIpAC PiiA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768500638; x=1769105438; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=8BemoNEWUh+R2I1lFJL4SATcppPiP2aVt64nnFLdo8s=; b=GWmlN3PaKS16d5Kzo86qbsYwwkG3HNNk3Tvx78JztZBynaRLDXcUA9Ow05CxcKRCYg u1QHc/qJsyVEIEVdrkuyjP4yXOgeNQgZF4W0HOVJKIg9/uxYmGOTKxq6ogMkLxJ7qQ6s CEbz204TlGcWhuiZSQ2SNSJE4Ob2qtXXRdTZyXcmMONTau5QomGuJilFViDHDT5Y3j+y KBGB9ph+AL55a2kepEQljTzpSY2ZbAXPRv1gCByapZ7K99HucwSMwhGEj+b35MqcQ6sG LDfbHONJ0mJvRjRyvigACmjUHV4qVfH7ZrizmCfcBkP7IeXhLSjRBOMPhbxyiX2D0Z4W ccKQ== X-Forwarded-Encrypted: i=1; AJvYcCUo3qE4B1j+XfBmzHyokuI8X0FtwXEqU2mdMN+S2bHX4OCotHFJUTDkVUsQDKyMU3KsRxNyitGcn/cggBc=@vger.kernel.org X-Gm-Message-State: AOJu0YxO2l5fPtUPR0J+s++2WeyxTDvSzULwdkYBP39NiEP45nTJwuYB HHeCBWxKY14Z3K14T353m/K6LVxovznSlhXehorUHIRZtt1tX4Xw6AEDDC+iNJpmEvs= X-Gm-Gg: AY/fxX6NRcgp4oxQeY2JZHHkqxekhEKjs+dLZfvSmcGYK2B8BFo/+9DRbcr7AFuGGBo C65TN33yo+m+paAfykQREv6fwjwDMVDfLWjpi1Fx/U8g4EQMhbAaInkrnZeeZrSEnsvMx0Xo77b KMjg6zmfme41fYTimw+/B/qzZZdrBV3tBLo4P2Lcumbh8/gfXiuG5l6QxvIfKayBtmPwyhliN1y goWNchx8KVMi08axyRjuKlST93D4RKNv09kIQh2iWRBBuJ4k53Pa4TgnExTYC9+69IckXR2sGsc zNXqKDrgiWXA9Tfg5HoIFHQKzHboHrdYi2Dmy7nwb67ebU+kKcixaav2DK4qsDuUxK6lcZM4sEa d3SFZQqDGjv3XKUO8JvMchC5Nu/bFUOAuGVzGGv/JQ7xVKd4fAsf192wCVi1e2QCQ9zRM X-Received: by 2002:a05:6830:81da:b0:7c7:8280:9205 with SMTP id 46e09a7af769-7cfdeeab6b7mr232896a34.37.1768500638371; Thu, 15 Jan 2026 10:10:38 -0800 (PST) Received: from CMGLRV3 ([2a09:bac5:947d:1b37::2b6:50]) by smtp.gmail.com with ESMTPSA id 46e09a7af769-7cfdf28ef44sm115682a34.18.2026.01.15.10.10.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Jan 2026 10:10:37 -0800 (PST) Date: Thu, 15 Jan 2026 12:10:35 -0600 From: Frederick Lawler To: Roberto Sassu Cc: Mimi Zohar , Roberto Sassu , Dmitry Kasatkin , Eric Snowberg , Paul Moore , James Morris , "Serge E. Hallyn" , "Darrick J. Wong" , Christian Brauner , Josef Bacik , Jeff Layton , linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, kernel-team@cloudflare.com Subject: Re: [PATCH] ima: Detect changes to files via kstat changes rather than i_version Message-ID: References: <20260112-xfs-ima-fixup-v1-1-8d13b6001312@cloudflare.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Thu, Jan 15, 2026 at 12:46:37PM +0100, Roberto Sassu wrote: > On Mon, 2026-01-12 at 16:32 -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 when > > calling into xfs_vn_getattr(). > > > > This introduced a regression 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 increment i_version. > > > > 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 an artificial version generated from kstat.ctime > > via integrity_ctime_guard() when there's no detected change > > to the kstat.change_cookie. > > > > This ensures that in the absence of i_version support for file systems, > > and in the absence of a kstat.change_cookie update, we ultimately have a > > unique-enough version to compare against. > > > > The exact implementation for integrity_ctime_guard() is to ensure that > > if tv_sec or tv_nsec are zero, there's some value to store back into > > struct integrity_inode_attributes.version. This also avoids the need to > > add additional storage and comparisons. > > > > Lastly, because EVM still relies on querying and caching a backing inode's > > i_version, the integrity_inode_attrs_changed() falls back to the > > original inode.i_version != cached comparison. This maintains the > > invariant that a re-evaluation in unknown change detection circumstances > > is required. > > > > Link: https://lore.kernel.org/all/aTspr4_h9IU4EyrR@CMGLRV3 > > Suggested-by: Jeff Layton > > Signed-off-by: Frederick Lawler > > --- > > We uncovered a case in kernels >= 6.13 where XFS is no longer updating > > struct kstat.change_cookie on i_op getattr() access calls. Instead, XFS is > > using multigrain ctime (as well as other file systems) for > > change detection in commit 1cf7e834a6fb ("xfs: switch to > > multigrain timestamps"). > > > > Because file systems may implement i_version as they see fit, IMA > > caching may be behind as well as file systems that don't support/export > > i_version. Thus we're proposing to compare against the kstat.change_cookie > > directly to the cached version, and fall back to a ctime guard when > > that's not updated. > > > > EVM is largely left alone since there's no trivial way to query a file > > directly in the LSM call paths to obtain kstat.change_cookie & > > kstat.ctime to cache. Thus retains accessing i_version directly. > > > > Regression tests will be added to the Linux Test Project instead of > > selftest to help catch future file system changes that may impact > > future evaluation of IMA. > > > > I'd like this to be backported to at least 6.18 if possible. > > > > Below is a simplified test that demonstrates the issue: > > > > _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_ > > > > 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 > > --- > > Changes since RFC: > > - Remove calls to I_IS_VERSION() > > - Function documentation/comments > > - Abide IMA/EVM change detection fallback invariants > > - Combined ctime guard into version for attributes struct > > - Link to RFC: https://lore.kernel.org/r/20251229-xfs-ima-fixup-v1-1-6a717c939f7c@cloudflare.com > > --- > > include/linux/integrity.h | 42 +++++++++++++++++++++++++++++++++++---- > > security/integrity/evm/evm_main.c | 5 ++--- > > security/integrity/ima/ima_api.c | 11 +++++++--- > > security/integrity/ima/ima_main.c | 15 +++++--------- > > 4 files changed, 53 insertions(+), 20 deletions(-) > > > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h > > index f5842372359be5341b6870a43b92e695e8fc78af..5eca8aa2769f9238c68bb40885ecc46910524f11 100644 > > --- a/include/linux/integrity.h > > +++ b/include/linux/integrity.h > > @@ -9,6 +9,7 @@ > > > > #include > > #include > > +#include > > > > enum integrity_status { > > INTEGRITY_PASS = 0, > > @@ -36,6 +37,14 @@ struct integrity_inode_attributes { > > dev_t dev; > > }; > > > > +/* > > + * Wrapper to generate an artificial version for a file. > > + */ > > +static inline u64 integrity_ctime_guard(struct kstat stat) > > +{ > > + return stat.ctime.tv_sec ^ stat.ctime.tv_nsec; > > Unfortunately, we cannot take the risk of a collision. Better use all > or a packed version. Sounds good. > > > +} > > + > > /* > > * On stacked filesystems the i_version alone is not enough to detect file data > > * or metadata change. Additional metadata is required. > > @@ -51,14 +60,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 integrity_ctime_guard(stat) != attrs->version; > > Yes, switching to the new field I guess it works, but I'm wondering if > we could have more uniformity across the filesystems, otherwise one has > to use one source for filesystem X, another source for filesystem Y. Agreed. But I'm under the impression from casual searching, that most file systems are likely to support ctime, than setting the change cookie with an i_version or even having/updating i_version consistently. Is there someone we could CC in here to get another opinion? > > Thanks > > Roberto > > > + > > + return true; > > } > > > > > > 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_api.c b/security/integrity/ima/ima_api.c > > index c35ea613c9f8d404ba4886e3b736c3bab29d1668..8096986f3689781d3cdf6595f330033782f9cc45 100644 > > --- a/security/integrity/ima/ima_api.c > > +++ b/security/integrity/ima/ima_api.c > > @@ -272,10 +272,15 @@ 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; > > + else if (stat.result_mask & STATX_CTIME) > > + i_version = integrity_ctime_guard(stat); > > + } > > hash.hdr.algo = algo; > > hash.hdr.length = hash_digest_size[algo]; > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > > index 5770cf691912aa912fc65280c59f5baac35dd725..3a4c32e254f925bba85cb91b63744ac142b3b049 100644 > > --- a/security/integrity/ima/ima_main.c > > +++ b/security/integrity/ima/ima_main.c > > @@ -22,6 +22,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -191,18 +192,13 @@ static void ima_check_last_writer(struct ima_iint_cache *iint, > > > > mutex_lock(&iint->mutex); > > if (atomic_read(&inode->i_writecount) == 1) { > > - struct kstat stat; > > - > > clear_bit(IMA_EMITTED_OPENWRITERS, &iint->atomic_flags); > > > > 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) { > > + integrity_inode_attrs_changed(&iint->real_inode, file, > > + inode)) { > > iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE); > > iint->measured_pcrs = 0; > > if (update) > > @@ -328,9 +324,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; > > } > > > > --- > > base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8 > > change-id: 20251212-xfs-ima-fixup-931780a62c2c > > > > Best regards, >