[PATCH v3 0/6] ima: Remove unnecessary inode locks

[PATCH v3 0/6] ima: Remove unnecessary inode locks

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

A recent syzbot report [1] showed a possible lock inversion between the
mmap lock and the inode lock. Paul Moore suggested to remove the inode lock
in IMA as a possible solution. A first patch set was made [2] to fulfill
that request, although incomplete due to not removing the inode lock around
the ima_appraise_measurement() call.

While the original report was fixed in a different way, by calling the
security_mmap_file() hook outside the mmap lock critical region in the
remap_file_pages() system call [3], the IMA patch set has benefits on its
own, since it merges two critical sections in one in process_measurement(),
and makes the latter and __ima_inode_hash() compete for the same lock.

Remove the inode lock in three phases (except from ima_update_xattr(), when
setting security.ima).

First, remove the S_IMA inode flag and the IS_IMA() macro, since S_IMA
needs to be set under the inode lock, and it is going to be removed. There
is no performance penalty in doing that, since the pointer of the inode
integrity metadata has been moved to the inode security blob when IMA was
made as a regular LSM [4], and retrieving such metadata can be done in
constant time (patch 1).

Second, move the mutex from the inode integrity metadata to the inode
security blob, so that the lock can be taken regardless of whether or not
the inode integrity metadata was allocated for that inode (patch 2).

Consequently, remove the inode lock just after the policy evaluation and
extend the critical region previously guarded by the integrity inode
metadata mutex to where the inode lock was.

Also, make sure that ima_inode_get() is called with the new mutex lock
held, to avoid non-atomic check/assignment of the integrity metadata in the
inode security blob (patch 3), and mark the pointer of inode integrity
metadata as a shared resource with READ_ONCE() and WRITE_ONCE() (patch 4).

Third, remove the inode lock around ima_appraise_measurement() by
postponing setting security.ima when IMA-Appraisal is in fix mode, to when
the file is closed (patch 5).

Finally, as a bug fix, do not reset the IMA_NEW_FILE flag when setting
inode attributes. By resetting the flag, IMA was not able to appraise new
files with modified metadata before security.ima was written to the disk
(patch 6).

