| Message ID | 20230601064244.33633-1-tianjia.zhang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] integrity: Fix possible multiple allocation in integrity_inode_get() | expand |
On Thu, 2023-06-01 at 14:42 +0800, Tianjia Zhang wrote: > When integrity_inode_get() is querying and inserting the cache, there > is a conditional race in the concurrent environment. > > 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. > > 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+ Thanks, Tianjia. The patch is now queued in next-integrity. Mimi
On Thu Jun 1, 2023 at 9:42 AM EEST, Tianjia Zhang wrote: > When integrity_inode_get() is querying and inserting the cache, there > is a conditional race in the concurrent environment. > > 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. > > 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+ s/v3.10/v4.14/ I.e. cover only currently maintained longterms, right? > --- > security/integrity/iint.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index c73858e8c6d5..a462df827de2 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; > } > > /* > @@ -113,10 +111,15 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) > parent = *p; > test_iint = rb_entry(parent, struct integrity_iint_cache, > rb_node); > - if (inode < test_iint->inode) > + 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; > -- > 2.24.3 (Apple Git-128) Mimi, are you picking this? Off-topic: how do you compile kernel on macOS, you're using VM right? I'm just interested because I recently bought Mac mini for both compiling and testing arm64. Optimal would be to be able to compile the kernel on bare metal and then deploy to a VM... BR, Jarkko
Hi Jarkko, On 6/9/23 10:24 PM, Jarkko Sakkinen wrote: > On Thu Jun 1, 2023 at 9:42 AM EEST, Tianjia Zhang wrote: >> When integrity_inode_get() is querying and inserting the cache, there >> is a conditional race in the concurrent environment. >> >> 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. >> >> 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+ > > s/v3.10/v4.14/ > > I.e. cover only currently maintained longterms, right? > Yes, the race condition was indeed introduced in 3.10, but the fix is estimated to only cover the LTS version. > >> --- >> security/integrity/iint.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/security/integrity/iint.c b/security/integrity/iint.c >> index c73858e8c6d5..a462df827de2 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; >> } >> >> /* >> @@ -113,10 +111,15 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) >> parent = *p; >> test_iint = rb_entry(parent, struct integrity_iint_cache, >> rb_node); >> - if (inode < test_iint->inode) >> + 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; >> -- >> 2.24.3 (Apple Git-128) > > Mimi, are you picking this? Mimi has picked this patch in next-integrity. > > Off-topic: how do you compile kernel on macOS, you're using VM right? > I'm just interested because I recently bought Mac mini for both > compiling and testing arm64. Optimal would be to be able to compile > the kernel on bare metal and then deploy to a VM... > I am currently only coding and sending the final patch on a Mac. Compilation and testing are still carried out in the linux environment. If you have experience in launching a linux VM on macOS, please share it with me, thanks. Best regards, Tianjia
diff --git a/security/integrity/iint.c b/security/integrity/iint.c index c73858e8c6d5..a462df827de2 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; } /* @@ -113,10 +111,15 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode) parent = *p; test_iint = rb_entry(parent, struct integrity_iint_cache, rb_node); - if (inode < test_iint->inode) + 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. 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. 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 | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)