xfs/ima: Regression caching i_version

xfs/ima: Regression caching i_version

[Frederick Lawler]

Hi Jeff,

While testing 6.18, I think I found a regression with
commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps") since 6.13
where IMA is no longer able to properly cache i_version when we overlay
tmpfs on top of XFS. Each measurement diff check in function
process_measurement() reports that the i_version is
always set to zero for iint->real_inode.version.

The function ima_collect_measurement() is looking to extract the version
from the cookie on next measurement to cache i_version.

I'm unclear from the commit description what the right approach here is:
update in IMA land by checking for time changes, or do
something else such as adding the cookie back.

Thanks,
Fred

Re: xfs/ima: Regression caching i_version

[Jeff Layton]

On Thu, 2025-12-11 at 14:29 -0600, Frederick Lawler wrote:
> Hi Jeff,
> 
> While testing 6.18, I think I found a regression with
> commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps") since 6.13
> where IMA is no longer able to properly cache i_version when we overlay
> tmpfs on top of XFS. Each measurement diff check in function
> process_measurement() reports that the i_version is
> always set to zero for iint->real_inode.version.
> 
> The function ima_collect_measurement() is looking to extract the version
> from the cookie on next measurement to cache i_version.
> 
> I'm unclear from the commit description what the right approach here is:
> update in IMA land by checking for time changes, or do
> something else such as adding the cookie back.
> 
> 

What we probably want to do is switch to using the ctime to manufacture
a change attribute when STATX_CHANGE_ATTRIBUTE is not set in the statx
reply.

IIRC, IMA doesn't need to persist these values across reboot, so
something like this (completely untested) might work, but it may be
better to lift nfsd4_change_attribute() into a common header and use
the same mechanism for both:

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c35ea613c9f8..5a71845f579e 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -272,10 +272,14 @@ 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 = stat.ctime.tv_sec ^ stat.ctime.tv_nsec;
+       }
        hash.hdr.algo = algo;
        hash.hdr.length = hash_digest_size[algo];
 

Re: xfs/ima: Regression caching i_version

[Frederick Lawler]

On Fri, Dec 12, 2025 at 05:55:45AM +0900, Jeff Layton wrote:
> On Thu, 2025-12-11 at 14:29 -0600, Frederick Lawler wrote:
> > Hi Jeff,
> > 
> > While testing 6.18, I think I found a regression with
> > commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps") since 6.13
> > where IMA is no longer able to properly cache i_version when we overlay
> > tmpfs on top of XFS. Each measurement diff check in function
> > process_measurement() reports that the i_version is
> > always set to zero for iint->real_inode.version.
> > 
> > The function ima_collect_measurement() is looking to extract the version
> > from the cookie on next measurement to cache i_version.
> > 
> > I'm unclear from the commit description what the right approach here is:
> > update in IMA land by checking for time changes, or do
> > something else such as adding the cookie back.
> > 
> > 
> 
> What we probably want to do is switch to using the ctime to manufacture
> a change attribute when STATX_CHANGE_ATTRIBUTE is not set in the statx
> reply.
> 
> IIRC, IMA doesn't need to persist these values across reboot, so
> something like this (completely untested) might work, but it may be
> better to lift nfsd4_change_attribute() into a common header and use
> the same mechanism for both:

I agree lifting nfsd4_change_attribute(), if anything else, a consistent
place to fetch the i_version from. Am I correct in my understanding that
the XOR on the times will cancel out and result in just the i_version?
IMA is calling into inode_eq_iversion() to perform the comparison
between the cached value and inode.i_version.
> 
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c35ea613c9f8..5a71845f579e 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -272,10 +272,14 @@ 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 = stat.ctime.tv_sec ^ stat.ctime.tv_nsec;
> +       }
>         hash.hdr.algo = algo;
>         hash.hdr.length = hash_digest_size[algo];
>  

Re: xfs/ima: Regression caching i_version

[Jeff Layton]

