[PATCH 1/3] ima: Remove inode lock

From: Roberto Sassu
Date: Tue Oct 08 2024 - 12:58:26 EST

Next message: Roberto Sassu: "[PATCH 3/3] ima: Mark concurrent accesses to the iint pointer in the inode security blob"
Previous message: Roberto Sassu: "[PATCH 2/3] ima: Ensure lock is held when setting iint pointer in inode security blob"
Next in thread: Roberto Sassu: "[PATCH 2/3] ima: Ensure lock is held when setting iint pointer in inode security blob"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>

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 simplify accessing the new structure in
the inode security blob.

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@xxxxxxxxxx>
---
security/integrity/ima/ima.h | 26 ++++++++---
security/integrity/ima/ima_api.c | 4 +-
security/integrity/ima/ima_iint.c | 77 ++++++++++++++++++++++++++-----
security/integrity/ima/ima_main.c | 39 +++++++---------
4 files changed, 104 insertions(+), 42 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3c323ca213d4..6474a15b584a 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,48 @@ struct ima_iint_cache {
struct ima_digest_data *ima_hash;
};

+struct ima_iint_cache_lock {
+ struct mutex mutex; /* protects: version, flags, digest */
+ struct ima_iint_cache *iint;
+};
+
extern struct lsm_blob_sizes ima_blob_sizes;

+static inline struct ima_iint_cache_lock *ima_inode_security(void *i_security)
+{
+ return i_security + ima_blob_sizes.lbs_inode;
+}
+
static inline struct ima_iint_cache *
ima_inode_get_iint(const struct inode *inode)
{
- struct ima_iint_cache **iint_sec;
+ struct ima_iint_cache_lock *iint_lock;

if (unlikely(!inode->i_security))
return NULL;

- iint_sec = inode->i_security + ima_blob_sizes.lbs_inode;
- return *iint_sec;
+ iint_lock = ima_inode_security(inode->i_security);
+ return iint_lock->iint;
}

static inline void ima_inode_set_iint(const struct inode *inode,
struct ima_iint_cache *iint)
{
- struct ima_iint_cache **iint_sec;
+ struct ima_iint_cache_lock *iint_lock;

if (unlikely(!inode->i_security))
return;

- iint_sec = inode->i_security + ima_blob_sizes.lbs_inode;
- *iint_sec = iint;
+ iint_lock = ima_inode_security(inode->i_security);
+ iint_lock->iint = iint;
}

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 00b249101f98..c176fd0faae7 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -40,18 +40,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
}

@@ -68,14 +68,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);
}

@@ -108,6 +105,26 @@ struct ima_iint_cache *ima_inode_get(struct inode *inode)
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
@@ -116,11 +133,49 @@ 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);
+
+ /* iint_lock->iint should be NULL if !IS_IMA(inode) */
+ 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);

- /* *iint_p should be NULL if !IS_IMA(inode) */
- 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 06132cf47016..7852212c43ce 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.34.1

Next message: Roberto Sassu: "[PATCH 3/3] ima: Mark concurrent accesses to the iint pointer in the inode security blob"
Previous message: Roberto Sassu: "[PATCH 2/3] ima: Ensure lock is held when setting iint pointer in inode security blob"
Next in thread: Roberto Sassu: "[PATCH 2/3] ima: Ensure lock is held when setting iint pointer in inode security blob"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]