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 <[email protected]>
Cc: Dmitry Kasatkin <[email protected]>
Cc: <[email protected]> # 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;
--
2.24.3 (Apple Git-128)
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 <[email protected]>
> Cc: Dmitry Kasatkin <[email protected]>
> Cc: <[email protected]> # 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
--
thanks,
Mimi
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 <[email protected]>
Cc: Dmitry Kasatkin <[email protected]>
Cc: <[email protected]> # v3.10+
---
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)
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 <[email protected]>
> Cc: Dmitry Kasatkin <[email protected]>
> Cc: <[email protected]> # 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 <[email protected]>
> Cc: Dmitry Kasatkin <[email protected]>
> Cc: <[email protected]> # 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 <[email protected]>
>> Cc: Dmitry Kasatkin <[email protected]>
>> Cc: <[email protected]> # 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