On Thu, 2025-12-11 at 15:12 -0600, Frederick Lawler wrote:
> On Fri, Dec 12, 2025 at 05:55:45AM +0900, Jeff Layton wrote:
> > On Thu, 2025-12-11 at 14:29 -0600, Frederick Lawler wrote:
> > > Hi Jeff,
> > > 
> > > While testing 6.18, I think I found a regression with
> > > commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps") since 6.13
> > > where IMA is no longer able to properly cache i_version when we overlay
> > > tmpfs on top of XFS. Each measurement diff check in function
> > > process_measurement() reports that the i_version is
> > > always set to zero for iint->real_inode.version.
> > > 
> > > The function ima_collect_measurement() is looking to extract the version
> > > from the cookie on next measurement to cache i_version.
> > > 
> > > I'm unclear from the commit description what the right approach here is:
> > > update in IMA land by checking for time changes, or do
> > > something else such as adding the cookie back.
> > > 
> > > 
> > 
> > What we probably want to do is switch to using the ctime to manufacture
> > a change attribute when STATX_CHANGE_ATTRIBUTE is not set in the statx
> > reply.
> > 
> > IIRC, IMA doesn't need to persist these values across reboot, so
> > something like this (completely untested) might work, but it may be
> > better to lift nfsd4_change_attribute() into a common header and use
> > the same mechanism for both:
> 
> I agree lifting nfsd4_change_attribute(), if anything else, a consistent
> place to fetch the i_version from. Am I correct in my understanding that
> the XOR on the times will cancel out and result in just the i_version?

No. I was just using the XOR to mix the tv_sec and tv_nsec fields
together in a way that (hopefully) wouldn't generate collisions. It's
quite not as robust as what nfsd4_change_attribute() does, but might be
sane enough for IMA.

> IMA is calling into inode_eq_iversion() to perform the comparison
> between the cached value and inode.i_version.

That just looks at the i_version field directly without going through -
>getattr, so that would need to be switched over as well. Could
integrity_inode_attrs_changed() use vfs_getattr_nosec() and compare the
result?


> > 
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index c35ea613c9f8..5a71845f579e 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -272,10 +272,14 @@ 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 = stat.ctime.tv_sec ^ stat.ctime.tv_nsec;
> > +       }
> >         hash.hdr.algo = algo;
> >         hash.hdr.length = hash_digest_size[algo];
> >  

-- 
Jeff Layton <jlayton@kernel.org>

Re: xfs/ima: Regression caching i_version

[Frederick Lawler]

On Fri, Dec 12, 2025 at 06:41:00AM +0900, Jeff Layton wrote:
> On Thu, 2025-12-11 at 15:12 -0600, Frederick Lawler wrote:
> > On Fri, Dec 12, 2025 at 05:55:45AM +0900, Jeff Layton wrote:
> > > On Thu, 2025-12-11 at 14:29 -0600, Frederick Lawler wrote:
> > > > Hi Jeff,
> > > > 
> > > > While testing 6.18, I think I found a regression with
> > > > commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps") since 6.13
> > > > where IMA is no longer able to properly cache i_version when we overlay
> > > > tmpfs on top of XFS. Each measurement diff check in function
> > > > process_measurement() reports that the i_version is
> > > > always set to zero for iint->real_inode.version.
> > > > 
> > > > The function ima_collect_measurement() is looking to extract the version
> > > > from the cookie on next measurement to cache i_version.
> > > > 
> > > > I'm unclear from the commit description what the right approach here is:
> > > > update in IMA land by checking for time changes, or do
> > > > something else such as adding the cookie back.
> > > > 
> > > > 
> > > 
> > > What we probably want to do is switch to using the ctime to manufacture
> > > a change attribute when STATX_CHANGE_ATTRIBUTE is not set in the statx
> > > reply.
> > > 
> > > IIRC, IMA doesn't need to persist these values across reboot, so
> > > something like this (completely untested) might work, but it may be
> > > better to lift nfsd4_change_attribute() into a common header and use
> > > the same mechanism for both:
> > 
> > I agree lifting nfsd4_change_attribute(), if anything else, a consistent
> > place to fetch the i_version from. Am I correct in my understanding that
> > the XOR on the times will cancel out and result in just the i_version?
> 
> No. I was just using the XOR to mix the tv_sec and tv_nsec fields
> together in a way that (hopefully) wouldn't generate collisions. It's
> quite not as robust as what nfsd4_change_attribute() does, but might be
> sane enough for IMA.
> 
> > IMA is calling into inode_eq_iversion() to perform the comparison
> > between the cached value and inode.i_version.
> 
> That just looks at the i_version field directly without going through -
> >getattr, so that would need to be switched over as well. Could
> integrity_inode_attrs_changed() use vfs_getattr_nosec() and compare the
> result?

