| Message ID | 20241128100621.461743-3-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ima: Remove unnecessary inode locks | expand |
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) = {
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) = { >
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) = {