This patch set applies on top of commit 4785ed362a24 ("ima: ignore suffixed
policy rule comments") of:

https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git


[1] https://lore.kernel.org/linux-security-module/66f7b10e.050a0220.46d20.0036.GAE@google.com/
[2] https://lore.kernel.org/linux-security-module/20241008165732.2603647-1-roberto.sassu@huaweicloud.com/
[3] https://lore.kernel.org/linux-security-module/20241018161415.3845146-1-roberto.sassu@huaweicloud.com/
[4] https://lore.kernel.org/linux-security-module/20240215103113.2369171-1-roberto.sassu@huaweicloud.com/

v2:
- Add recommendation to set evm=fix in the kernel command line (suggested
  by Mimi)
- Completely remove S_IMA definition (suggested by Jan Kara)
- Reverse mode and i_writecount checks in ima_check_last_writer()
  (suggested by Mimi)
- Drop O_PATH patch (ima_check_last_writer() was erroneously considering
  file descriptors with that open flag set due to a bug in the appraisal
  patch)

v1:
- New patches (1 suggested by Shu Han, 4-6)
- Remove ima_inode_get_iint() and ima_inode_set_iint() and access inode
  integrity metadata from the new ima_iint_cache_lock structure directly
- Return immediately in ima_inode_get() if the inode does not have a
  security blob (suggested by Paul Moore)

Roberto Sassu (6):
  fs: ima: Remove S_IMA and IS_IMA()
  ima: Remove inode lock
  ima: Detect if lock is held when iint pointer is set in inode security
    blob
  ima: Mark concurrent accesses to the iint pointer in the inode
    security blob
  ima: Defer fixing security.ima to __fput()
  ima: Reset IMA_NONACTION_RULE_FLAGS after post_setattr

 .../admin-guide/kernel-parameters.txt         |  3 +
 include/linux/fs.h                            |  2 -
 security/integrity/ima/ima.h                  | 33 +++----
 security/integrity/ima/ima_api.c              |  4 +-
 security/integrity/ima/ima_appraise.c         |  7 +-
 security/integrity/ima/ima_iint.c             | 95 +++++++++++++++----
 security/integrity/ima/ima_main.c             | 75 ++++++++-------
 7 files changed, 144 insertions(+), 75 deletions(-)

-- 
2.34.1

[PATCH v3 1/6] fs: ima: Remove S_IMA and IS_IMA()

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Commit 196f518128d2e ("IMA: explicit IMA i_flag to remove global lock on
inode_delete") introduced the new S_IMA inode flag to determine whether or
not an inode was processed by IMA. In that way, it was not necessary to
take the global lock on inode delete.

Since commit 4de2f084fbff ("ima: Make it independent from 'integrity'
LSM"), the pointer of the inode integrity metadata managed by IMA has been
moved to the inode security blob, from the rb-tree. The pointer is not NULL
only if the inode has been processed by IMA, i.e. ima_inode_get() has been
called for that inode.

Thus, since the IS_IMA() check can be now implemented by trivially testing
whether or not the pointer of inode integrity metadata is NULL, remove the
S_IMA definition in include/linux/fs.h and also the IS_IMA() macro.

Remove also the IS_IMA() invocation in ima_rdwr_violation_check(), since
whether the inode was processed by IMA will be anyway detected by a
subsequent call to ima_iint_find(). It does not have an additional overhead
since the decision can be made in constant time, as opposed to logarithm
when the inode integrity metadata was stored in the rb-tree.

Suggested-by: Shu Han <ebpqwerty472123@gmail.com>
Reviewed-by: Christian Brauner <brauner@kernel.org>
Acked-by: Jan Kara <jack@suse.cz>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/fs.h                | 2 --
 security/integrity/ima/ima_iint.c | 5 -----
 security/integrity/ima/ima_main.c | 2 +-
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 7e29433c5ecc..8ee6961ab54a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2272,7 +2272,6 @@ struct super_operations {
 #define S_NOCMTIME	(1 << 7)  /* Do not update file c/mtime */
 #define S_SWAPFILE	(1 << 8)  /* Do not truncate: swapon got its bmaps */
 #define S_PRIVATE	(1 << 9)  /* Inode is fs-internal */
-#define S_IMA		(1 << 10) /* Inode has an associated IMA struct */
 #define S_AUTOMOUNT	(1 << 11) /* Automount/referral quasi-directory */
 #define S_NOSEC		(1 << 12) /* no suid or xattr security attributes */
 #ifdef CONFIG_FS_DAX
@@ -2330,7 +2329,6 @@ static inline bool sb_rdonly(const struct super_block *sb) { return sb->s_flags
 #endif
 
 #define IS_PRIVATE(inode)	((inode)->i_flags & S_PRIVATE)
-#define IS_IMA(inode)		((inode)->i_flags & S_IMA)
 #define IS_AUTOMOUNT(inode)	((inode)->i_flags & S_AUTOMOUNT)
 #define IS_NOSEC(inode)		((inode)->i_flags & S_NOSEC)
 #define IS_DAX(inode)		((inode)->i_flags & S_DAX)
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 00b249101f98..9d9fc7a911ad 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -26,9 +26,6 @@ static struct kmem_cache *ima_iint_cache __ro_after_init;
  */
 struct ima_iint_cache *ima_iint_find(struct inode *inode)
 {
-	if (!IS_IMA(inode))
-		return NULL;
-
 	return ima_inode_get_iint(inode);
 }
 
@@ -102,7 +99,6 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
 
 	ima_iint_init_always(iint, inode);
 
-	inode->i_flags |= S_IMA;
 	ima_inode_set_iint(inode, iint);
 
 	return iint;
@@ -118,7 +114,6 @@ void ima_inode_free_rcu(void *inode_security)
 {
 	struct ima_iint_cache **iint_p = inode_security + ima_blob_sizes.lbs_inode;
 
-	/* *iint_p should be NULL if !IS_IMA(inode) */
 	if (*iint_p)
 		ima_iint_free(*iint_p);
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9b87556b03a7..6551be5754de 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -126,7 +126,7 @@ static void ima_rdwr_violation_check(struct file *file,
 	bool send_tomtou = false, send_writers = false;
 
 	if (mode & FMODE_WRITE) {
-		if (atomic_read(&inode->i_readcount) && IS_IMA(inode)) {
+		if (atomic_read(&inode->i_readcount)) {
 			if (!iint)
 				iint = ima_iint_find(inode);
 			/* IMA_MEASURE is set from reader side */
-- 
2.34.1

Re: [PATCH v3 1/6] fs: ima: Remove S_IMA and IS_IMA()

[Mimi Zohar]

On Wed, 2025-01-22 at 18:24 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit 196f518128d2e ("IMA: explicit IMA i_flag to remove global lock on
> inode_delete") introduced the new S_IMA inode flag to determine whether or
> not an inode was processed by IMA. In that way, it was not necessary to
> take the global lock on inode delete.
> 
> Since commit 4de2f084fbff ("ima: Make it independent from 'integrity'
> LSM"), the pointer of the inode integrity metadata managed by IMA has been
> moved to the inode security blob, from the rb-tree. The pointer is not NULL
> only if the inode has been processed by IMA, i.e. ima_inode_get() has been
> called for that inode.
> 
> Thus, since the IS_IMA() check can be now implemented by trivially testing
> whether or not the pointer of inode integrity metadata is NULL, remove the
> S_IMA definition in include/linux/fs.h and also the IS_IMA() macro.
> 
> Remove also the IS_IMA() invocation in ima_rdwr_violation_check(), since
> whether the inode was processed by IMA will be anyway detected by a
> subsequent call to ima_iint_find(). It does not have an additional overhead
> since the decision can be made in constant time, as opposed to logarithm
> when the inode integrity metadata was stored in the rb-tree.
> 
> Suggested-by: Shu Han <ebpqwerty472123@gmail.com>
> Reviewed-by: Christian Brauner <brauner@kernel.org>
> Acked-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewd-by: Mimi Zohar <zohar@linux.ibm.com>

[PATCH v3 2/6] ima: Remove inode lock

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Move out the mutex in the ima_iint_cache structure to a new structure
called ima_iint_cache_lock, so that a lock can be taken regardless of
whether or not inode integrity metadata are stored in the inode.

Introduce ima_inode_security() to retrieve the ima_iint_cache_lock
structure, if inode i_security is not NULL, and consequently remove
ima_inode_get_iint() and ima_inode_set_iint(), since the ima_iint_cache
structure can be read and modified from the new structure.

Move the mutex initialization and annotation in the new function
ima_inode_alloc_security() and introduce ima_iint_lock() and
ima_iint_unlock() to respectively lock and unlock the mutex.

Finally, expand the critical region in process_measurement() guarded by
iint->mutex up to where the inode was locked, use only one iint lock in
__ima_inode_hash(), since the mutex is now in the inode security blob, and
replace the inode_lock()/inode_unlock() calls in ima_check_last_writer().

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Paul Moore <paul@paul-moore.com>
---
 security/integrity/ima/ima.h      | 31 ++++-------
 security/integrity/ima/ima_api.c  |  4 +-
 security/integrity/ima/ima_iint.c | 92 ++++++++++++++++++++++++++-----
 security/integrity/ima/ima_main.c | 39 ++++++-------
 4 files changed, 109 insertions(+), 57 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 24d09ea91b87..f96021637bcf 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -182,7 +182,6 @@ struct ima_kexec_hdr {
 
 /* IMA integrity metadata associated with an inode */
 struct ima_iint_cache {
-	struct mutex mutex;	/* protects: version, flags, digest */
 	struct integrity_inode_attributes real_inode;
 	unsigned long flags;
 	unsigned long measured_pcrs;
@@ -195,35 +194,27 @@ struct ima_iint_cache {
 	struct ima_digest_data *ima_hash;
 };
 
+struct ima_iint_cache_lock {
+	struct mutex mutex;	/* protects: iint version, flags, digest */
+	struct ima_iint_cache *iint;
+};
+
 extern struct lsm_blob_sizes ima_blob_sizes;
 
-static inline struct ima_iint_cache *
-ima_inode_get_iint(const struct inode *inode)
+static inline struct ima_iint_cache_lock *ima_inode_security(void *i_security)
 {
-	struct ima_iint_cache **iint_sec;
-
-	if (unlikely(!inode->i_security))
+	if (unlikely(!i_security))
 		return NULL;
 
-	iint_sec = inode->i_security + ima_blob_sizes.lbs_inode;
-	return *iint_sec;
-}
-
-static inline void ima_inode_set_iint(const struct inode *inode,
-				      struct ima_iint_cache *iint)
-{
-	struct ima_iint_cache **iint_sec;
-
-	if (unlikely(!inode->i_security))
-		return;
-
-	iint_sec = inode->i_security + ima_blob_sizes.lbs_inode;
-	*iint_sec = iint;
+	return i_security + ima_blob_sizes.lbs_inode;
 }
 
 struct ima_iint_cache *ima_iint_find(struct inode *inode);
 struct ima_iint_cache *ima_inode_get(struct inode *inode);
+int ima_inode_alloc_security(struct inode *inode);
 void ima_inode_free_rcu(void *inode_security);
+void ima_iint_lock(struct inode *inode);
+void ima_iint_unlock(struct inode *inode);
 void __init ima_iintcache_init(void);
 
 extern const int read_idmap[];
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c35ea613c9f8..76b7280632fc 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -234,7 +234,7 @@ static bool ima_get_verity_digest(struct ima_iint_cache *iint,
  * Calculate the file hash, if it doesn't already exist,
  * storing the measurement and i_version in the iint.
  *
- * Must be called with iint->mutex held.
+ * Must be called with iint mutex held.
  *
  * Return 0 on success, error code otherwise
  */
@@ -343,7 +343,7 @@ int ima_collect_measurement(struct ima_iint_cache *iint, struct file *file,
  *	- the inode was previously flushed as well as the iint info,
  *	  containing the hashing info.
  *
- * Must be called with iint->mutex held.
+ * Must be called with iint mutex held.
  */
 void ima_store_measurement(struct ima_iint_cache *iint, struct file *file,
 			   const unsigned char *filename,
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 9d9fc7a911ad..dcc32483d29f 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -26,7 +26,13 @@ static struct kmem_cache *ima_iint_cache __ro_after_init;
  */
 struct ima_iint_cache *ima_iint_find(struct inode *inode)
 {
-	return ima_inode_get_iint(inode);
+	struct ima_iint_cache_lock *iint_lock;
+
+	iint_lock = ima_inode_security(inode->i_security);
+	if (!iint_lock)
+		return NULL;
+
+	return iint_lock->iint;
 }
 
 #define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH + 1)
@@ -37,18 +43,18 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode)
  * mutex to avoid lockdep false positives related to IMA + overlayfs.
  * See ovl_lockdep_annotate_inode_mutex_key() for more details.
  */
-static inline void ima_iint_lockdep_annotate(struct ima_iint_cache *iint,
-					     struct inode *inode)
+static inline void ima_iint_lock_lockdep_annotate(struct mutex *mutex,
+						  struct inode *inode)
 {
 #ifdef CONFIG_LOCKDEP
-	static struct lock_class_key ima_iint_mutex_key[IMA_MAX_NESTING];
+	static struct lock_class_key ima_iint_lock_mutex_key[IMA_MAX_NESTING];
 
 	int depth = inode->i_sb->s_stack_depth;
 
 	if (WARN_ON_ONCE(depth < 0 || depth >= IMA_MAX_NESTING))
 		depth = 0;
 
-	lockdep_set_class(&iint->mutex, &ima_iint_mutex_key[depth]);
+	lockdep_set_class(mutex, &ima_iint_lock_mutex_key[depth]);
 #endif
 }
 
@@ -65,14 +71,11 @@ static void ima_iint_init_always(struct ima_iint_cache *iint,
 	iint->ima_read_status = INTEGRITY_UNKNOWN;
 	iint->ima_creds_status = INTEGRITY_UNKNOWN;
 	iint->measured_pcrs = 0;
-	mutex_init(&iint->mutex);
-	ima_iint_lockdep_annotate(iint, inode);
 }
 
 static void ima_iint_free(struct ima_iint_cache *iint)
 {
 	kfree(iint->ima_hash);
-	mutex_destroy(&iint->mutex);
 	kmem_cache_free(ima_iint_cache, iint);
 }
 
@@ -87,9 +90,14 @@ static void ima_iint_free(struct ima_iint_cache *iint)
  */
 struct ima_iint_cache *ima_inode_get(struct inode *inode)
 {
+	struct ima_iint_cache_lock *iint_lock;
 	struct ima_iint_cache *iint;
 
-	iint = ima_iint_find(inode);
+	iint_lock = ima_inode_security(inode->i_security);
+	if (!iint_lock)
+		return NULL;
+
+	iint = iint_lock->iint;
 	if (iint)
 		return iint;
 
@@ -99,11 +107,31 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
 
 	ima_iint_init_always(iint, inode);
 
-	ima_inode_set_iint(inode, iint);
+	iint_lock->iint = iint;
 
 	return iint;
 }
 
+/**
+ * ima_inode_alloc_security - Called to init an inode
+ * @inode: Pointer to the inode
+ *
+ * Initialize and annotate the mutex in the ima_iint_cache_lock structure.
+ *
+ * Return: Zero.
+ */
+int ima_inode_alloc_security(struct inode *inode)
+{
+	struct ima_iint_cache_lock *iint_lock;
+
+	iint_lock = ima_inode_security(inode->i_security);
+
+	mutex_init(&iint_lock->mutex);
+	ima_iint_lock_lockdep_annotate(&iint_lock->mutex, inode);
+
+	return 0;
+}
+
 /**
  * ima_inode_free_rcu - Called to free an inode via a RCU callback
  * @inode_security: The inode->i_security pointer
@@ -112,10 +140,48 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
  */
 void ima_inode_free_rcu(void *inode_security)
 {
-	struct ima_iint_cache **iint_p = inode_security + ima_blob_sizes.lbs_inode;
+	struct ima_iint_cache_lock *iint_lock;
+
+	iint_lock = ima_inode_security(inode_security);
+
+	mutex_destroy(&iint_lock->mutex);
+
+	if (iint_lock->iint)
+		ima_iint_free(iint_lock->iint);
+}
+
+/**
+ * ima_iint_lock - Lock integrity metadata
+ * @inode: Pointer to the inode
+ *
+ * Lock integrity metadata.
+ */
+void ima_iint_lock(struct inode *inode)
+{
+	struct ima_iint_cache_lock *iint_lock;
+
+	iint_lock = ima_inode_security(inode->i_security);
+
+	/* Only inodes with i_security are processed by IMA. */
+	if (iint_lock)
+		mutex_lock(&iint_lock->mutex);
+}
+
+/**
+ * ima_iint_unlock - Unlock integrity metadata
+ * @inode: Pointer to the inode
+ *
+ * Unlock integrity metadata.
+ */
+void ima_iint_unlock(struct inode *inode)
+{
+	struct ima_iint_cache_lock *iint_lock;
+
+	iint_lock = ima_inode_security(inode->i_security);
 
-	if (*iint_p)
-		ima_iint_free(*iint_p);
+	/* Only inodes with i_security are processed by IMA. */
+	if (iint_lock)
+		mutex_unlock(&iint_lock->mutex);
 }
 
 static void ima_iint_init_once(void *foo)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6551be5754de..006f1e3725d6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -163,7 +163,7 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
 	if (!(mode & FMODE_WRITE))
 		return;
 
-	mutex_lock(&iint->mutex);
+	ima_iint_lock(inode);
 	if (atomic_read(&inode->i_writecount) == 1) {
 		struct kstat stat;
 
@@ -181,7 +181,7 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
 				ima_update_xattr(iint, file);
 		}
 	}
-	mutex_unlock(&iint->mutex);
+	ima_iint_unlock(inode);
 }
 
 /**
@@ -247,7 +247,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	if (action & IMA_FILE_APPRAISE)
 		func = FILE_CHECK;
 
-	inode_lock(inode);
+	ima_iint_lock(inode);
 
 	if (action) {
 		iint = ima_inode_get(inode);
@@ -259,15 +259,11 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		ima_rdwr_violation_check(file, iint, action & IMA_MEASURE,
 					 &pathbuf, &pathname, filename);
 
-	inode_unlock(inode);
-
 	if (rc)
 		goto out;
 	if (!action)
 		goto out;
 
-	mutex_lock(&iint->mutex);
-
 	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
 		/* reset appraisal flags if ima_inode_post_setattr was called */
 		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
@@ -412,10 +408,10 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	if ((mask & MAY_WRITE) && test_bit(IMA_DIGSIG, &iint->atomic_flags) &&
 	     !(iint->flags & IMA_NEW_FILE))
 		rc = -EACCES;
-	mutex_unlock(&iint->mutex);
 	kfree(xattr_value);
 	ima_free_modsig(modsig);
 out:
+	ima_iint_unlock(inode);
 	if (pathbuf)
 		__putname(pathbuf);
 	if (must_appraise) {
@@ -580,18 +576,13 @@ static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
 	struct ima_iint_cache *iint = NULL, tmp_iint;
 	int rc, hash_algo;
 
-	if (ima_policy_flag) {
+	ima_iint_lock(inode);
+
+	if (ima_policy_flag)
 		iint = ima_iint_find(inode);
-		if (iint)
-			mutex_lock(&iint->mutex);
-	}
 
 	if ((!iint || !(iint->flags & IMA_COLLECTED)) && file) {
-		if (iint)
-			mutex_unlock(&iint->mutex);
-
 		memset(&tmp_iint, 0, sizeof(tmp_iint));
-		mutex_init(&tmp_iint.mutex);
 
 		rc = ima_collect_measurement(&tmp_iint, file, NULL, 0,
 					     ima_hash_algo, NULL);
@@ -600,22 +591,24 @@ static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
 			if (rc != -ENOMEM)
 				kfree(tmp_iint.ima_hash);
 
+			ima_iint_unlock(inode);
 			return -EOPNOTSUPP;
 		}
 
 		iint = &tmp_iint;
-		mutex_lock(&iint->mutex);
 	}
 
-	if (!iint)
+	if (!iint) {
+		ima_iint_unlock(inode);
 		return -EOPNOTSUPP;
+	}
 
 	/*
 	 * ima_file_hash can be called when ima_collect_measurement has still
 	 * not been called, we might not always have a hash.
 	 */
 	if (!iint->ima_hash || !(iint->flags & IMA_COLLECTED)) {
-		mutex_unlock(&iint->mutex);
+		ima_iint_unlock(inode);
 		return -EOPNOTSUPP;
 	}
 
@@ -626,11 +619,12 @@ static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
 		memcpy(buf, iint->ima_hash->digest, copied_size);
 	}
 	hash_algo = iint->ima_hash->algo;
-	mutex_unlock(&iint->mutex);
 
 	if (iint == &tmp_iint)
 		kfree(iint->ima_hash);
 
+	ima_iint_unlock(inode);
+
 	return hash_algo;
 }
 
@@ -1115,7 +1109,7 @@ EXPORT_SYMBOL_GPL(ima_measure_critical_data);
  * @kmod_name: kernel module name
  *
  * Avoid a verification loop where verifying the signature of the modprobe
- * binary requires executing modprobe itself. Since the modprobe iint->mutex
+ * binary requires executing modprobe itself. Since the modprobe iint mutex
  * is already held when the signature verification is performed, a deadlock
  * occurs as soon as modprobe is executed within the critical region, since
  * the same lock cannot be taken again.
@@ -1190,6 +1184,7 @@ static struct security_hook_list ima_hooks[] __ro_after_init = {
 #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
 	LSM_HOOK_INIT(kernel_module_request, ima_kernel_module_request),
 #endif
+	LSM_HOOK_INIT(inode_alloc_security, ima_inode_alloc_security),
 	LSM_HOOK_INIT(inode_free_security_rcu, ima_inode_free_rcu),
 };
 
@@ -1207,7 +1202,7 @@ static int __init init_ima_lsm(void)
 }
 
 struct lsm_blob_sizes ima_blob_sizes __ro_after_init = {
-	.lbs_inode = sizeof(struct ima_iint_cache *),
+	.lbs_inode = sizeof(struct ima_iint_cache_lock),
 };
 
 DEFINE_LSM(ima) = {
-- 
2.34.1

Re: [PATCH v3 2/6] ima: Remove inode lock

[Mimi Zohar]

On Wed, 2025-01-22 at 18:24 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Move out the mutex in the ima_iint_cache structure to a new structure
> called ima_iint_cache_lock, so that a lock can be taken regardless of
> whether or not inode integrity metadata are stored in the inode.
> 
> Introduce ima_inode_security() to retrieve the ima_iint_cache_lock
> structure, if inode i_security is not NULL, and consequently remove
> ima_inode_get_iint() and ima_inode_set_iint(), since the ima_iint_cache
> structure can be read and modified from the new structure.
> 
> Move the mutex initialization and annotation in the new function
> ima_inode_alloc_security() and introduce ima_iint_lock() and
> ima_iint_unlock() to respectively lock and unlock the mutex.
> 
> Finally, expand the critical region in process_measurement() guarded by
> iint->mutex up to where the inode was locked, use only one iint lock in
> __ima_inode_hash(), since the mutex is now in the inode security blob, and
> replace the inode_lock()/inode_unlock() calls in ima_check_last_writer().
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Paul Moore <paul@paul-moore.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

[PATCH v3 3/6] ima: Detect if lock is held when iint pointer is set in inode security blob

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

IMA stores a pointer of the ima_iint_cache structure, containing integrity
metadata, in the inode security blob. However, check and assignment of this
pointer is not atomic, and it might happen that two tasks both see that the
iint pointer is NULL and try to set it, causing a memory leak.

Detect if the iint check and assignment is guarded by the iint_lock mutex,
by adding a lockdep assertion in ima_inode_get().

Consequently, guard the remaining ima_inode_get() calls, in
ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
warnings.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_iint.c |  2 ++
 security/integrity/ima/ima_main.c | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index dcc32483d29f..fca9db293c79 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -97,6 +97,8 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
 	if (!iint_lock)
 		return NULL;
 
+	lockdep_assert_held(&iint_lock->mutex);
+
 	iint = iint_lock->iint;
 	if (iint)
 		return iint;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 006f1e3725d6..0aed8f730c42 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -705,14 +705,19 @@ static void ima_post_create_tmpfile(struct mnt_idmap *idmap,
 	if (!must_appraise)
 		return;
 
+	ima_iint_lock(inode);
+
 	/* Nothing to do if we can't allocate memory */
 	iint = ima_inode_get(inode);
-	if (!iint)
+	if (!iint) {
+		ima_iint_unlock(inode);
 		return;
+	}
 
 	/* needed for writing the security xattrs */
 	set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
 	iint->ima_file_status = INTEGRITY_PASS;
+	ima_iint_unlock(inode);
 }
 
 /**
@@ -737,13 +742,18 @@ static void ima_post_path_mknod(struct mnt_idmap *idmap, struct dentry *dentry)
 	if (!must_appraise)
 		return;
 
+	ima_iint_lock(inode);
+
 	/* Nothing to do if we can't allocate memory */
 	iint = ima_inode_get(inode);
-	if (!iint)
+	if (!iint) {
+		ima_iint_unlock(inode);
 		return;
+	}
 
 	/* needed for re-opening empty files */
 	iint->flags |= IMA_NEW_FILE;
+	ima_iint_unlock(inode);
 }
 
 /**
-- 
2.34.1

Re: [PATCH v3 3/6] ima: Detect if lock is held when iint pointer is set in inode security blob

[Mimi Zohar]

On Wed, 2025-01-22 at 18:24 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> IMA stores a pointer of the ima_iint_cache structure, containing integrity
> metadata, in the inode security blob. However, check and assignment of this
> pointer is not atomic, and it might happen that two tasks both see that the
> iint pointer is NULL and try to set it, causing a memory leak.
> 
> Detect if the iint check and assignment is guarded by the iint_lock mutex,
> by adding a lockdep assertion in ima_inode_get().
> 
> Consequently, guard the remaining ima_inode_get() calls, in
> ima_post_create_tmpfile() and ima_post_path_mknod(), to avoid the lockdep
> warnings.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Thank you for updating the patch description.  You might also want to mention that
CONFIG_LOCKDEP_DEBUG is required to see the warnings.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

[PATCH v3 4/6] ima: Mark concurrent accesses to the iint pointer in the inode security blob

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Use the READ_ONCE() and WRITE_ONCE() macros to mark concurrent read and
write accesses to the portion of the inode security blob containing the
iint pointer.

Writers are serialized by the iint lock.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_iint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index fca9db293c79..c763f431fbc1 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -32,7 +32,7 @@ struct ima_iint_cache *ima_iint_find(struct inode *inode)
 	if (!iint_lock)
 		return NULL;
 
-	return iint_lock->iint;
+	return READ_ONCE(iint_lock->iint);
 }
 
 #define IMA_MAX_NESTING (FILESYSTEM_MAX_STACK_DEPTH + 1)
@@ -99,7 +99,7 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
 
 	lockdep_assert_held(&iint_lock->mutex);
 
-	iint = iint_lock->iint;
+	iint = READ_ONCE(iint_lock->iint);
 	if (iint)
 		return iint;
 
@@ -109,7 +109,7 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
 
 	ima_iint_init_always(iint, inode);
 
-	iint_lock->iint = iint;
+	WRITE_ONCE(iint_lock->iint, iint);
 
 	return iint;
 }
-- 
2.34.1

Re: [PATCH v3 4/6] ima: Mark concurrent accesses to the iint pointer in the inode security blob

[Mimi Zohar]

On Wed, 2025-01-22 at 18:24 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Use the READ_ONCE() and WRITE_ONCE() macros to mark concurrent read and
> write accesses to the portion of the inode security blob containing the
> iint pointer.
> 
> Writers are serialized by the iint lock.
> 
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

[PATCH v3 5/6] ima: Defer fixing security.ima to __fput()

[Roberto Sassu]

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.

Deferring fixing security.ima has a side effect: metadata of files with an
invalid EVM HMAC cannot be updated until the file is close. In alternative
to waiting, it is also recommended to add 'evm=fix' in the kernel command
line to handle this case (recommendation added to kernel-parameters.txt as
well).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 .../admin-guide/kernel-parameters.txt          |  3 +++
 security/integrity/ima/ima.h                   |  1 +
 security/integrity/ima/ima_appraise.c          |  7 +++++--
 security/integrity/ima/ima_main.c              | 18 +++++++++++-------
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index dc663c0ca670..07219a3a2ee5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2083,6 +2083,9 @@
 			Format: { "off" | "enforce" | "fix" | "log" }
 			default: "enforce"
 
+			ima_appraise=fix should be used in conjunction with
+			evm=fix, when also inode metadata should be fixed.
+
 	ima_appraise_tcb [IMA] Deprecated.  Use ima_policy= instead.
 			The builtin appraise policy appraises all files
 			owned by uid=0.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f96021637bcf..e1a3d1239bee 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 884a3533f7af..ec57b36925cf 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 0aed8f730c42..46adfd524dd8 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 ((mode & FMODE_WRITE) && atomic_read(&inode->i_writecount) == 1) {
 		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);
-- 
2.34.1

Re: [PATCH v3 5/6] ima: Defer fixing security.ima to __fput()

[Mimi Zohar]

On Wed, 2025-01-22 at 18:24 +0100, Roberto Sassu wrote:
> 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.
> 
> Deferring fixing security.ima has a side effect: metadata of files with an
> invalid EVM HMAC cannot be updated until the file is close. In alternative
> to waiting, it is also recommended to add 'evm=fix' in the kernel command
> line to handle this case (recommendation added to kernel-parameters.txt as
> well).
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---

[ ... ]

> --- 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 ((mode & FMODE_WRITE) && atomic_read(&inode->i_writecount) == 1) {
>  		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);

In ima_appraise_measurement() IMA calls EVM to verify the file metadata
(evm_verifyxattr). One would think that since IMA is not "fixing" security.ima, EVM
would not require the inode lock to be taken by IMA.  However, in addition to
verifying the file metdata, EVM converts the original file metadata signature to an
HMAC.  This does require the inode lock. Perhaps the EVM conversion from a signature
to an HMAC needs to be deferred as well.

Mimi

[PATCH v3 6/6] ima: Reset IMA_NONACTION_RULE_FLAGS after post_setattr

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

Commit 11c60f23ed13 ("integrity: Remove unused macro
IMA_ACTION_RULE_FLAGS") removed the IMA_ACTION_RULE_FLAGS mask, due to it
not being used after commit 0d73a55208e9 ("ima: re-introduce own integrity
cache lock").

However, it seems that the latter commit mistakenly used the wrong mask
when moving the code from ima_inode_post_setattr() to
process_measurement(). There is no mention in the commit message about this
change and it looks quite important, since changing from IMA_ACTIONS_FLAGS
(later renamed to IMA_NONACTION_FLAGS) to IMA_ACTION_RULE_FLAGS was done by
commit 42a4c603198f0 ("ima: fix ima_inode_post_setattr").

Restore the original change of resetting only the policy-specific flags and
not the new file status, but with new mask 0xfb000000 since the
policy-specific flags changed meanwhile. Also rename IMA_ACTION_RULE_FLAGS
to IMA_NONACTION_RULE_FLAGS, to be consistent with IMA_NONACTION_FLAGS.

Cc: stable@vger.kernel.org # v4.16.x
Fixes: 11c60f23ed13 ("integrity: Remove unused macro IMA_ACTION_RULE_FLAGS")
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima.h      | 1 +
 security/integrity/ima/ima_main.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e1a3d1239bee..615900d4150d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -141,6 +141,7 @@ struct ima_kexec_hdr {
 
 /* IMA iint policy rule cache flags */
 #define IMA_NONACTION_FLAGS	0xff000000
+#define IMA_NONACTION_RULE_FLAGS	0xfb000000
 #define IMA_DIGSIG_REQUIRED	0x01000000
 #define IMA_PERMIT_DIRECTIO	0x02000000
 #define IMA_NEW_FILE		0x04000000
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 46adfd524dd8..7173dca20c23 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -275,7 +275,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		/* reset appraisal flags if ima_inode_post_setattr was called */
 		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
-				 IMA_NONACTION_FLAGS);
+				 IMA_NONACTION_RULE_FLAGS);
 
 	/*
 	 * Re-evaulate the file if either the xattr has changed or the
-- 
2.34.1

Re: [PATCH v3 6/6] ima: Reset IMA_NONACTION_RULE_FLAGS after post_setattr

[Mimi Zohar]

On Wed, 2025-01-22 at 18:24 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Commit 11c60f23ed13 ("integrity: Remove unused macro
> IMA_ACTION_RULE_FLAGS") removed the IMA_ACTION_RULE_FLAGS mask, due to it
> not being used after commit 0d73a55208e9 ("ima: re-introduce own integrity
> cache lock").
> 
> However, it seems that the latter commit mistakenly used the wrong mask
> when moving the code from ima_inode_post_setattr() to process_measurement(). There
> is no mention in the commit message about this
> change and it looks quite important, since changing from IMA_ACTIONS_FLAGS
> (later renamed to IMA_NONACTION_FLAGS) to IMA_ACTION_RULE_FLAGS was done by
> commit 42a4c603198f0 ("ima: fix ima_inode_post_setattr").

Roberto, thank you for the detailed explanation.  Could we summarize the problem as: 

Commit 0d73a55208e9 ("ima: re-introduce own integrity cache lock") mistakenly
reverted the performance improvement introduced in commit 42a4c603198f0 ("ima: fix
ima_inode_post_setattr").  The unused bit mask was subsequently removed by commit
11c60f23ed13 ("integrity: Remove unused macro IMA_ACTION_RULE_FLAGS").

> 
> Restore the original change of resetting only the policy-specific flags and
> not the new file status, but with new mask 0xfb000000 since the
> policy-specific flags changed meanwhile. Also rename IMA_ACTION_RULE_FLAGS
> to IMA_NONACTION_RULE_FLAGS, to be consistent with IMA_NONACTION_FLAGS.

Instead of restoring the bit mask that is used only once, consider inlining the
correct bit mask (e.g. IMA_NONACTION_FLAGS & ~IMA_NEW_FILE) and expanding the
existing comment.

> 
> Cc: stable@vger.kernel.org # v4.16.x
> Fixes: 11c60f23ed13 ("integrity: Remove unused macro IMA_ACTION_RULE_FLAGS")

Please update the Fixes tag to refer to commit 0d73a55208e9.

> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

> ---
>  security/integrity/ima/ima.h      | 1 +
>  security/integrity/ima/ima_main.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index e1a3d1239bee..615900d4150d 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -141,6 +141,7 @@ struct ima_kexec_hdr {
>  
>  /* IMA iint policy rule cache flags */
>  #define IMA_NONACTION_FLAGS	0xff000000
> +#define IMA_NONACTION_RULE_FLAGS	0xfb000000
>  #define IMA_DIGSIG_REQUIRED	0x01000000
>  #define IMA_PERMIT_DIRECTIO	0x02000000
>  #define IMA_NEW_FILE		0x04000000
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 46adfd524dd8..7173dca20c23 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -275,7 +275,7 @@ static int process_measurement(struct file *file, const struct
> cred *cred,
>  		/* reset appraisal flags if ima_inode_post_setattr was called */

Update the comment based on the original commit 42a4c603198f ("ima: fix
ima_inode_post_setattr") patch description.

thanks,

Mimi

>  		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
>  				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
> -				 IMA_NONACTION_FLAGS);
> +				 IMA_NONACTION_RULE_FLAGS);
>  	/*
>  	 * Re-evaulate the file if either the xattr has changed or the