| Message ID | 20240130214620.3155380-4-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | evm: Support signatures on stacked filesystem | expand |
On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@linux.ibm.com> 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 <stefanb@linux.ibm.com> > --- > 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 <stefanb@linux.ibm.com> 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@linux.ibm.com> >> --- >> 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.
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); } }
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 <stefanb@linux.ibm.com> --- include/linux/evm.h | 8 ++++++++ security/integrity/evm/evm_main.c | 7 +++++++ security/integrity/ima/ima_main.c | 2 ++ 3 files changed, 17 insertions(+)