* [PATCH] integrity: Fix possible multiple allocation in integrity_inode_get()
@ 2023-05-30 12:14 Tianjia Zhang
2023-05-30 13:58 ` Mimi Zohar
2023-06-01 6:42 ` [PATCH v2] " Tianjia Zhang
0 siblings, 2 replies; 6+ messages in thread
From: Tianjia Zhang @ 2023-05-30 12:14 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, linux-security-module,
linux-kernel
Cc: Tianjia Zhang
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(-)
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] integrity: Fix possible multiple allocation in integrity_inode_get()
2023-05-30 12:14 [PATCH] integrity: Fix possible multiple allocation in integrity_inode_get() Tianjia Zhang
@ 2023-05-30 13:58 ` Mimi Zohar
2023-06-01 6:42 ` [PATCH v2] " Tianjia Zhang
1 sibling, 0 replies; 6+ messages in thread
From: Mimi Zohar @ 2023-05-30 13:58 UTC (permalink / raw)
To: Tianjia Zhang, Dmitry Kasatkin, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, linux-security-module,
linux-kernel
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
--
thanks,
Mimi
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()
2023-05-30 12:14 [PATCH] integrity: Fix possible multiple allocation in integrity_inode_get() Tianjia Zhang
2023-05-30 13:58 ` Mimi Zohar
@ 2023-06-01 6:42 ` Tianjia Zhang
2023-06-05 11:52 ` Mimi Zohar
2023-06-09 14:24 ` Jarkko Sakkinen
1 sibling, 2 replies; 6+ messages in thread
From: Tianjia Zhang @ 2023-06-01 6:42 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, linux-security-module,
linux-kernel
Cc: Tianjia Zhang
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(-)
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)
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()
2023-06-01 6:42 ` [PATCH v2] " Tianjia Zhang
@ 2023-06-05 11:52 ` Mimi Zohar
2023-06-09 14:24 ` Jarkko Sakkinen
1 sibling, 0 replies; 6+ messages in thread
From: Mimi Zohar @ 2023-06-05 11:52 UTC (permalink / raw)
To: Tianjia Zhang, Dmitry Kasatkin, Paul Moore, James Morris,
Serge E. Hallyn, linux-integrity, linux-security-module,
linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()
2023-06-01 6:42 ` [PATCH v2] " Tianjia Zhang
2023-06-05 11:52 ` Mimi Zohar
@ 2023-06-09 14:24 ` Jarkko Sakkinen
2023-06-15 9:08 ` Tianjia Zhang
1 sibling, 1 reply; 6+ messages in thread
From: Jarkko Sakkinen @ 2023-06-09 14:24 UTC (permalink / raw)
To: Tianjia Zhang, Mimi Zohar, Dmitry Kasatkin, Paul Moore,
James Morris, Serge E. Hallyn, linux-integrity,
linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()
2023-06-09 14:24 ` Jarkko Sakkinen
@ 2023-06-15 9:08 ` Tianjia Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Tianjia Zhang @ 2023-06-15 9:08 UTC (permalink / raw)
To: Jarkko Sakkinen, Mimi Zohar, Dmitry Kasatkin, Paul Moore,
James Morris, Serge E. Hallyn, linux-integrity,
linux-security-module, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-15 9:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-30 12:14 [PATCH] integrity: Fix possible multiple allocation in integrity_inode_get() Tianjia Zhang
2023-05-30 13:58 ` Mimi Zohar
2023-06-01 6:42 ` [PATCH v2] " Tianjia Zhang
2023-06-05 11:52 ` Mimi Zohar
2023-06-09 14:24 ` Jarkko Sakkinen
2023-06-15 9:08 ` Tianjia Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).