diff mbox series

[v2,5/7] ima: Set security.ima on file close when ima_appraise=fix

Message ID 20241128100621.461743-6-roberto.sassu@huaweicloud.com (mailing list archive)
State New
Headers show
Series ima: Remove unnecessary inode locks | expand

Commit Message

Roberto Sassu Nov. 28, 2024, 10:06 a.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

IMA-Appraisal implements a fix mode, selectable from the kernel command
line by specifying ima_appraise=fix.

The fix mode is meant to be used in a TOFU (trust on first use) model,
where systems are supposed to work under controlled conditions before the
real enforcement starts.

Since the systems are under controlled conditions, it is assumed that the
files are not corrupted, and thus their current data digest can be trusted,
and written to security.ima.

When IMA-Appraisal is switched to enforcing mode, the security.ima value
collected during the fix mode is used as a reference value, and a mismatch
with the current value cause the access request to be denied.

However, since fixing security.ima is placed in ima_appraise_measurement()
during the integrity check, it requires the inode lock to be taken in
process_measurement(), in addition to ima_update_xattr() invoked at file
close.

Postpone the security.ima update to ima_check_last_writer(), by setting the
new atomic flag IMA_UPDATE_XATTR_FIX in the inode integrity metadata, in
ima_appraise_measurement(), if security.ima needs to be fixed. In this way,
the inode lock can be removed from process_measurement(). Also, set the
cause appropriately for the fix operation and for allowing access to new
and empty signed files.

Finally, update security.ima when IMA_UPDATE_XATTR_FIX is set, and when
there wasn't a previous security.ima update, which occurs if the process
closing the file descriptor is the last writer.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h          |  1 +
 security/integrity/ima/ima_appraise.c |  7 +++++--
 security/integrity/ima/ima_main.c     | 18 +++++++++++-------
 3 files changed, 17 insertions(+), 9 deletions(-)
diff mbox series

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index b4eeab48f08a..22c3b87cfcac 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -179,6 +179,7 @@  struct ima_kexec_hdr {
 #define IMA_CHANGE_ATTR		2
 #define IMA_DIGSIG		3
 #define IMA_MUST_MEASURE	4
+#define IMA_UPDATE_XATTR_FIX	5
 
 /* IMA integrity metadata associated with an inode */
 struct ima_iint_cache {
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 656c709b974f..94401de8b805 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -576,8 +576,10 @@  int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
 		if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
 		    (!xattr_value ||
 		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
-			if (!ima_fix_xattr(dentry, iint))
-				status = INTEGRITY_PASS;
+			/* Fix by setting security.ima on file close. */
+			set_bit(IMA_UPDATE_XATTR_FIX, &iint->atomic_flags);
+			status = INTEGRITY_PASS;
+			cause = "fix";
 		}
 
 		/*
@@ -587,6 +589,7 @@  int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
 		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
 		    test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
 			status = INTEGRITY_PASS;
+			cause = "new-signed-file";
 		}
 
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1e474ff6a777..50b37420ea2c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -158,13 +158,16 @@  static void ima_check_last_writer(struct ima_iint_cache *iint,
 				  struct inode *inode, struct file *file)
 {
 	fmode_t mode = file->f_mode;
-	bool update;
+	bool update = false, update_fix;
 
-	if (!(mode & FMODE_WRITE))
+	update_fix = test_and_clear_bit(IMA_UPDATE_XATTR_FIX,
+					&iint->atomic_flags);
+
+	if (!(mode & FMODE_WRITE) && !update_fix)
 		return;
 
 	ima_iint_lock(inode);
-	if (atomic_read(&inode->i_writecount) == 1) {
+	if (atomic_read(&inode->i_writecount) == 1 && (mode & FMODE_WRITE)) {
 		struct kstat stat;
 
 		update = test_and_clear_bit(IMA_UPDATE_XATTR,
@@ -181,6 +184,10 @@  static void ima_check_last_writer(struct ima_iint_cache *iint,
 				ima_update_xattr(iint, file);
 		}
 	}
+
+	if (!update && update_fix)
+		ima_update_xattr(iint, file);
+
 	ima_iint_unlock(inode);
 }
 
@@ -378,13 +385,10 @@  static int process_measurement(struct file *file, const struct cred *cred,
 				      template_desc);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
 		rc = ima_check_blacklist(iint, modsig, pcr);
-		if (rc != -EPERM) {
-			inode_lock(inode);
+		if (rc != -EPERM)
 			rc = ima_appraise_measurement(func, iint, file,
 						      pathname, xattr_value,
 						      xattr_len, modsig);
-			inode_unlock(inode);
-		}
 		if (!rc)
 			rc = mmap_violation_check(func, file, &pathbuf,
 						  &pathname, filename);