[PATCH v2 0/7] ima: Remove unnecessary inode locks

[PATCH v2 0/7] 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 make 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).

Second, 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).

While testing the new functionality, two bugs were found and corrected.
Discard in IMA files opened with the O_PATH open flags, since no data are
accessed (the file descriptor is used for different purposes). Otherwise,
IMA ended up trying to read the files anyway, causing a kernel warning to
be printed in the kernel log (patch 6).

Do not reset the IMA_NEW_FILE flag as a result of setting inode attributes,
as it was before the patch to reintroduce the inode integrity metadata
mutex. By resetting the flag, IMA was not able to appraise new files with
modified metadata before security.ima was written to the disk (patch 7).

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

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 (7):
  fs: ima: Remove S_IMA and IS_IMA()
  ima: Remove inode lock
  ima: Ensure lock is held when setting iint pointer in inode security
    blob
  ima: Mark concurrent accesses to the iint pointer in the inode
    security blob
  ima: Set security.ima on file close when ima_appraise=fix
  ima: Discard files opened with O_PATH
  ima: Reset IMA_NONACTION_RULE_FLAGS after post_setattr

 include/linux/fs.h                    |  3 +-
 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     | 81 +++++++++++++----------
 6 files changed, 146 insertions(+), 77 deletions(-)

-- 
2.47.0.118.gfd3785337b

[PATCH v2 1/7] 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>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 include/linux/fs.h                | 3 +--
 security/integrity/ima/ima_iint.c | 5 -----
 security/integrity/ima/ima_main.c | 2 +-
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3559446279c1..b33363becbdd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2261,7 +2261,7 @@ 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_IMA	(1 << 10) Inode has an associated IMA struct (unused) */
 #define S_AUTOMOUNT	(1 << 11) /* Automount/referral quasi-directory */
 #define S_NOSEC		(1 << 12) /* no suid or xattr security attributes */
 #ifdef CONFIG_FS_DAX
@@ -2319,7 +2319,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 06132cf47016..cea0afbbc28d 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.47.0.118.gfd3785337b

[PATCH v2 2/7] 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 3f1a82b7cd71..b4eeab48f08a 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 984e861f6e33..37c2a228f0e1 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 cea0afbbc28d..05cfb04cd02b 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;
 }
 
@@ -1118,7 +1112,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.
@@ -1193,6 +1187,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),
 };
 
@@ -1210,7 +1205,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.47.0.118.gfd3785337b

[PATCH v2 3/7] ima: Ensure lock is held when setting iint pointer 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.

Ensure that the iint check and assignment is guarded, 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 05cfb04cd02b..1e474ff6a777 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.47.0.118.gfd3785337b

[PATCH v2 4/7] 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.

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.47.0.118.gfd3785337b

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

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

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 --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);
-- 
2.47.0.118.gfd3785337b

[PATCH v2 6/7] ima: Discard files opened with O_PATH

[Roberto Sassu]

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

According to man open.2, files opened with O_PATH are not really opened. The
obtained file descriptor is used to indicate a location in the filesystem
tree and to perform operations that act purely at the file descriptor
level.

Thus, ignore open() syscalls with O_PATH, since IMA cares about file data.

Cc: stable@vger.kernel.org # v2.6.39.x
Fixes: 1abf0c718f15a ("New kind of open files - "location only".")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 50b37420ea2c..712c3a522e6c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -202,7 +202,8 @@ static void ima_file_free(struct file *file)
 	struct inode *inode = file_inode(file);
 	struct ima_iint_cache *iint;
 
-	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
+	    (file->f_flags & O_PATH))
 		return;
 
 	iint = ima_iint_find(inode);
@@ -232,7 +233,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	enum hash_algo hash_algo;
 	unsigned int allowed_algos = 0;
 
-	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
+	    (file->f_flags & O_PATH))
 		return 0;
 
 	/* Return an IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT action
-- 
2.47.0.118.gfd3785337b

[PATCH v2 7/7] 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, but with new mask 0xfb000000 since the
policy-specific flags changed meanwhile, and 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")
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 22c3b87cfcac..32ffef2cc92a 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 712c3a522e6c..83e467ad18d4 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -277,7 +277,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.47.0.118.gfd3785337b

Re: [PATCH v2 1/7] fs: ima: Remove S_IMA and IS_IMA()

[Jan Kara]

On Thu 28-11-24 11:06:14, 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>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  include/linux/fs.h                | 3 +--
>  security/integrity/ima/ima_iint.c | 5 -----
>  security/integrity/ima/ima_main.c | 2 +-
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3559446279c1..b33363becbdd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2261,7 +2261,7 @@ 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_IMA	(1 << 10) Inode has an associated IMA struct (unused) */

