EVM signature verification on stacked filesystem has recently been
completely disabled by declaring some filesystems as unsupported
(only overlayfs). This series now enables copy-up of "portable
and immutable" signatures on those filesystems and enables the
enforcement of "portable and immultable" as well as the "original"
signatures on previously unsupported filesystem when evm is enabled
with EVM_INIT_X509. HMAC verification and generation remains disabled.
"Portable and immutable" signatures can be copied up since they are
not created over file-specific metadata, such as UUID or generation.
Instead, they are only covering file metadata such as mode bits, uid, and
gid, that will all be preserved during a copy-up of the file metadata.
Regards,
Stefan
v2:
- Added patch to rename backing_inode to real_inode (1/9)
- Added patches renaming flag and function due to RSA enablement (7,8/9)
- Added patch to record i_version of real_inode for change detection (9/9)
- Use Amir's function to get inode holding metadata now (4,5/9)
Stefan Berger (9):
ima: Rename backing_inode to real_inode
security: allow finer granularity in permitting copy-up of security
xattrs
evm: Implement per signature type decision in
security_inode_copy_up_xattr
ima: Reset EVM status upon detecting changes to the real file
evm: Use the inode holding the metadata to calculate metadata hash
evm: Enforce signatures on unsupported filesystem for EVM_INIT_X509
fs: Rename SB_I_EVM_UNSUPPORTED to SB_I_EVM_HMAC_UNSUPPORTED
evm: Rename is_unsupported_fs to is_unsupported_hmac_fs
ima: Record i_version of real_inode for change detection
fs/overlayfs/copy_up.c | 2 +-
fs/overlayfs/super.c | 2 +-
include/linux/evm.h | 13 +++++-
include/linux/fs.h | 2 +-
include/linux/lsm_hook_defs.h | 3 +-
include/linux/security.h | 4 +-
security/integrity/evm/evm_crypto.c | 2 +-
security/integrity/evm/evm_main.c | 69 ++++++++++++++++++++++-------
security/integrity/ima/ima_api.c | 28 ++++++------
security/integrity/ima/ima_main.c | 23 ++++++----
security/security.c | 7 +--
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
13 files changed, 107 insertions(+), 52 deletions(-)
--
2.43.0
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 <[email protected]>
---
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);
}
/* Possibly temporary failure due to type of read (eg. O_DIRECT) */
--
2.43.0
Now that EVM supports RSA signatures for previously completely
unsupported filesystems rename the flag SB_I_EVM_UNSUPPORTED to
SB_I_EVM_HMAC_UNSUPPORTED to reflect that only HMAC is not supported.
Suggested-by: Amir Goldstein <[email protected]>
Suggested-by: Mimi Zohar <[email protected]>
Signed-off-by: Stefan Berger <[email protected]>
---
fs/overlayfs/super.c | 2 +-
include/linux/fs.h | 2 +-
security/integrity/evm/evm_main.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 460126b7e1cd..db132d437e14 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1445,7 +1445,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
* lead to unexpected results.
*/
sb->s_iflags |= SB_I_NOUMASK;
- sb->s_iflags |= SB_I_EVM_UNSUPPORTED;
+ sb->s_iflags |= SB_I_EVM_HMAC_UNSUPPORTED;
err = -ENOMEM;
root_dentry = ovl_get_root(sb, ctx->upper.dentry, oe);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1823a93202bd..37306a09b4dc 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1177,7 +1177,7 @@ extern int send_sigurg(struct fown_struct *fown);
#define SB_I_USERNS_VISIBLE 0x00000010 /* fstype already mounted */
#define SB_I_IMA_UNVERIFIABLE_SIGNATURE 0x00000020
#define SB_I_UNTRUSTED_MOUNTER 0x00000040
-#define SB_I_EVM_UNSUPPORTED 0x00000080
+#define SB_I_EVM_HMAC_UNSUPPORTED 0x00000080
#define SB_I_SKIP_SYNC 0x00000100 /* Skip superblock at global sync */
#define SB_I_PERSB_BDI 0x00000200 /* has a per-sb bdi */
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index c3bd88aba78c..ff659e622f4a 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -155,7 +155,7 @@ static int is_unsupported_fs(struct dentry *dentry)
{
struct inode *inode = d_backing_inode(dentry);
- if (inode->i_sb->s_iflags & SB_I_EVM_UNSUPPORTED) {
+ if (inode->i_sb->s_iflags & SB_I_EVM_HMAC_UNSUPPORTED) {
pr_info_once("%s not supported\n", inode->i_sb->s_type->name);
return 1;
}
--
2.43.0
Copying up xattrs is solely based on the security xattr name. For finer
granularity add a dentry parameter to the security_inode_copy_up_xattr
hook definition, allowing decisions to be based on the xattr content as
well.
Signed-off-by: Stefan Berger <[email protected]>
---
fs/overlayfs/copy_up.c | 2 +-
include/linux/evm.h | 5 +++--
include/linux/lsm_hook_defs.h | 3 ++-
include/linux/security.h | 4 ++--
security/integrity/evm/evm_main.c | 2 +-
security/security.c | 7 ++++---
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
8 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8586e2f5d243..de20fe9333c0 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
if (ovl_is_private_xattr(sb, name))
continue;
- error = security_inode_copy_up_xattr(name);
+ error = security_inode_copy_up_xattr(old, name);
if (error < 0 && error != -EOPNOTSUPP)
break;
if (error == 1) {
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 36ec884320d9..840ffbdc2860 100644
--- a/include/linux/evm.h
+++ b/include/linux/evm.h
@@ -31,7 +31,7 @@ extern void evm_inode_post_setxattr(struct dentry *dentry,
const char *xattr_name,
const void *xattr_value,
size_t xattr_value_len);
-extern int evm_inode_copy_up_xattr(const char *name);
+extern int evm_inode_copy_up_xattr(struct dentry *dentry, const char *name);
extern int evm_inode_removexattr(struct mnt_idmap *idmap,
struct dentry *dentry, const char *xattr_name);
extern void evm_inode_post_removexattr(struct dentry *dentry,
@@ -118,7 +118,8 @@ static inline void evm_inode_post_setxattr(struct dentry *dentry,
return;
}
-static inline int evm_inode_copy_up_xattr(const char *name)
+static inline int evm_inode_copy_up_xattr(struct dentry *dentry,
+ const char *name)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 185924c56378..7dd61f51d84a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -163,7 +163,8 @@ LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
size_t buffer_size)
LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
-LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, const char *name)
+LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, struct dentry *src,
+ const char *name)
LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
struct kernfs_node *kn)
LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
diff --git a/include/linux/security.h b/include/linux/security.h
index d0eb20f90b26..9fc9ca6284d6 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -387,7 +387,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
void security_inode_getsecid(struct inode *inode, u32 *secid);
int security_inode_copy_up(struct dentry *src, struct cred **new);
-int security_inode_copy_up_xattr(const char *name);
+int security_inode_copy_up_xattr(struct dentry *src, const char *name);
int security_kernfs_init_security(struct kernfs_node *kn_dir,
struct kernfs_node *kn);
int security_file_permission(struct file *file, int mask);
@@ -980,7 +980,7 @@ static inline int security_kernfs_init_security(struct kernfs_node *kn_dir,
return 0;
}
-static inline int security_inode_copy_up_xattr(const char *name)
+static inline int security_inode_copy_up_xattr(struct dentry *src, const char *name)
{
return -EOPNOTSUPP;
}
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index cc7956d7878b..2555aa4501ae 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -896,7 +896,7 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
evm_update_evmxattr(dentry, NULL, NULL, 0);
}
-int evm_inode_copy_up_xattr(const char *name)
+int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
{
if (strcmp(name, XATTR_NAME_EVM) == 0)
return 1; /* Discard */
diff --git a/security/security.c b/security/security.c
index 0144a98d3712..ee63863c1dc0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2596,6 +2596,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
/**
* security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
+ * @src: union dentry of copy-up file
* @name: xattr name
*
* Filter the xattrs being copied up when a unioned file is copied up from a
@@ -2606,7 +2607,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
* if the security module does not know about attribute, or a negative
* error code to abort the copy up.
*/
-int security_inode_copy_up_xattr(const char *name)
+int security_inode_copy_up_xattr(struct dentry *src, const char *name)
{
struct security_hook_list *hp;
int rc;
@@ -2618,12 +2619,12 @@ int security_inode_copy_up_xattr(const char *name)
*/
hlist_for_each_entry(hp,
&security_hook_heads.inode_copy_up_xattr, list) {
- rc = hp->hook.inode_copy_up_xattr(name);
+ rc = hp->hook.inode_copy_up_xattr(src, name);
if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
return rc;
}
- return evm_inode_copy_up_xattr(name);
+ return evm_inode_copy_up_xattr(src, name);
}
EXPORT_SYMBOL(security_inode_copy_up_xattr);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6bf90ace84c..ebb8876837c6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3530,7 +3530,7 @@ static int selinux_inode_copy_up(struct dentry *src, struct cred **new)
return 0;
}
-static int selinux_inode_copy_up_xattr(const char *name)
+static int selinux_inode_copy_up_xattr(struct dentry *dentry, const char *name)
{
/* The copy_up hook above sets the initial context on an inode, but we
* don't then want to overwrite it by blindly copying all the lower
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0fdbf04cc258..bffca165f07f 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4873,7 +4873,7 @@ static int smack_inode_copy_up(struct dentry *dentry, struct cred **new)
return 0;
}
-static int smack_inode_copy_up_xattr(const char *name)
+static int smack_inode_copy_up_xattr(struct dentry *src, const char *name)
{
/*
* Return 1 if this is the smack access Smack attribute.
--
2.43.0
To support portable and immutable signatures on otherwise unsupported
filesystems, determine the EVM signature type by the content of a file's
xattr. If the file has the appropriate signature then allow it to be
copied up. All other signature types are discarded as before.
"Portable and immutable" EVM signatures can be copied up by stacked file-
systems since the metadata their signature covers does not include file-
system-specific data such as a file's inode number, generation, and UUID.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/evm/evm_main.c | 31 ++++++++++++++++++++++++++++---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2555aa4501ae..565c36471408 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -898,9 +898,34 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
{
- if (strcmp(name, XATTR_NAME_EVM) == 0)
- return 1; /* Discard */
- return -EOPNOTSUPP;
+ struct evm_ima_xattr_data *xattr_data = NULL;
+ int rc;
+
+ if (strcmp(name, XATTR_NAME_EVM) != 0)
+ return -EOPNOTSUPP;
+
+ /* first need to know the sig type */
+ rc = vfs_getxattr_alloc(&nop_mnt_idmap, src, XATTR_NAME_EVM,
+ (char **)&xattr_data, 0, GFP_NOFS);
+ if (rc <= 0)
+ return -EPERM;
+
+ if (rc < offsetof(struct evm_ima_xattr_data, type) +
+ sizeof(xattr_data->type))
+ return -EPERM;
+
+ switch (xattr_data->type) {
+ case EVM_XATTR_PORTABLE_DIGSIG:
+ rc = 0; /* allow copy-up */
+ break;
+ case EVM_XATTR_HMAC:
+ case EVM_IMA_XATTR_DIGSIG:
+ default:
+ rc = 1; /* discard */
+ }
+
+ kfree(xattr_data);
+ return rc;
}
/*
--
2.43.0
Changes to file attributes (mode bits, uid, gid) on the lower layer are
not taken into account when d_backing_inode() is used when a file is
accessed on the overlay layer and this file has not yet been copied up.
This is because d_backing_inode() does not return the real inode of the
lower layer but instead returns the backing inode which in this case
holds wrong file attributes. Further, when CONFIG_OVERLAY_FS_METACOPY is
enabled and a copy-up is triggered due to file metadata changes, then
the metadata are held by the backing inode while the data are still held
by the real inode. Therefore, use d_inode(d_real(dentry, D_REAL_METADATA))
to get to the inode holding the file's metadata and use it to calculate
the metadata hash with.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/evm/evm_crypto.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index b1ffd4cc0b44..51e24a75742c 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
size_t req_xattr_value_len,
uint8_t type, struct evm_digest *data)
{
- struct inode *inode = d_backing_inode(dentry);
+ struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));
struct xattr_list *xattr;
struct shash_desc *desc;
size_t xattr_size = 0;
--
2.43.0
Rename the backing_inode variable to real_inode since it gets its value
from real_inode().
Suggested-by: Amir Goldstein <[email protected]>
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/ima/ima_main.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cc1217ac2c6f..f1a01d32b92a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -208,7 +208,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
u32 secid, char *buf, loff_t size, int mask,
enum ima_hooks func)
{
- struct inode *backing_inode, *inode = file_inode(file);
+ struct inode *real_inode, *inode = file_inode(file);
struct integrity_iint_cache *iint = NULL;
struct ima_template_desc *template_desc = NULL;
char *pathbuf = NULL;
@@ -285,14 +285,16 @@ static int process_measurement(struct file *file, const struct cred *cred,
iint->measured_pcrs = 0;
}
- /* Detect and re-evaluate changes made to the backing file. */
- backing_inode = d_real_inode(file_dentry(file));
- if (backing_inode != inode &&
+ /*
+ * Detect and re-evaluate changes made to the inode holding file data.
+ */
+ real_inode = d_real_inode(file_dentry(file));
+ if (real_inode != inode &&
(action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
- if (!IS_I_VERSION(backing_inode) ||
- backing_inode->i_sb->s_dev != iint->real_dev ||
- backing_inode->i_ino != iint->real_ino ||
- !inode_eq_iversion(backing_inode, iint->version)) {
+ if (!IS_I_VERSION(real_inode) ||
+ real_inode->i_sb->s_dev != iint->real_dev ||
+ real_inode->i_ino != iint->real_ino ||
+ !inode_eq_iversion(real_inode, iint->version)) {
iint->flags &= ~IMA_DONE_MASK;
iint->measured_pcrs = 0;
}
--
2.43.0
Rename is_unsupported_fs to is_unsupported_hmac_fs since only HMAC is
unsupported now on certain filesystems.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/evm/evm_main.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index ff659e622f4a..60ca7e9713ca 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -151,7 +151,7 @@ static int evm_find_protected_xattrs(struct dentry *dentry)
return count;
}
-static int is_unsupported_fs(struct dentry *dentry)
+static int is_unsupported_hmac_fs(struct dentry *dentry)
{
struct inode *inode = d_backing_inode(dentry);
@@ -196,7 +196,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
* On unsupported filesystems with EVM_INIT_X509 not enabled, skip
* signature verification.
*/
- if (!(evm_initialized & EVM_INIT_X509) && is_unsupported_fs(dentry))
+ if (!(evm_initialized & EVM_INIT_X509) &&
+ is_unsupported_hmac_fs(dentry))
return INTEGRITY_UNKNOWN;
/* if status is not PASS, try to check again - against -ENOMEM */
@@ -267,7 +268,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
} else if (!IS_RDONLY(inode) &&
!(inode->i_sb->s_readonly_remount) &&
!IS_IMMUTABLE(inode) &&
- !is_unsupported_fs(dentry)) {
+ !is_unsupported_hmac_fs(dentry)) {
evm_update_evmxattr(dentry, xattr_name,
xattr_value,
xattr_value_len);
@@ -510,12 +511,12 @@ static int evm_protect_xattr(struct mnt_idmap *idmap,
if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) {
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
- if (is_unsupported_fs(dentry))
+ if (is_unsupported_hmac_fs(dentry))
return -EPERM;
} else if (!evm_protected_xattr(xattr_name)) {
if (!posix_xattr_acl(xattr_name))
return 0;
- if (is_unsupported_fs(dentry))
+ if (is_unsupported_hmac_fs(dentry))
return 0;
evm_status = evm_verify_current_integrity(dentry);
@@ -523,7 +524,7 @@ static int evm_protect_xattr(struct mnt_idmap *idmap,
(evm_status == INTEGRITY_NOXATTRS))
return 0;
goto out;
- } else if (is_unsupported_fs(dentry))
+ } else if (is_unsupported_hmac_fs(dentry))
return 0;
evm_status = evm_verify_current_integrity(dentry);
@@ -782,7 +783,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
if (!(evm_initialized & EVM_INIT_HMAC))
return;
- if (is_unsupported_fs(dentry))
+ if (is_unsupported_hmac_fs(dentry))
return;
evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
@@ -849,7 +850,7 @@ int evm_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
return 0;
- if (is_unsupported_fs(dentry))
+ if (is_unsupported_hmac_fs(dentry))
return 0;
if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)))
@@ -898,7 +899,7 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
if (!(evm_initialized & EVM_INIT_HMAC))
return;
- if (is_unsupported_fs(dentry))
+ if (is_unsupported_hmac_fs(dentry))
return;
if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
--
2.43.0
Piggyback the resetting of EVM status on IMA's file content detection that
is triggered when a not-yet-copied-up file on the 'lower' layer was
changed. However, since EVM only cares about changes to the file metadata,
only reset the EVM status if the 'lower' layer file is also the one holding
the file metadata.
Note that in the case of a stacked filesystem (e.g., overlayfs) the iint
represents the file_inode() of a file on the overlay layer. The data in
the in iint must help detect file content (IMA) and file metadata (EVM)
changes occurring on the lower layer for as long as the content or
metadata have not been copied up yet. After copy-up the iit must continue
detecting them on the overlay layer.
Changes to the file metadata on the overlay layer are causing an EVM
status reset through existing evm_inode_post_sattr/setxattr/removexattr
functions *if* an iint for a file exist. An iint exists if the file is
'in (IMA) policy', meaning that IMA created an iint for the file's inode
since the file is covered by the IMA policy.
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 | 5 +++++
3 files changed, 20 insertions(+)
diff --git a/include/linux/evm.h b/include/linux/evm.h
index 840ffbdc2860..eade9fff7d0b 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
@@ -190,5 +192,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 565c36471408..81c94967f136 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 f1a01d32b92a..b6ba829c4e67 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"
@@ -297,6 +298,10 @@ static int process_measurement(struct file *file, const struct cred *cred,
!inode_eq_iversion(real_inode, iint->version)) {
iint->flags &= ~IMA_DONE_MASK;
iint->measured_pcrs = 0;
+
+ if (real_inode == d_inode(d_real(file_dentry(file),
+ D_REAL_METADATA)))
+ evm_reset_cache_status(file_dentry(file), iint);
}
}
--
2.43.0
Unsupported filesystems currently do not enforce any signatures. Add
support for signature enforcement of the "original" and "portable &
immutable" signatures when EVM_INIT_X509 is enabled.
The "original" signature type contains filesystem specific metadata.
Thus it cannot be copied up and verified. However with EVM_INIT_X509
and EVM_ALLOW_METADATA_WRITES enabled, the "original" file signature
may be written.
When EVM_ALLOW_METADATA_WRITES is not set or once it is removed from
/sys/kernel/security/evm by setting EVM_INIT_HMAC for example, it is not
possible to write or remove xattrs on the overlay filesystem.
This change still prevents EVM from writing HMAC signatures on
unsupported filesystem when EVM_INIT_HMAC is enabled.
Signed-off-by: Stefan Berger <[email protected]>
---
security/integrity/evm/evm_main.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 81c94967f136..c3bd88aba78c 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -192,7 +192,11 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
iint->evm_status == INTEGRITY_PASS_IMMUTABLE))
return iint->evm_status;
- if (is_unsupported_fs(dentry))
+ /*
+ * On unsupported filesystems with EVM_INIT_X509 not enabled, skip
+ * signature verification.
+ */
+ if (!(evm_initialized & EVM_INIT_X509) && is_unsupported_fs(dentry))
return INTEGRITY_UNKNOWN;
/* if status is not PASS, try to check again - against -ENOMEM */
@@ -262,7 +266,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
evm_status = INTEGRITY_PASS_IMMUTABLE;
} else if (!IS_RDONLY(inode) &&
!(inode->i_sb->s_readonly_remount) &&
- !IS_IMMUTABLE(inode)) {
+ !IS_IMMUTABLE(inode) &&
+ !is_unsupported_fs(dentry)) {
evm_update_evmxattr(dentry, xattr_name,
xattr_value,
xattr_value_len);
@@ -422,9 +427,6 @@ enum integrity_status evm_verifyxattr(struct dentry *dentry,
if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
return INTEGRITY_UNKNOWN;
- if (is_unsupported_fs(dentry))
- return INTEGRITY_UNKNOWN;
-
if (!iint) {
iint = integrity_iint_find(d_backing_inode(dentry));
if (!iint)
--
2.43.0
Hi Stefan,
kernel test robot noticed the following build errors:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on pcmoore-selinux/next linus/master v6.8-rc3 next-20240206]
[cannot apply to mszeredi-vfs/overlayfs-next mszeredi-vfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/ima-Rename-backing_inode-to-real_inode/20240206-022848
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link: https://lore.kernel.org/r/20240205182506.3569743-5-stefanb%40linux.ibm.com
patch subject: [PATCH v2 4/9] ima: Reset EVM status upon detecting changes to the real file
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240206/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
security/integrity/ima/ima_main.c: In function 'process_measurement':
>> security/integrity/ima/ima_main.c:303:58: error: 'D_REAL_METADATA' undeclared (first use in this function)
303 | D_REAL_METADATA)))
| ^~~~~~~~~~~~~~~
security/integrity/ima/ima_main.c:303:58: note: each undeclared identifier is reported only once for each function it appears in
vim +/D_REAL_METADATA +303 security/integrity/ima/ima_main.c
207
208 static int process_measurement(struct file *file, const struct cred *cred,
209 u32 secid, char *buf, loff_t size, int mask,
210 enum ima_hooks func)
211 {
212 struct inode *real_inode, *inode = file_inode(file);
213 struct integrity_iint_cache *iint = NULL;
214 struct ima_template_desc *template_desc = NULL;
215 char *pathbuf = NULL;
216 char filename[NAME_MAX];
217 const char *pathname = NULL;
218 int rc = 0, action, must_appraise = 0;
219 int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
220 struct evm_ima_xattr_data *xattr_value = NULL;
221 struct modsig *modsig = NULL;
222 int xattr_len = 0;
223 bool violation_check;
224 enum hash_algo hash_algo;
225 unsigned int allowed_algos = 0;
226
227 if (!ima_policy_flag || !S_ISREG(inode->i_mode))
228 return 0;
229
230 /* Return an IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT action
231 * bitmask based on the appraise/audit/measurement policy.
232 * Included is the appraise submask.
233 */
234 action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
235 mask, func, &pcr, &template_desc, NULL,
236 &allowed_algos);
237 violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
238 func == MMAP_CHECK_REQPROT) &&
239 (ima_policy_flag & IMA_MEASURE));
240 if (!action && !violation_check)
241 return 0;
242
243 must_appraise = action & IMA_APPRAISE;
244
245 /* Is the appraise rule hook specific? */
246 if (action & IMA_FILE_APPRAISE)
247 func = FILE_CHECK;
248
249 inode_lock(inode);
250
251 if (action) {
252 iint = integrity_inode_get(inode);
253 if (!iint)
254 rc = -ENOMEM;
255 }
256
257 if (!rc && violation_check)
258 ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
259 &pathbuf, &pathname, filename);
260
261 inode_unlock(inode);
262
263 if (rc)
264 goto out;
265 if (!action)
266 goto out;
267
268 mutex_lock(&iint->mutex);
269
270 if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
271 /* reset appraisal flags if ima_inode_post_setattr was called */
272 iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
273 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
274 IMA_NONACTION_FLAGS);
275
276 /*
277 * Re-evaulate the file if either the xattr has changed or the
278 * kernel has no way of detecting file change on the filesystem.
279 * (Limited to privileged mounted filesystems.)
280 */
281 if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
282 ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
283 !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) &&
284 !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) {
285 iint->flags &= ~IMA_DONE_MASK;
286 iint->measured_pcrs = 0;
287 }
288
289 /*
290 * Detect and re-evaluate changes made to the inode holding file data.
291 */
292 real_inode = d_real_inode(file_dentry(file));
293 if (real_inode != inode &&
294 (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
295 if (!IS_I_VERSION(real_inode) ||
296 real_inode->i_sb->s_dev != iint->real_dev ||
297 real_inode->i_ino != iint->real_ino ||
298 !inode_eq_iversion(real_inode, iint->version)) {
299 iint->flags &= ~IMA_DONE_MASK;
300 iint->measured_pcrs = 0;
301
302 if (real_inode == d_inode(d_real(file_dentry(file),
> 303 D_REAL_METADATA)))
304 evm_reset_cache_status(file_dentry(file), iint);
305 }
306 }
307
308 /* Determine if already appraised/measured based on bitmask
309 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
310 * IMA_AUDIT, IMA_AUDITED)
311 */
312 iint->flags |= action;
313 action &= IMA_DO_MASK;
314 action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
315
316 /* If target pcr is already measured, unset IMA_MEASURE action */
317 if ((action & IMA_MEASURE) && (iint->measured_pcrs & (0x1 << pcr)))
318 action ^= IMA_MEASURE;
319
320 /* HASH sets the digital signature and update flags, nothing else */
321 if ((action & IMA_HASH) &&
322 !(test_bit(IMA_DIGSIG, &iint->atomic_flags))) {
323 xattr_len = ima_read_xattr(file_dentry(file),
324 &xattr_value, xattr_len);
325 if ((xattr_value && xattr_len > 2) &&
326 (xattr_value->type == EVM_IMA_XATTR_DIGSIG))
327 set_bit(IMA_DIGSIG, &iint->atomic_flags);
328 iint->flags |= IMA_HASHED;
329 action ^= IMA_HASH;
330 set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
331 }
332
333 /* Nothing to do, just return existing appraised status */
334 if (!action) {
335 if (must_appraise) {
336 rc = mmap_violation_check(func, file, &pathbuf,
337 &pathname, filename);
338 if (!rc)
339 rc = ima_get_cache_status(iint, func);
340 }
341 goto out_locked;
342 }
343
344 if ((action & IMA_APPRAISE_SUBMASK) ||
345 strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
346 /* read 'security.ima' */
347 xattr_len = ima_read_xattr(file_dentry(file),
348 &xattr_value, xattr_len);
349
350 /*
351 * Read the appended modsig if allowed by the policy, and allow
352 * an additional measurement list entry, if needed, based on the
353 * template format and whether the file was already measured.
354 */
355 if (iint->flags & IMA_MODSIG_ALLOWED) {
356 rc = ima_read_modsig(func, buf, size, &modsig);
357
358 if (!rc && ima_template_has_modsig(template_desc) &&
359 iint->flags & IMA_MEASURED)
360 action |= IMA_MEASURE;
361 }
362 }
363
364 hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
365
366 rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
367 if (rc != 0 && rc != -EBADF && rc != -EINVAL)
368 goto out_locked;
369
370 if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
371 pathname = ima_d_path(&file->f_path, &pathbuf, filename);
372
373 if (action & IMA_MEASURE)
374 ima_store_measurement(iint, file, pathname,
375 xattr_value, xattr_len, modsig, pcr,
376 template_desc);
377 if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
378 rc = ima_check_blacklist(iint, modsig, pcr);
379 if (rc != -EPERM) {
380 inode_lock(inode);
381 rc = ima_appraise_measurement(func, iint, file,
382 pathname, xattr_value,
383 xattr_len, modsig);
384 inode_unlock(inode);
385 }
386 if (!rc)
387 rc = mmap_violation_check(func, file, &pathbuf,
388 &pathname, filename);
389 }
390 if (action & IMA_AUDIT)
391 ima_audit_measurement(iint, pathname);
392
393 if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
394 rc = 0;
395
396 /* Ensure the digest was generated using an allowed algorithm */
397 if (rc == 0 && must_appraise && allowed_algos != 0 &&
398 (allowed_algos & (1U << hash_algo)) == 0) {
399 rc = -EACCES;
400
401 integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
402 pathname, "collect_data",
403 "denied-hash-algorithm", rc, 0);
404 }
405 out_locked:
406 if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) &&
407 !(iint->flags & IMA_NEW_FILE))
408 rc = -EACCES;
409 mutex_unlock(&iint->mutex);
410 kfree(xattr_value);
411 ima_free_modsig(modsig);
412 out:
413 if (pathbuf)
414 __putname(pathbuf);
415 if (must_appraise) {
416 if (rc && (ima_appraise & IMA_APPRAISE_ENFORCE))
417 return -EACCES;
418 if (file->f_mode & FMODE_WRITE)
419 set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
420 }
421 return 0;
422 }
423
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <[email protected]> wrote:
>
> Now that EVM supports RSA signatures for previously completely
> unsupported filesystems rename the flag SB_I_EVM_UNSUPPORTED to
> SB_I_EVM_HMAC_UNSUPPORTED to reflect that only HMAC is not supported.
>
> Suggested-by: Amir Goldstein <[email protected]>
> Suggested-by: Mimi Zohar <[email protected]>
> Signed-off-by: Stefan Berger <[email protected]>
Acked-by: Amir Goldstein <[email protected]>
> ---
> fs/overlayfs/super.c | 2 +-
> include/linux/fs.h | 2 +-
> security/integrity/evm/evm_main.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 460126b7e1cd..db132d437e14 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1445,7 +1445,7 @@ int ovl_fill_super(struct super_block *sb, struct fs_context *fc)
> * lead to unexpected results.
> */
> sb->s_iflags |= SB_I_NOUMASK;
> - sb->s_iflags |= SB_I_EVM_UNSUPPORTED;
> + sb->s_iflags |= SB_I_EVM_HMAC_UNSUPPORTED;
>
> err = -ENOMEM;
> root_dentry = ovl_get_root(sb, ctx->upper.dentry, oe);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1823a93202bd..37306a09b4dc 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1177,7 +1177,7 @@ extern int send_sigurg(struct fown_struct *fown);
> #define SB_I_USERNS_VISIBLE 0x00000010 /* fstype already mounted */
> #define SB_I_IMA_UNVERIFIABLE_SIGNATURE 0x00000020
> #define SB_I_UNTRUSTED_MOUNTER 0x00000040
> -#define SB_I_EVM_UNSUPPORTED 0x00000080
> +#define SB_I_EVM_HMAC_UNSUPPORTED 0x00000080
>
> #define SB_I_SKIP_SYNC 0x00000100 /* Skip superblock at global sync */
> #define SB_I_PERSB_BDI 0x00000200 /* has a per-sb bdi */
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index c3bd88aba78c..ff659e622f4a 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -155,7 +155,7 @@ static int is_unsupported_fs(struct dentry *dentry)
> {
> struct inode *inode = d_backing_inode(dentry);
>
> - if (inode->i_sb->s_iflags & SB_I_EVM_UNSUPPORTED) {
> + if (inode->i_sb->s_iflags & SB_I_EVM_HMAC_UNSUPPORTED) {
> pr_info_once("%s not supported\n", inode->i_sb->s_type->name);
> return 1;
> }
> --
> 2.43.0
>
On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <[email protected]> wrote:
>
> Rename the backing_inode variable to real_inode since it gets its value
> from real_inode().
>
> Suggested-by: Amir Goldstein <[email protected]>
> Signed-off-by: Stefan Berger <[email protected]>
Acked-by: Amir Goldstein <[email protected]>
> ---
> security/integrity/ima/ima_main.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index cc1217ac2c6f..f1a01d32b92a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -208,7 +208,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
> u32 secid, char *buf, loff_t size, int mask,
> enum ima_hooks func)
> {
> - struct inode *backing_inode, *inode = file_inode(file);
> + struct inode *real_inode, *inode = file_inode(file);
> struct integrity_iint_cache *iint = NULL;
> struct ima_template_desc *template_desc = NULL;
> char *pathbuf = NULL;
> @@ -285,14 +285,16 @@ static int process_measurement(struct file *file, const struct cred *cred,
> iint->measured_pcrs = 0;
> }
>
> - /* Detect and re-evaluate changes made to the backing file. */
> - backing_inode = d_real_inode(file_dentry(file));
> - if (backing_inode != inode &&
> + /*
> + * Detect and re-evaluate changes made to the inode holding file data.
> + */
> + real_inode = d_real_inode(file_dentry(file));
> + if (real_inode != inode &&
> (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
> - if (!IS_I_VERSION(backing_inode) ||
> - backing_inode->i_sb->s_dev != iint->real_dev ||
> - backing_inode->i_ino != iint->real_ino ||
> - !inode_eq_iversion(backing_inode, iint->version)) {
> + if (!IS_I_VERSION(real_inode) ||
> + real_inode->i_sb->s_dev != iint->real_dev ||
> + real_inode->i_ino != iint->real_ino ||
> + !inode_eq_iversion(real_inode, iint->version)) {
> iint->flags &= ~IMA_DONE_MASK;
> iint->measured_pcrs = 0;
> }
> --
> 2.43.0
>
On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <[email protected]> wrote:
>
> Copying up xattrs is solely based on the security xattr name. For finer
> granularity add a dentry parameter to the security_inode_copy_up_xattr
> hook definition, allowing decisions to be based on the xattr content as
> well.
>
> Signed-off-by: Stefan Berger <[email protected]>
For ovl part
Acked-by: Amir Goldstein <[email protected]>
> ---
> fs/overlayfs/copy_up.c | 2 +-
> include/linux/evm.h | 5 +++--
> include/linux/lsm_hook_defs.h | 3 ++-
> include/linux/security.h | 4 ++--
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 7 ++++---
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> 8 files changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8586e2f5d243..de20fe9333c0 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -114,7 +114,7 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
> if (ovl_is_private_xattr(sb, name))
> continue;
>
> - error = security_inode_copy_up_xattr(name);
> + error = security_inode_copy_up_xattr(old, name);
> if (error < 0 && error != -EOPNOTSUPP)
> break;
> if (error == 1) {
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 36ec884320d9..840ffbdc2860 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -31,7 +31,7 @@ extern void evm_inode_post_setxattr(struct dentry *dentry,
> const char *xattr_name,
> const void *xattr_value,
> size_t xattr_value_len);
> -extern int evm_inode_copy_up_xattr(const char *name);
> +extern int evm_inode_copy_up_xattr(struct dentry *dentry, const char *name);
> extern int evm_inode_removexattr(struct mnt_idmap *idmap,
> struct dentry *dentry, const char *xattr_name);
> extern void evm_inode_post_removexattr(struct dentry *dentry,
> @@ -118,7 +118,8 @@ static inline void evm_inode_post_setxattr(struct dentry *dentry,
> return;
> }
>
> -static inline int evm_inode_copy_up_xattr(const char *name)
> +static inline int evm_inode_copy_up_xattr(struct dentry *dentry,
> + const char *name)
> {
> return 0;
> }
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 185924c56378..7dd61f51d84a 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -163,7 +163,8 @@ LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
> size_t buffer_size)
> LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
> LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
> -LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, const char *name)
> +LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, struct dentry *src,
> + const char *name)
> LSM_HOOK(int, 0, kernfs_init_security, struct kernfs_node *kn_dir,
> struct kernfs_node *kn)
> LSM_HOOK(int, 0, file_permission, struct file *file, int mask)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d0eb20f90b26..9fc9ca6284d6 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -387,7 +387,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
> int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
> void security_inode_getsecid(struct inode *inode, u32 *secid);
> int security_inode_copy_up(struct dentry *src, struct cred **new);
> -int security_inode_copy_up_xattr(const char *name);
> +int security_inode_copy_up_xattr(struct dentry *src, const char *name);
> int security_kernfs_init_security(struct kernfs_node *kn_dir,
> struct kernfs_node *kn);
> int security_file_permission(struct file *file, int mask);
> @@ -980,7 +980,7 @@ static inline int security_kernfs_init_security(struct kernfs_node *kn_dir,
> return 0;
> }
>
> -static inline int security_inode_copy_up_xattr(const char *name)
> +static inline int security_inode_copy_up_xattr(struct dentry *src, const char *name)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cc7956d7878b..2555aa4501ae 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -896,7 +896,7 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> evm_update_evmxattr(dentry, NULL, NULL, 0);
> }
>
> -int evm_inode_copy_up_xattr(const char *name)
> +int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
> {
> if (strcmp(name, XATTR_NAME_EVM) == 0)
> return 1; /* Discard */
> diff --git a/security/security.c b/security/security.c
> index 0144a98d3712..ee63863c1dc0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2596,6 +2596,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
>
> /**
> * security_inode_copy_up_xattr() - Filter xattrs in an overlayfs copy-up op
> + * @src: union dentry of copy-up file
> * @name: xattr name
> *
> * Filter the xattrs being copied up when a unioned file is copied up from a
> @@ -2606,7 +2607,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
> * if the security module does not know about attribute, or a negative
> * error code to abort the copy up.
> */
> -int security_inode_copy_up_xattr(const char *name)
> +int security_inode_copy_up_xattr(struct dentry *src, const char *name)
> {
> struct security_hook_list *hp;
> int rc;
> @@ -2618,12 +2619,12 @@ int security_inode_copy_up_xattr(const char *name)
> */
> hlist_for_each_entry(hp,
> &security_hook_heads.inode_copy_up_xattr, list) {
> - rc = hp->hook.inode_copy_up_xattr(name);
> + rc = hp->hook.inode_copy_up_xattr(src, name);
> if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
> return rc;
> }
>
> - return evm_inode_copy_up_xattr(name);
> + return evm_inode_copy_up_xattr(src, name);
> }
> EXPORT_SYMBOL(security_inode_copy_up_xattr);
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a6bf90ace84c..ebb8876837c6 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3530,7 +3530,7 @@ static int selinux_inode_copy_up(struct dentry *src, struct cred **new)
> return 0;
> }
>
> -static int selinux_inode_copy_up_xattr(const char *name)
> +static int selinux_inode_copy_up_xattr(struct dentry *dentry, const char *name)
> {
> /* The copy_up hook above sets the initial context on an inode, but we
> * don't then want to overwrite it by blindly copying all the lower
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 0fdbf04cc258..bffca165f07f 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4873,7 +4873,7 @@ static int smack_inode_copy_up(struct dentry *dentry, struct cred **new)
> return 0;
> }
>
> -static int smack_inode_copy_up_xattr(const char *name)
> +static int smack_inode_copy_up_xattr(struct dentry *src, const char *name)
> {
> /*
> * Return 1 if this is the smack access Smack attribute.
> --
> 2.43.0
>
On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <[email protected]> wrote:
>
> Piggyback the resetting of EVM status on IMA's file content detection that
> is triggered when a not-yet-copied-up file on the 'lower' layer was
> changed. However, since EVM only cares about changes to the file metadata,
> only reset the EVM status if the 'lower' layer file is also the one holding
> the file metadata.
>
> Note that in the case of a stacked filesystem (e.g., overlayfs) the iint
> represents the file_inode() of a file on the overlay layer. The data in
> the in iint must help detect file content (IMA) and file metadata (EVM)
> changes occurring on the lower layer for as long as the content or
> metadata have not been copied up yet. After copy-up the iit must continue
> detecting them on the overlay layer.
>
> Changes to the file metadata on the overlay layer are causing an EVM
> status reset through existing evm_inode_post_sattr/setxattr/removexattr
> functions *if* an iint for a file exist. An iint exists if the file is
> 'in (IMA) policy', meaning that IMA created an iint for the file's inode
> since the file is covered by the IMA policy.
>
> 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 | 5 +++++
> 3 files changed, 20 insertions(+)
>
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 840ffbdc2860..eade9fff7d0b 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
> @@ -190,5 +192,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 565c36471408..81c94967f136 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))
Is this really needed?
You get here after checking (real_inode != inode) already
> + 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 f1a01d32b92a..b6ba829c4e67 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"
>
> @@ -297,6 +298,10 @@ static int process_measurement(struct file *file, const struct cred *cred,
> !inode_eq_iversion(real_inode, iint->version)) {
> iint->flags &= ~IMA_DONE_MASK;
> iint->measured_pcrs = 0;
> +
> + if (real_inode == d_inode(d_real(file_dentry(file),
> + D_REAL_METADATA)))
> + evm_reset_cache_status(file_dentry(file), iint);
Technically, you'd also need to store iint->real_meta_{dev,ino}
when calculating EVM to be sure if the metadata inode had changed,
because there is a possibility that file was not copied up yet, but the file
is a metacopy in a middle layer and the lower data is in another layer.
Think file metadata was copied from lower to upper layer, then the
upper layer was made a middle layer and another upper layer added
on top of it.
In this situation, real_inode is in the lower layer, real_meta_inode is in
the middle layer and after copy up of metadata, real_meta_inode will
become in the upper layer.
Not sure if this use case is interesting to EVM.
Thanks,
Amir.
On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:
> On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <[email protected]> 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 <[email protected]>
> > ---
> > 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.
>
> 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
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?
--
Jeff Layton <[email protected]>
On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <[email protected]> wrote:
>
> Changes to file attributes (mode bits, uid, gid) on the lower layer are
> not taken into account when d_backing_inode() is used when a file is
> accessed on the overlay layer and this file has not yet been copied up.
> This is because d_backing_inode() does not return the real inode of the
> lower layer but instead returns the backing inode which in this case
> holds wrong file attributes. Further, when CONFIG_OVERLAY_FS_METACOPY is
> enabled and a copy-up is triggered due to file metadata changes, then
> the metadata are held by the backing inode while the data are still held
> by the real inode. Therefore, use d_inode(d_real(dentry, D_REAL_METADATA))
> to get to the inode holding the file's metadata and use it to calculate
> the metadata hash with.
>
> Signed-off-by: Stefan Berger <[email protected]>
Acked-by: Amir Goldstein <[email protected]>
> ---
> security/integrity/evm/evm_crypto.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index b1ffd4cc0b44..51e24a75742c 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -223,7 +223,7 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
> size_t req_xattr_value_len,
> uint8_t type, struct evm_digest *data)
> {
> - struct inode *inode = d_backing_inode(dentry);
> + struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));
> struct xattr_list *xattr;
> struct shash_desc *desc;
> size_t xattr_size = 0;
> --
> 2.43.0
>
On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <[email protected]> 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 <[email protected]>
> ---
> 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);
> }
>
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.
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."
Jeff?
Thanks,
Amir.
Hi Stefan,
kernel test robot noticed the following build errors:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on pcmoore-selinux/next linus/master v6.8-rc3 next-20240206]
[cannot apply to mszeredi-vfs/overlayfs-next mszeredi-vfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/ima-Rename-backing_inode-to-real_inode/20240206-022848
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link: https://lore.kernel.org/r/20240205182506.3569743-6-stefanb%40linux.ibm.com
patch subject: [PATCH v2 5/9] evm: Use the inode holding the metadata to calculate metadata hash
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240207/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240207/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
security/integrity/evm/evm_crypto.c: In function 'evm_calc_hmac_or_hash':
>> security/integrity/evm/evm_crypto.c:226:54: error: 'D_REAL_METADATA' undeclared (first use in this function)
226 | struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));
| ^~~~~~~~~~~~~~~
security/integrity/evm/evm_crypto.c:226:54: note: each undeclared identifier is reported only once for each function it appears in
vim +/D_REAL_METADATA +226 security/integrity/evm/evm_crypto.c
212
213 /*
214 * Calculate the HMAC value across the set of protected security xattrs.
215 *
216 * Instead of retrieving the requested xattr, for performance, calculate
217 * the hmac using the requested xattr value. Don't alloc/free memory for
218 * each xattr, but attempt to re-use the previously allocated memory.
219 */
220 static int evm_calc_hmac_or_hash(struct dentry *dentry,
221 const char *req_xattr_name,
222 const char *req_xattr_value,
223 size_t req_xattr_value_len,
224 uint8_t type, struct evm_digest *data)
225 {
> 226 struct inode *inode = d_inode(d_real(dentry, D_REAL_METADATA));
227 struct xattr_list *xattr;
228 struct shash_desc *desc;
229 size_t xattr_size = 0;
230 char *xattr_value = NULL;
231 int error;
232 int size, user_space_size;
233 bool ima_present = false;
234
235 if (!(inode->i_opflags & IOP_XATTR) ||
236 inode->i_sb->s_user_ns != &init_user_ns)
237 return -EOPNOTSUPP;
238
239 desc = init_desc(type, data->hdr.algo);
240 if (IS_ERR(desc))
241 return PTR_ERR(desc);
242
243 data->hdr.length = crypto_shash_digestsize(desc->tfm);
244
245 error = -ENODATA;
246 list_for_each_entry_lockless(xattr, &evm_config_xattrnames, list) {
247 bool is_ima = false;
248
249 if (strcmp(xattr->name, XATTR_NAME_IMA) == 0)
250 is_ima = true;
251
252 /*
253 * Skip non-enabled xattrs for locally calculated
254 * signatures/HMACs.
255 */
256 if (type != EVM_XATTR_PORTABLE_DIGSIG && !xattr->enabled)
257 continue;
258
259 if ((req_xattr_name && req_xattr_value)
260 && !strcmp(xattr->name, req_xattr_name)) {
261 error = 0;
262 crypto_shash_update(desc, (const u8 *)req_xattr_value,
263 req_xattr_value_len);
264 if (is_ima)
265 ima_present = true;
266
267 dump_security_xattr(req_xattr_name,
268 req_xattr_value,
269 req_xattr_value_len);
270 continue;
271 }
272 size = vfs_getxattr_alloc(&nop_mnt_idmap, dentry, xattr->name,
273 &xattr_value, xattr_size, GFP_NOFS);
274 if (size == -ENOMEM) {
275 error = -ENOMEM;
276 goto out;
277 }
278 if (size < 0)
279 continue;
280
281 user_space_size = vfs_getxattr(&nop_mnt_idmap, dentry,
282 xattr->name, NULL, 0);
283 if (user_space_size != size)
284 pr_debug("file %s: xattr %s size mismatch (kernel: %d, user: %d)\n",
285 dentry->d_name.name, xattr->name, size,
286 user_space_size);
287 error = 0;
288 xattr_size = size;
289 crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
290 if (is_ima)
291 ima_present = true;
292
293 dump_security_xattr(xattr->name, xattr_value, xattr_size);
294 }
295 hmac_add_misc(desc, inode, type, data->digest);
296
297 /* Portable EVM signatures must include an IMA hash */
298 if (type == EVM_XATTR_PORTABLE_DIGSIG && !ima_present)
299 error = -EPERM;
300 out:
301 kfree(xattr_value);
302 kfree(desc);
303 return error;
304 }
305
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Stefan,
kernel test robot noticed the following build errors:
[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on pcmoore-selinux/next linus/master v6.8-rc3 next-20240206]
[cannot apply to mszeredi-vfs/overlayfs-next mszeredi-vfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/ima-Rename-backing_inode-to-real_inode/20240206-022848
base: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link: https://lore.kernel.org/r/20240205182506.3569743-5-stefanb%40linux.ibm.com
patch subject: [PATCH v2 4/9] ima: Reset EVM status upon detecting changes to the real file
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240207/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240207/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> security/integrity/ima/ima_main.c:303:9: error: use of undeclared identifier 'D_REAL_METADATA'
303 | D_REAL_METADATA)))
| ^
1 error generated.
vim +/D_REAL_METADATA +303 security/integrity/ima/ima_main.c
207
208 static int process_measurement(struct file *file, const struct cred *cred,
209 u32 secid, char *buf, loff_t size, int mask,
210 enum ima_hooks func)
211 {
212 struct inode *real_inode, *inode = file_inode(file);
213 struct integrity_iint_cache *iint = NULL;
214 struct ima_template_desc *template_desc = NULL;
215 char *pathbuf = NULL;
216 char filename[NAME_MAX];
217 const char *pathname = NULL;
218 int rc = 0, action, must_appraise = 0;
219 int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
220 struct evm_ima_xattr_data *xattr_value = NULL;
221 struct modsig *modsig = NULL;
222 int xattr_len = 0;
223 bool violation_check;
224 enum hash_algo hash_algo;
225 unsigned int allowed_algos = 0;
226
227 if (!ima_policy_flag || !S_ISREG(inode->i_mode))
228 return 0;
229
230 /* Return an IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT action
231 * bitmask based on the appraise/audit/measurement policy.
232 * Included is the appraise submask.
233 */
234 action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
235 mask, func, &pcr, &template_desc, NULL,
236 &allowed_algos);
237 violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
238 func == MMAP_CHECK_REQPROT) &&
239 (ima_policy_flag & IMA_MEASURE));
240 if (!action && !violation_check)
241 return 0;
242
243 must_appraise = action & IMA_APPRAISE;
244
245 /* Is the appraise rule hook specific? */
246 if (action & IMA_FILE_APPRAISE)
247 func = FILE_CHECK;
248
249 inode_lock(inode);
250
251 if (action) {
252 iint = integrity_inode_get(inode);
253 if (!iint)
254 rc = -ENOMEM;
255 }
256
257 if (!rc && violation_check)
258 ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
259 &pathbuf, &pathname, filename);
260
261 inode_unlock(inode);
262
263 if (rc)
264 goto out;
265 if (!action)
266 goto out;
267
268 mutex_lock(&iint->mutex);
269
270 if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
271 /* reset appraisal flags if ima_inode_post_setattr was called */
272 iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
273 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
274 IMA_NONACTION_FLAGS);
275
276 /*
277 * Re-evaulate the file if either the xattr has changed or the
278 * kernel has no way of detecting file change on the filesystem.
279 * (Limited to privileged mounted filesystems.)
280 */
281 if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
282 ((inode->i_sb->s_iflags & SB_I_IMA_UNVERIFIABLE_SIGNATURE) &&
283 !(inode->i_sb->s_iflags & SB_I_UNTRUSTED_MOUNTER) &&
284 !(action & IMA_FAIL_UNVERIFIABLE_SIGS))) {
285 iint->flags &= ~IMA_DONE_MASK;
286 iint->measured_pcrs = 0;
287 }
288
289 /*
290 * Detect and re-evaluate changes made to the inode holding file data.
291 */
292 real_inode = d_real_inode(file_dentry(file));
293 if (real_inode != inode &&
294 (action & IMA_DO_MASK) && (iint->flags & IMA_DONE_MASK)) {
295 if (!IS_I_VERSION(real_inode) ||
296 real_inode->i_sb->s_dev != iint->real_dev ||
297 real_inode->i_ino != iint->real_ino ||
298 !inode_eq_iversion(real_inode, iint->version)) {
299 iint->flags &= ~IMA_DONE_MASK;
300 iint->measured_pcrs = 0;
301
302 if (real_inode == d_inode(d_real(file_dentry(file),
> 303 D_REAL_METADATA)))
304 evm_reset_cache_status(file_dentry(file), iint);
305 }
306 }
307
308 /* Determine if already appraised/measured based on bitmask
309 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,
310 * IMA_AUDIT, IMA_AUDITED)
311 */
312 iint->flags |= action;
313 action &= IMA_DO_MASK;
314 action &= ~((iint->flags & (IMA_DONE_MASK ^ IMA_MEASURED)) >> 1);
315
316 /* If target pcr is already measured, unset IMA_MEASURE action */
317 if ((action & IMA_MEASURE) && (iint->measured_pcrs & (0x1 << pcr)))
318 action ^= IMA_MEASURE;
319
320 /* HASH sets the digital signature and update flags, nothing else */
321 if ((action & IMA_HASH) &&
322 !(test_bit(IMA_DIGSIG, &iint->atomic_flags))) {
323 xattr_len = ima_read_xattr(file_dentry(file),
324 &xattr_value, xattr_len);
325 if ((xattr_value && xattr_len > 2) &&
326 (xattr_value->type == EVM_IMA_XATTR_DIGSIG))
327 set_bit(IMA_DIGSIG, &iint->atomic_flags);
328 iint->flags |= IMA_HASHED;
329 action ^= IMA_HASH;
330 set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
331 }
332
333 /* Nothing to do, just return existing appraised status */
334 if (!action) {
335 if (must_appraise) {
336 rc = mmap_violation_check(func, file, &pathbuf,
337 &pathname, filename);
338 if (!rc)
339 rc = ima_get_cache_status(iint, func);
340 }
341 goto out_locked;
342 }
343
344 if ((action & IMA_APPRAISE_SUBMASK) ||
345 strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
346 /* read 'security.ima' */
347 xattr_len = ima_read_xattr(file_dentry(file),
348 &xattr_value, xattr_len);
349
350 /*
351 * Read the appended modsig if allowed by the policy, and allow
352 * an additional measurement list entry, if needed, based on the
353 * template format and whether the file was already measured.
354 */
355 if (iint->flags & IMA_MODSIG_ALLOWED) {
356 rc = ima_read_modsig(func, buf, size, &modsig);
357
358 if (!rc && ima_template_has_modsig(template_desc) &&
359 iint->flags & IMA_MEASURED)
360 action |= IMA_MEASURE;
361 }
362 }
363
364 hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
365
366 rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
367 if (rc != 0 && rc != -EBADF && rc != -EINVAL)
368 goto out_locked;
369
370 if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
371 pathname = ima_d_path(&file->f_path, &pathbuf, filename);
372
373 if (action & IMA_MEASURE)
374 ima_store_measurement(iint, file, pathname,
375 xattr_value, xattr_len, modsig, pcr,
376 template_desc);
377 if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
378 rc = ima_check_blacklist(iint, modsig, pcr);
379 if (rc != -EPERM) {
380 inode_lock(inode);
381 rc = ima_appraise_measurement(func, iint, file,
382 pathname, xattr_value,
383 xattr_len, modsig);
384 inode_unlock(inode);
385 }
386 if (!rc)
387 rc = mmap_violation_check(func, file, &pathbuf,
388 &pathname, filename);
389 }
390 if (action & IMA_AUDIT)
391 ima_audit_measurement(iint, pathname);
392
393 if ((file->f_flags & O_DIRECT) && (iint->flags & IMA_PERMIT_DIRECTIO))
394 rc = 0;
395
396 /* Ensure the digest was generated using an allowed algorithm */
397 if (rc == 0 && must_appraise && allowed_algos != 0 &&
398 (allowed_algos & (1U << hash_algo)) == 0) {
399 rc = -EACCES;
400
401 integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
402 pathname, "collect_data",
403 "denied-hash-algorithm", rc, 0);
404 }
405 out_locked:
406 if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) &&
407 !(iint->flags & IMA_NEW_FILE))
408 rc = -EACCES;
409 mutex_unlock(&iint->mutex);
410 kfree(xattr_value);
411 ima_free_modsig(modsig);
412 out:
413 if (pathbuf)
414 __putname(pathbuf);
415 if (must_appraise) {
416 if (rc && (ima_appraise & IMA_APPRAISE_ENFORCE))
417 return -EACCES;
418 if (file->f_mode & FMODE_WRITE)
419 set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
420 }
421 return 0;
422 }
423
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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.
On Mon, Feb 5, 2024 at 1:25 PM Stefan Berger <[email protected]> wrote:
>
> Copying up xattrs is solely based on the security xattr name. For finer
> granularity add a dentry parameter to the security_inode_copy_up_xattr
> hook definition, allowing decisions to be based on the xattr content as
> well.
>
> Signed-off-by: Stefan Berger <[email protected]>
> ---
> fs/overlayfs/copy_up.c | 2 +-
> include/linux/evm.h | 5 +++--
> include/linux/lsm_hook_defs.h | 3 ++-
> include/linux/security.h | 4 ++--
> security/integrity/evm/evm_main.c | 2 +-
> security/security.c | 7 ++++---
> security/selinux/hooks.c | 2 +-
> security/smack/smack_lsm.c | 2 +-
> 8 files changed, 15 insertions(+), 12 deletions(-)
Acked-by: Paul Moore <[email protected]> (LSM,SELinux)
--
paul-moore.com