On Thu, 2024-11-28 at 11:06 +0100, Roberto Sassu wrote: > From: Roberto Sassu <[email protected]> > > 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 <[email protected]> > --- > 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); > } > > /**