Well, I guess you can just delete this line. These are internal kernel
flags so we can do whatever we want with them. Otherwise the patch looks
good. Feel free to add:

Acked-by: Jan Kara <jack@suse.cz>

								Honza

>  #define S_AUTOMOUNT	(1 << 11) /* Automount/referral quasi-directory */
>  #define S_NOSEC		(1 << 12) /* no suid or xattr security attributes */
>  #ifdef CONFIG_FS_DAX
> @@ -2319,7 +2319,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 06132cf47016..cea0afbbc28d 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.47.0.118.gfd3785337b
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 1/7] fs: ima: Remove S_IMA and IS_IMA()

[Christian Brauner]

On Thu, Nov 28, 2024 at 11:06:14AM +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>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  include/linux/fs.h                | 3 +--
>  security/integrity/ima/ima_iint.c | 5 -----
>  security/integrity/ima/ima_main.c | 2 +-
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3559446279c1..b33363becbdd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2261,7 +2261,7 @@ 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_IMA	(1 << 10) Inode has an associated IMA struct (unused) */

As Jan said, that line should be deleted. Otherwise,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH v2 6/7] ima: Discard files opened with O_PATH

[Christian Brauner]

On Thu, Nov 28, 2024 at 11:06:19AM +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> According to man open.2, files opened with O_PATH are not really opened. The
> obtained file descriptor is used to indicate a location in the filesystem
> tree and to perform operations that act purely at the file descriptor
> level.
> 
> Thus, ignore open() syscalls with O_PATH, since IMA cares about file data.
> 
> Cc: stable@vger.kernel.org # v2.6.39.x
> Fixes: 1abf0c718f15a ("New kind of open files - "location only".")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 50b37420ea2c..712c3a522e6c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -202,7 +202,8 @@ static void ima_file_free(struct file *file)
>  	struct inode *inode = file_inode(file);
>  	struct ima_iint_cache *iint;
>  
> -	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> +	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
> +	    (file->f_flags & O_PATH))
>  		return;
>  
>  	iint = ima_iint_find(inode);
> @@ -232,7 +233,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  	enum hash_algo hash_algo;
>  	unsigned int allowed_algos = 0;
>  
> -	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> +	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
> +	    (file->f_flags & O_PATH))
>  		return 0;

if (file->f_mode & FMODE_PATH)

please.

Re: [PATCH v2 6/7] ima: Discard files opened with O_PATH

[Roberto Sassu]

On Thu, 2024-11-28 at 17:22 +0100, Christian Brauner wrote:
> On Thu, Nov 28, 2024 at 11:06:19AM +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > According to man open.2, files opened with O_PATH are not really opened. The
> > obtained file descriptor is used to indicate a location in the filesystem
> > tree and to perform operations that act purely at the file descriptor
> > level.
> > 
> > Thus, ignore open() syscalls with O_PATH, since IMA cares about file data.
> > 
> > Cc: stable@vger.kernel.org # v2.6.39.x
> > Fixes: 1abf0c718f15a ("New kind of open files - "location only".")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/ima/ima_main.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 50b37420ea2c..712c3a522e6c 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -202,7 +202,8 @@ static void ima_file_free(struct file *file)
> >  	struct inode *inode = file_inode(file);
> >  	struct ima_iint_cache *iint;
> >  
> > -	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > +	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
> > +	    (file->f_flags & O_PATH))
> >  		return;
> >  
> >  	iint = ima_iint_find(inode);
> > @@ -232,7 +233,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
> >  	enum hash_algo hash_algo;
> >  	unsigned int allowed_algos = 0;
> >  
> > -	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> > +	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
> > +	    (file->f_flags & O_PATH))
> >  		return 0;
> 
> if (file->f_mode & FMODE_PATH)
> 
> please.

Oh, ok.

Thanks

Roberto

