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.
Signed-off-by: Stefan Berger <[email protected]>
---
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);
}
}
--
2.43.0
On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> 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?
Doesn't this change also protect the same threat after copy up?
> Signed-off-by: Stefan Berger <[email protected]>
> ---
> 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.
On 1/31/24 08:56, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <[email protected]> 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 <[email protected]>
>> ---
>> 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.