On 2/6/24 10:54, Jeff Layton wrote:
On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:
On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
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@xxxxxxxxxxxxx>
---
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.
}
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?
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.
