[v2] integrity: Fix possible multiple allocation in integrity_inode_get()

[PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()

Posted by Tianjia Zhang 2 years, 8 months ago

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)

Re: [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()

Posted by Jarkko Sakkinen 2 years, 8 months ago

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

Re: [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()

Posted by Tianjia Zhang 2 years, 7 months ago

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

Re: [PATCH v2] integrity: Fix possible multiple allocation in integrity_inode_get()

Posted by Mimi Zohar 2 years, 8 months ago

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