Re: [PATCH v2 2/7] ima: Remove inode lock

[Mimi Zohar]

On Thu, 2024-11-28 at 11:06 +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>
> ---
>  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 3f1a82b7cd71..b4eeab48f08a 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)
>  {

Is there a reason for naming the function ima_inode_security() and passing
i_security, when the other LSMs name it <lsm>_inode() and pass the inode?

static inline struct inode_smack *smack_inode(const struct inode *inode)
static inline struct inode_security_struct *selinux_inode(const struct inode *inode)
static inline struct landlock_inode_security *landlock_inode(const struct inode
*const inode)

Mimi


> -	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 984e861f6e33..37c2a228f0e1 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 cea0afbbc28d..05cfb04cd02b 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;
>  }
>  
> @@ -1118,7 +1112,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.
> @@ -1193,6 +1187,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),
>  };
>  
> @@ -1210,7 +1205,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) = {

Re: [PATCH v2 3/7] ima: Ensure lock is held when setting iint pointer in inode security blob

[Mimi Zohar]

On Thu, 2024-11-28 at 11:06 +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.
> 
> Ensure that the iint check and assignment is guarded, by adding a lockdep
> assertion in ima_inode_get().

-> is guarded by the ima_iint_cache_lock mutex, ...

> 
> 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);
> +

lockdep_assert_held() doesn't actually "ensure" the lock is held, but emits a warning
when the lock is not held (if debugging is enabled).  Semantically "ensure" gives the
impression of enforcing.

Mimi

