On 1/31/24 08:56, Amir Goldstein wrote:
On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@xxxxxxxxxxxxx> wrote:
To avoid caching effects to take effect reset the EVM status upon
detecting changes to the overlay backing files. This prevents a not-yet-
copied-up file on the overlay from executing if for example the
security.evm xattr on the file on the 'lower' layer has been removed.
And what is expected to happen when file is executed after copy up?
The copy-up may be triggered by changing file content or file metadata.
For EVM file metadata (file attributes and xattrs) are important and if
they change EVM would re-evaluate the file, meaning that it would
determine the file mode bits, uid, gid and xattrs and calculate a hash
over them and compare this hash against the signature in security.evm.
Doesn't this change also protect the same threat after copy up?
From what I remember from my testing is that file attribute or extended
attribute changes on an already copied-up file were already handled
correctly, meaning they caused the re-evaluation of the file as
described above.
Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
---
include/linux/evm.h | 8 ++++++++
security/integrity/evm/evm_main.c | 7 +++++++
security/integrity/ima/ima_main.c | 2 ++
3 files changed, 17 insertions(+)
diff --git a/include/linux/evm.h b/include/linux/evm.h
index d8c0343436b8..e7d6742eee9d 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -66,6 +66,8 @@ extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
int buffer_size, char type,
bool canonical_fmt);
+extern void evm_reset_cache_status(struct dentry *dentry,
+ struct integrity_iint_cache *iint);
#ifdef CONFIG_FS_POSIX_ACL
extern int posix_xattr_acl(const char *xattrname);
#else
@@ -189,5 +191,11 @@ static inline int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
return -EOPNOTSUPP;
}
+static inline void evm_reset_cache_status(struct dentry *dentry,
+ struct integrity_iint_cache *iint)
+{
+ return;
+}
+
#endif /* CONFIG_EVM */
#endif /* LINUX_EVM_H */
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 22a5e26860ea..e96d127b48a2 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -721,6 +721,13 @@ static void evm_reset_status(struct inode *inode)
iint->evm_status = INTEGRITY_UNKNOWN;
}
+void evm_reset_cache_status(struct dentry *dentry,
+ struct integrity_iint_cache *iint)
+{
+ if (d_real_inode(dentry) != d_backing_inode(dentry))
+ iint->evm_status = INTEGRITY_UNKNOWN;
+}
+
/**
* evm_revalidate_status - report whether EVM status re-validation is necessary
* @xattr_name: pointer to the affected extended attribute name
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cc1217ac2c6f..84bdc6e58329 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -26,6 +26,7 @@
#include <linux/ima.h>
#include <linux/fs.h>
#include <linux/iversion.h>
+#include <linux/evm.h>
#include "ima.h"
@@ -295,6 +296,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
!inode_eq_iversion(backing_inode, iint->version)) {
iint->flags &= ~IMA_DONE_MASK;
iint->measured_pcrs = 0;
+ evm_reset_cache_status(file_dentry(file), iint);
}
}
Make sense.
Unrelated to your change, I now noticed something odd about Mimi's change:
backing_inode = d_real_inode(file_dentry(file));
I find the choice of variable name to be quite confusing, because ima/evm code
uses d_backing_inode() all over the place and d_real_inode() !=
d_backing_inode().
First of all, there is never any reason to use d_backing_inode() and its name is
quite confusing in the first place, but it will be a big cleanup to
remove them all.
Suggest to rename the variable to real_inode, same as in
ima_collect_measurement()
to be consistent and reduce confusion factor, which is already high enough ;)
Thanks,
Amir.