That makes sense to me. I'll look through it a bit more, roll a patch, and
see what the IMA folks have to say (unless they comment here first).

Thanks Jeff

> 
> 
> > > 
> > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > > index c35ea613c9f8..5a71845f579e 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -272,10 +272,14 @@ 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 = stat.ctime.tv_sec ^ stat.ctime.tv_nsec;
> > > +       }
> > >         hash.hdr.algo = algo;
> > >         hash.hdr.length = hash_digest_size[algo];
> > >  
> 
> -- 
> Jeff Layton <jlayton@kernel.org>

Re: xfs/ima: Regression caching i_version

[Jeff Layton]

On Thu, 2025-12-11 at 16:29 -0600, Frederick Lawler wrote:
> On Fri, Dec 12, 2025 at 06:41:00AM +0900, Jeff Layton wrote:
> > On Thu, 2025-12-11 at 15:12 -0600, Frederick Lawler wrote:
> > > On Fri, Dec 12, 2025 at 05:55:45AM +0900, Jeff Layton wrote:
> > > > On Thu, 2025-12-11 at 14:29 -0600, Frederick Lawler wrote:
> > > > > Hi Jeff,
> > > > > 
> > > > > While testing 6.18, I think I found a regression with
> > > > > commit 1cf7e834a6fb ("xfs: switch to multigrain timestamps") since 6.13
> > > > > where IMA is no longer able to properly cache i_version when we overlay
> > > > > tmpfs on top of XFS. Each measurement diff check in function
> > > > > process_measurement() reports that the i_version is
> > > > > always set to zero for iint->real_inode.version.
> > > > > 
> > > > > The function ima_collect_measurement() is looking to extract the version
> > > > > from the cookie on next measurement to cache i_version.
> > > > > 
> > > > > I'm unclear from the commit description what the right approach here is:
> > > > > update in IMA land by checking for time changes, or do
> > > > > something else such as adding the cookie back.
> > > > > 
> > > > > 
> > > > 
> > > > What we probably want to do is switch to using the ctime to manufacture
> > > > a change attribute when STATX_CHANGE_ATTRIBUTE is not set in the statx
> > > > reply.
> > > > 
> > > > IIRC, IMA doesn't need to persist these values across reboot, so
> > > > something like this (completely untested) might work, but it may be
> > > > better to lift nfsd4_change_attribute() into a common header and use
> > > > the same mechanism for both:
> > > 
> > > I agree lifting nfsd4_change_attribute(), if anything else, a consistent
> > > place to fetch the i_version from. Am I correct in my understanding that
> > > the XOR on the times will cancel out and result in just the i_version?
> > 
> > No. I was just using the XOR to mix the tv_sec and tv_nsec fields
> > together in a way that (hopefully) wouldn't generate collisions. It's
> > quite not as robust as what nfsd4_change_attribute() does, but might be
> > sane enough for IMA.
> > 
> > > IMA is calling into inode_eq_iversion() to perform the comparison
> > > between the cached value and inode.i_version.
> > 
> > That just looks at the i_version field directly without going through -
> > > getattr, so that would need to be switched over as well. Could
> > integrity_inode_attrs_changed() use vfs_getattr_nosec() and compare the
> > result?
> 
> That makes sense to me. I'll look through it a bit more, roll a patch, and
> see what the IMA folks have to say (unless they comment here first).
> 

Great, thanks. Looks like ima_check_last_writer() might also need a
similar change.

I'm not sure, but It's possible that vfs_getattr_nosec() may not be
callable from all the contexts where integrity_inode_attrs_changed() is
called. If that is the case, then you may just need to convert IMA back
to accessing the i_version field directly instead of going through
vfs_getattr_nosec().

That's less than ideal though, as it will limit the filesystems where
IMA can be implemented. You'll need to come up with a way to deal with
the fact that XFS's i_version field can change on atime updates.
-- 
Jeff Layton <jlayton@kernel.org>