On 2/6/24 10:54, Jeff Layton wrote:

On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:

quoted

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger [off-list ref] wrote:

quoted

process_measurement() will try to detect file content changes for not-yet-
copied-up files on a stacked filesystem based on the i_version number of
the real inode: !inode_eq_iversion(real_inode, iint->version)
Therefore, take a snapshot of the i_version of the real file to be used
for i_version number-based file content change detection by IMA in
process_meassurements().

In this case vfs_getattr_nosec() cannot be used since it will return the
i_version number of the file on the overlay layer which will trigger more
iint resets in process_measurements() than necessary since this i_version
number represents different state than that of the real_inode (of a
not-yet-copied up file).

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
  security/integrity/ima/ima_api.c | 28 +++++++++++++++-------------
  1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 597ea0c4d72f..530888cc481e 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c

@@ -14,6 +14,7 @@
  #include <linux/xattr.h>
  #include <linux/evm.h>
  #include <linux/fsverity.h>
+#include <linux/iversion.h>

  #include "ima.h"

@@ -250,7 +251,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
         int result = 0;
         int length;
         void *tmpbuf;
-       u64 i_version = 0;

         /*
          * Always collect the modsig, because IMA might have already collected

@@ -263,16 +263,6 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
         if (iint->flags & IMA_COLLECTED)
                 goto out;

-       /*
-        * Detecting file change is based on i_version. On filesystems
-        * which do not support i_version, support was originally limited
-        * 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,
-                                  AT_STATX_SYNC_AS_STAT);
-       if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
-               i_version = stat.change_cookie;
         hash.hdr.algo = algo;
         hash.hdr.length = hash_digest_size[algo];

@@ -302,10 +292,22 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,

         iint->ima_hash = tmpbuf;
         memcpy(iint->ima_hash, &hash, length);
-       iint->version = i_version;
-       if (real_inode != inode) {
+       if (real_inode == inode) {
+               /*
+                * Detecting file change is based on i_version. On filesystems
+                * which do not support i_version, support was originally limited
+                * 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,
+                                          AT_STATX_SYNC_AS_STAT);
+               if (!result && (stat.result_mask & STATX_CHANGE_COOKIE))
+                       iint->version = stat.change_cookie;
+       } else {
                 iint->real_ino = real_inode->i_ino;
                 iint->real_dev = real_inode->i_sb->s_dev;
+               iint->version = inode_query_iversion(real_inode);

You only want to do this if IS_I_VERSION(inode) is true. If the
underlying filesystem is doing its own thing wrt the i_version field,
calling inode_query_iversion on it may corrupt it.

quoted

quoted

         }

The commit that removed inode_query_iversion db1d1e8b9867 ("IMA: use
vfs_getattr_nosec to get the i_version") claimed to do that because
inode_query_iversion() did not work in overlayfs and now this commit
uses inode_query_iversion() only for overlayfs.

Following this patch inode_query_version() would only be used when 
real_inode != inode, such as when a copy-up has not occurred, yet. If 
real_inode == inode then this is the case for the 'overlay' layer of 
overlayfs as well as any other non-stacked filesystem that would then 
still use vfs_getattr_nosec(). So is vfs_getattr_nosec() NOT the more 
general approach for all filesystems to use here?

quoted

STATX_CHANGE_COOKIE does not seem to make much sense in this
code anymore, unless it is still needed, according to original commit to
"allow IMA to work properly with a broader class of filesystems in the future."

I don't have a real opinion here. When I did the original patch that
switched this over to to use vfs_getattr_nosec, I didn't consider that
it could end up being called from an atomic context. Reverting that

Under what conditions do we have an atomic context here? I was/am not 
aware of this.

seems like the correct thing to do if it's still broken.

If you're fine with this only working on a subset of local filesystems,
then doing something like this is probably fine:

	if (IS_I_VERSION(real_inode))
		iint->version = inode_query_iversion(real_inode);

...but it's not clear to me what you should do if IS_I_VERSION is false.
I guess IMA just falls back to checking the ctime in that case?

It does not use ctime but assumes that something has changed.