| Message ID | 20241128100621.461743-4-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | New |
| 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> > > 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); > } > > /**
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); >> } >> >> /** >
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); } /**