>  	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 05cfb04cd02b..1e474ff6a777 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);
>  }
>  
>  /**

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

[Mimi Zohar]

On Thu, 2024-11-28 at 11:06 +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.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks, Roberto.

Reviewed-by:  Mimi Zohar <zohar@linux.ibm.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;
>  }

Re: [PATCH v2 2/7] ima: Remove inode lock

[Roberto Sassu]

On 1/14/2025 2:35 PM, Mimi Zohar wrote:
> On Thu, 2024-11-28 at 11:06 +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>
>> ---
>>   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 3f1a82b7cd71..b4eeab48f08a 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)
>>   {
> 
> Is there a reason for naming the function ima_inode_security() and passing
> i_security, when the other LSMs name it <lsm>_inode() and pass the inode?

Yes, ima_inode_free_rcu() only provides the inode security blob, and not 
the inode itself.

Thanks

Roberto

> static inline struct inode_smack *smack_inode(const struct inode *inode)
> static inline struct inode_security_struct *selinux_inode(const struct inode *inode)
> static inline struct landlock_inode_security *landlock_inode(const struct inode
> *const inode)
> 
> Mimi
> 
> 
>> -	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 984e861f6e33..37c2a228f0e1 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 cea0afbbc28d..05cfb04cd02b 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;
>>   }
>>   
>> @@ -1118,7 +1112,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.
>> @@ -1193,6 +1187,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),
>>   };
>>   
>> @@ -1210,7 +1205,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) = {
> 

Re: [PATCH v2 3/7] ima: Ensure lock is held when setting iint pointer in inode security blob

[Roberto Sassu]

On 1/14/2025 3:20 PM, Mimi Zohar wrote:
> On Thu, 2024-11-28 at 11:06 +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.
>>
>> Ensure that the iint check and assignment is guarded, by adding a lockdep
>> assertion in ima_inode_get().
> 
> -> is guarded by the ima_iint_cache_lock mutex, ...

By the iint_lock mutex...

>> 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);
>> +
> 
> lockdep_assert_held() doesn't actually "ensure" the lock is held, but emits a warning
> when the lock is not held (if debugging is enabled).  Semantically "ensure" gives the
> impression of enforcing.

I agree. I would replace ensure with detect.

Thanks

Roberto

> Mimi
> 
>>   	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 05cfb04cd02b..1e474ff6a777 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);
>>   }
>>   
>>   /**
> 

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

[Mimi Zohar]

Please use "__fput()" rather than "file close".  Perhaps update the subject line to
something like "ima: Defer fixing security.ima to __fput()". 

On Thu, 2024-11-28 at 11:06 +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.  
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Roberto, I really like the idea of removing the inode_lock in process_measurement()
needed for writing xattrs, but I'm concerned about the delay being introduced.  For
example, does it interfere with labeling the filesystem with file signatures
(with/without EVM enabled)?

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

Probably better to reverse the "mode & FMODE_WRITE" and atomic_read() test order.

Mimi

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

Re: [PATCH v2 6/7] ima: Discard files opened with O_PATH

[Mimi Zohar]

On Thu, 2024-11-28 at 11:06 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> According to man open.2, files opened with O_PATH are not really opened. The
> obtained file descriptor is used to indicate a location in the filesystem
> tree and to perform operations that act purely at the file descriptor
> level.
> 
> Thus, ignore open() syscalls with O_PATH, since IMA cares about file data.
> 
> Cc: stable@vger.kernel.org # v2.6.39.x
> Fixes: 1abf0c718f15a ("New kind of open files - "location only".")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Thanks, Roberto.

Note: Ignoring open() with O_PATH impacts policies containing "func=FILE_CHECK"
rules.

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

> ---
>  security/integrity/ima/ima_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 50b37420ea2c..712c3a522e6c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -202,7 +202,8 @@ static void ima_file_free(struct file *file)
>  	struct inode *inode = file_inode(file);
>  	struct ima_iint_cache *iint;
>  
> -	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> +	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
> +	    (file->f_flags & O_PATH))
>  		return;
>  
>  	iint = ima_iint_find(inode);
> @@ -232,7 +233,8 @@ static int process_measurement(struct file *file, const struct
> cred *cred,
>  	enum hash_algo hash_algo;
>  	unsigned int allowed_algos = 0;
>  
> -	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
> +	if (!ima_policy_flag || !S_ISREG(inode->i_mode) ||
> +	    (file->f_flags & O_PATH))
>  		return 0;
>  
>  	/* Return an IMA_MEASURE, IMA_APPRAISE, IMA_AUDIT action

Re: [PATCH v2 7/7] ima: Reset IMA_NONACTION_RULE_FLAGS after post_setattr

[Mimi Zohar]

On Thu, 2024-11-28 at 11:06 +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").
> 
> Restore the original change, but with new mask 0xfb000000 since the
> policy-specific flags changed meanwhile, and rename IMA_ACTION_RULE_FLAGS
> to IMA_NONACTION_RULE_FLAGS, to be consistent with IMA_NONACTION_FLAGS.

Thanks, Roberto.  Please summarize the reason for reverting the change.  Something
like:  Restore the original change to not reset the new file status after ...

> 
> Cc: stable@vger.kernel.org # v4.16.x
> Fixes: 11c60f23ed13 ("integrity: Remove unused macro IMA_ACTION_RULE_FLAGS")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.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 22c3b87cfcac..32ffef2cc92a 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 712c3a522e6c..83e467ad18d4 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -277,7 +277,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

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

[Roberto Sassu]

On Wed, 2025-01-15 at 08:46 -0500, Mimi Zohar wrote:
> Please use "__fput()" rather than "file close".  Perhaps update the subject line to
> something like "ima: Defer fixing security.ima to __fput()". 
> 
> On Thu, 2024-11-28 at 11:06 +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.  
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Roberto, I really like the idea of removing the inode_lock in process_measurement()
> needed for writing xattrs, but I'm concerned about the delay being introduced.  For
> example, does it interfere with labeling the filesystem with file signatures
> (with/without EVM enabled)?

There will be a difference when EVM is enabled, and inode metadata are
corrupted.

In that case, currently IMA in fix mode is able to fix inode metadata
as well, by writing security.ima. That happens because IMA ignores the
result of evm_verifyxattr() and writes the xattr directly, causing EVM
to update the HMAC to a valid one.

With the new patch, the EVM HMAC remains invalid until file close,
meaning that it will not be possible for example to set xattr on the
opened file descriptor. It works after closing the file though.

Setting other LSMs xattrs will fail as well, if the EVM HMAC is
invalid.

If the problem is EVM, I would recommend setting evm=fix as well, so
that inode metadata can be properly fixed.

I will update the documentation to describe the limitation introduced
by this patch, and to suggest to use evm=fix.

Thanks

Roberto

> > ---
> >  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 --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)) {
> 
> Probably better to reverse the "mode & FMODE_WRITE" and atomic_read() test order.
> 
> Mimi
> 
> >  		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);