[PATCH v2 0/9] evm: Support signatures on stacked filesystem

[PATCH v2 0/9] evm: Support signatures on stacked filesystem

[Stefan Berger]

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

[PATCH v2 1/9] ima: Rename backing_inode to real_inode

[Stefan Berger]

Rename the backing_inode variable to real_inode since it gets its value
from real_inode().

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 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

[PATCH v2 2/9] security: allow finer granularity in permitting copy-up of security xattrs

[Stefan Berger]

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 <stefanb@linux.ibm.com>
---
 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

[PATCH v2 3/9] evm: Implement per signature type decision in security_inode_copy_up_xattr

[Stefan Berger]

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 <stefanb@linux.ibm.com>
---
 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

[PATCH v2 4/9] ima: Reset EVM status upon detecting changes to the real file

[Stefan Berger]

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 <stefanb@linux.ibm.com>
---
 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

[PATCH v2 5/9] evm: Use the inode holding the metadata to calculate metadata hash

[Stefan Berger]

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 <stefanb@linux.ibm.com>
---
 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

[PATCH v2 6/9] evm: Enforce signatures on unsupported filesystem for EVM_INIT_X509

[Stefan Berger]

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 <stefanb@linux.ibm.com>
---
 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

[PATCH v2 7/9] fs: Rename SB_I_EVM_UNSUPPORTED to SB_I_EVM_HMAC_UNSUPPORTED

[Stefan Berger]

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 <amir73il@gmail.com>
Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 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

[PATCH v2 8/9] evm: Rename is_unsupported_fs to is_unsupported_hmac_fs

[Stefan Berger]

Rename is_unsupported_fs to is_unsupported_hmac_fs since only HMAC is
unsupported now on certain filesystems.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 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

[PATCH v2 9/9] ima: Record i_version of real_inode for change detection

[Stefan Berger]

process_measurement() will try to detect file content changes for not-yet-
copied-up files on a stacked filesystem based on the i_version number of
the real inode: !inode_eq_iversion(real_inode, iint->version)
Therefore, take a snapshot of the i_version of the real file to be used
for i_version number-based file content change detection by IMA in
process_meassurements().

In this case vfs_getattr_nosec() cannot be used since it will return the
i_version number of the file on the overlay layer which will trigger more
iint resets in process_measurements() than necessary since this i_version
number represents different state than that of the real_inode (of a
not-yet-copied up file).

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 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

Re: [PATCH v2 4/9] ima: Reset EVM status upon detecting changes to the real file

[kernel test robot]

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/202402062032.8kRzlrPA-lkp@intel.com/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/202402062032.8kRzlrPA-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402062032.8kRzlrPA-lkp@intel.com/

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

Re: [PATCH v2 7/9] fs: Rename SB_I_EVM_UNSUPPORTED to SB_I_EVM_HMAC_UNSUPPORTED

[Amir Goldstein]

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> 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 <amir73il@gmail.com>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Acked-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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
>

Re: [PATCH v2 2/9] security: allow finer granularity in permitting copy-up of security xattrs

[Amir Goldstein]

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> 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 <stefanb@linux.ibm.com>

For ovl part

Acked-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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
>

Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change detection

[Amir Goldstein]

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> process_measurement() will try to detect file content changes for not-yet-
> copied-up files on a stacked filesystem based on the i_version number of
> the real inode: !inode_eq_iversion(real_inode, iint->version)
> Therefore, take a snapshot of the i_version of the real file to be used
> for i_version number-based file content change detection by IMA in
> process_meassurements().
>
> In this case vfs_getattr_nosec() cannot be used since it will return the
> i_version number of the file on the overlay layer which will trigger more
> iint resets in process_measurements() than necessary since this i_version
> number represents different state than that of the real_inode (of a
> not-yet-copied up file).
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  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.

Re: [PATCH v2 1/9] ima: Rename backing_inode to real_inode

[Amir Goldstein]

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> Rename the backing_inode variable to real_inode since it gets its value
> from real_inode().
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Acked-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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
>

Re: [PATCH v2 5/9] evm: Use the inode holding the metadata to calculate metadata hash

[Amir Goldstein]

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> 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 <stefanb@linux.ibm.com>

Acked-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  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
>

Re: [PATCH v2 4/9] ima: Reset EVM status upon detecting changes to the real file

[Amir Goldstein]

On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> 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 <stefanb@linux.ibm.com>
> ---
>  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.

Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change detection

[Jeff Layton]

On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:
> On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > 
> > process_measurement() will try to detect file content changes for not-yet-
> > copied-up files on a stacked filesystem based on the i_version number of
> > the real inode: !inode_eq_iversion(real_inode, iint->version)
> > Therefore, take a snapshot of the i_version of the real file to be used
> > for i_version number-based file content change detection by IMA in
> > process_meassurements().
> > 
> > In this case vfs_getattr_nosec() cannot be used since it will return the
> > i_version number of the file on the overlay layer which will trigger more
> > iint resets in process_measurements() than necessary since this i_version
> > number represents different state than that of the real_inode (of a
> > not-yet-copied up file).
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  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 <jlayton@kernel.org>

Re: [PATCH v2 5/9] evm: Use the inode holding the metadata to calculate metadata hash

[kernel test robot]

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/202402070220.eYpQ6zcm-lkp@intel.com/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/202402070220.eYpQ6zcm-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402070220.eYpQ6zcm-lkp@intel.com/

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

Re: [PATCH v2 4/9] ima: Reset EVM status upon detecting changes to the real file

[kernel test robot]

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/202402071226.lXIiGtbl-lkp@intel.com/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/202402071226.lXIiGtbl-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402071226.lXIiGtbl-lkp@intel.com/

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

Re: [PATCH v2 9/9] ima: Record i_version of real_inode for change detection

[Stefan Berger]

On 2/6/24 10:54, Jeff Layton wrote:
> On Tue, 2024-02-06 at 17:23 +0200, Amir Goldstein wrote:
>> On Mon, Feb 5, 2024 at 8:25 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>
>>> process_measurement() will try to detect file content changes for not-yet-
>>> copied-up files on a stacked filesystem based on the i_version number of
>>> the real inode: !inode_eq_iversion(real_inode, iint->version)
>>> Therefore, take a snapshot of the i_version of the real file to be used
>>> for i_version number-based file content change detection by IMA in
>>> process_meassurements().
>>>
>>> In this case vfs_getattr_nosec() cannot be used since it will return the
>>> i_version number of the file on the overlay layer which will trigger more
>>> iint resets in process_measurements() than necessary since this i_version
>>> number represents different state than that of the real_inode (of a
>>> not-yet-copied up file).
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   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.

Re: [PATCH v2 2/9] security: allow finer granularity in permitting copy-up of security xattrs

[Paul Moore]

On Mon, Feb 5, 2024 at 1:25 PM Stefan Berger <stefanb@linux.ibm.com> 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 <stefanb@linux.ibm.com>
> ---
>  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 <paul@paul-moore.com> (LSM,SELinux)

-- 
paul-moore.com