| Message ID | 20230530121453.10249-1-tianjia.zhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | integrity: Fix possible multiple allocation in integrity_inode_get() | expand |
Hi Tianjia, On Tue, 2023-05-30 at 20:14 +0800, Tianjia Zhang wrote: > When integrity_inode_get() is querying and inserting the cache, there > is a conditional race in the concurrent environment. > > Query iint within the read-lock. If there is no result, allocate iint > first and insert the iint cache in the write-lock protection. When the > iint cache does not exist, and when multiple execution streams come at > the same time, there will be a race condition, and multiple copies of > iint will be allocated at the same time, and then put into the cache > one by one under the write-lock protection. Right, the race condition is the result of not properly implementing "double-checked locking". In this case, it first checks to see if the iint cache record exists before taking the lock, but doesn't check again after taking the integrity_iint_lock. > > This is mainly because the red-black tree insertion does not perform > duplicate detection. This is not the desired result, when this > happens, the repeated allocation should be freed and the existing > iint cache should be returned. > > Fixes: bf2276d10ce5 ("ima: allocating iint improvements") > Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: <stable@vger.kernel.org> # v3.10+ > --- > security/integrity/iint.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index c73858e8c6d5..d49c843a88ee 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -43,12 +43,10 @@ static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode) > else if (inode > iint->inode) > n = n->rb_right; > else > - break; > + return iint; > } > - if (!n) > - return NULL; > > - return iint; > + return NULL; > } > > /* > @@ -115,8 +113,13 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) > rb_node); > if (inode < test_iint->inode) > p = &(*p)->rb_left; > - else > + else if (inode > test_iint->inode) > p = &(*p)->rb_right; > + else { > + write_unlock(&integrity_iint_lock); > + kmem_cache_free(iint_cache, iint); > + return test_iint; > + } > } > > iint->inode = inode; scripts/checkpatch.pl with the -strict option complains: CHECK: Unbalanced braces around else statement #56: FILE: security/integrity/iint.c:118: + else { total: 0 errors, 0 warnings, 1 checks, 28 lines checked
diff --git a/security/integrity/iint.c b/security/integrity/iint.c index c73858e8c6d5..d49c843a88ee 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -43,12 +43,10 @@ static struct integrity_iint_cache *__integrity_iint_find(struct inode *inode) else if (inode > iint->inode) n = n->rb_right; else - break; + return iint; } - if (!n) - return NULL; - return iint; + return NULL; } /* @@ -115,8 +113,13 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) rb_node); if (inode < test_iint->inode) p = &(*p)->rb_left; - else + else if (inode > test_iint->inode) p = &(*p)->rb_right; + else { + write_unlock(&integrity_iint_lock); + kmem_cache_free(iint_cache, iint); + return test_iint; + } } iint->inode = inode;
When integrity_inode_get() is querying and inserting the cache, there is a conditional race in the concurrent environment. Query iint within the read-lock. If there is no result, allocate iint first and insert the iint cache in the write-lock protection. When the iint cache does not exist, and when multiple execution streams come at the same time, there will be a race condition, and multiple copies of iint will be allocated at the same time, and then put into the cache one by one under the write-lock protection. This is mainly because the red-black tree insertion does not perform duplicate detection. This is not the desired result, when this happens, the repeated allocation should be freed and the existing iint cache should be returned. Fixes: bf2276d10ce5 ("ima: allocating iint improvements") Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> Cc: <stable@vger.kernel.org> # v3.10+ --- security/integrity/iint.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)