diff mbox series

tomoyo: Avoid potential null pointer access

Message ID 20201125121043.107662-1-zhengzengkai@huawei.com (mailing list archive)
State New
Headers show
Series tomoyo: Avoid potential null pointer access | expand

Commit Message

Zheng Zengkai Nov. 25, 2020, 12:10 p.m. UTC
Calls to kzalloc() should be null-checked in order to avoid
any potential failures or unnecessary code execution.
Fix this by adding null checks for _entry_ right after allocation.

Fixes: 57c2590fb7fd ("TOMOYO: Update profile structure")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com>
---
 security/tomoyo/common.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tetsuo Handa Nov. 25, 2020, 12:25 p.m. UTC | #1
Hello, Zheng.

Thank you for a patch, but I won't apply this patch.
Expected behavior is that tomoyo_warn_oom() is called
if tomoyo_memory_ok() is called with entry == NULL.

Adding __GFP_NOWARN might be OK, but returning without tomoyo_warn_oom() is NG.

On 2020/11/25 21:10, Zheng Zengkai wrote:
> Calls to kzalloc() should be null-checked in order to avoid
> any potential failures or unnecessary code execution.
> Fix this by adding null checks for _entry_ right after allocation.
> 
> Fixes: 57c2590fb7fd ("TOMOYO: Update profile structure")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com>

Nacked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

> ---
>  security/tomoyo/common.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index 4bee32bfe16d..99b4fafcb100 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -499,6 +499,8 @@ static struct tomoyo_profile *tomoyo_assign_profile
>  	if (ptr)
>  		return ptr;
>  	entry = kzalloc(sizeof(*entry), GFP_NOFS);
> +	if (!entry)
> +		return NULL;
>  	if (mutex_lock_interruptible(&tomoyo_policy_lock))
>  		goto out;
>  	ptr = ns->profile_ptr[profile];
>
Zheng Zengkai Nov. 26, 2020, 6:33 a.m. UTC | #2
Hello, Tetsuo

Got it , Thank you for your explanation.

> Hello, Zheng.
>
> Thank you for a patch, but I won't apply this patch.
> Expected behavior is that tomoyo_warn_oom() is called
> if tomoyo_memory_ok() is called with entry == NULL.
>
> Adding __GFP_NOWARN might be OK, but returning without tomoyo_warn_oom() is NG.
>
> On 2020/11/25 21:10, Zheng Zengkai wrote:
>> Calls to kzalloc() should be null-checked in order to avoid
>> any potential failures or unnecessary code execution.
>> Fix this by adding null checks for _entry_ right after allocation.
>>
>> Fixes: 57c2590fb7fd ("TOMOYO: Update profile structure")
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Zheng Zengkai <zhengzengkai@huawei.com>
> Nacked-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

As your say,  I found the function tomoyo_assign_namespace( )

in security/tomoyo/domain.c has the similar situation,

Can I add __GFP_NOWARN for both and remove the null check for _entry_ in 
tomoyo_assign_namespace( )?

diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 4bee32bfe16d..bc54d3c8c70a 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -498,7 +498,7 @@ static struct tomoyo_profile *tomoyo_assign_profile
         ptr = ns->profile_ptr[profile];
         if (ptr)
                 return ptr;
-       entry = kzalloc(sizeof(*entry), GFP_NOFS);
+       entry = kzalloc(sizeof(*entry), GFP_NOFS | __GFP_NOWARN);
         if (mutex_lock_interruptible(&tomoyo_policy_lock))
                 goto out;
         ptr = ns->profile_ptr[profile];
diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index dc4ecc0b2038..c6e5cc5cc7cd 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -473,9 +473,7 @@ struct tomoyo_policy_namespace 
*tomoyo_assign_namespace(const char *domainname)
                 return ptr;
         if (len >= TOMOYO_EXEC_TMPSIZE - 10 || 
!tomoyo_domain_def(domainname))
                 return NULL;
-       entry = kzalloc(sizeof(*entry) + len + 1, GFP_NOFS);
-       if (!entry)
-               return NULL;
+       entry = kzalloc(sizeof(*entry) + len + 1, GFP_NOFS | __GFP_NOWARN);
         if (mutex_lock_interruptible(&tomoyo_policy_lock))
                 goto out;
         ptr = tomoyo_find_namespace(domainname, len);

>> ---
>>   security/tomoyo/common.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
>> index 4bee32bfe16d..99b4fafcb100 100644
>> --- a/security/tomoyo/common.c
>> +++ b/security/tomoyo/common.c
>> @@ -499,6 +499,8 @@ static struct tomoyo_profile *tomoyo_assign_profile
>>   	if (ptr)
>>   		return ptr;
>>   	entry = kzalloc(sizeof(*entry), GFP_NOFS);
>> +	if (!entry)
>> +		return NULL;
>>   	if (mutex_lock_interruptible(&tomoyo_policy_lock))
>>   		goto out;
>>   	ptr = ns->profile_ptr[profile];
>>
> .
>
Tetsuo Handa Nov. 26, 2020, 6:57 a.m. UTC | #3
On 2020/11/26 15:33, Zheng Zengkai wrote:
> As your say,  I found the function tomoyo_assign_namespace( )
> 
> in security/tomoyo/domain.c has the similar situation,
> 
> Can I add __GFP_NOWARN for both and remove the null check for _entry_ in tomoyo_assign_namespace( )?
> 

Good catch. Yes, please send as a patch.
Zheng Zengkai Nov. 27, 2020, 7:17 a.m. UTC | #4
Hello Tetsuo,
> On 2020/11/26 15:33, Zheng Zengkai wrote:
>> As your say,  I found the function tomoyo_assign_namespace( )
>>
>> in security/tomoyo/domain.c has the similar situation,
>>
>> Can I add __GFP_NOWARN for both and remove the null check for _entry_ in tomoyo_assign_namespace( )?
>>
> Good catch. Yes, please send as a patch.
> .

I have resent a patch, thanks!
Tetsuo Handa Nov. 27, 2020, 10:52 a.m. UTC | #5
On 2020/11/27 16:17, Zheng Zengkai wrote:
> Hello Tetsuo,
>> On 2020/11/26 15:33, Zheng Zengkai wrote:
>>> As your say,  I found the function tomoyo_assign_namespace( )
>>>
>>> in security/tomoyo/domain.c has the similar situation,
>>>
>>> Can I add __GFP_NOWARN for both and remove the null check for _entry_ in tomoyo_assign_namespace( )?
>>>
>> Good catch. Yes, please send as a patch.
>> .
> 
> I have resent a patch, thanks!
> 

Applied to tomoyo-test1.git tree. Thank you.

By the way, since some people automatically backport patches with Fixes: tag,
I think we don't need to add Fixes: tag for patches like this one.
diff mbox series

Patch

diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 4bee32bfe16d..99b4fafcb100 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -499,6 +499,8 @@  static struct tomoyo_profile *tomoyo_assign_profile
 	if (ptr)
 		return ptr;
 	entry = kzalloc(sizeof(*entry), GFP_NOFS);
+	if (!entry)
+		return NULL;
 	if (mutex_lock_interruptible(&tomoyo_policy_lock))
 		goto out;
 	ptr = ns->profile_ptr[profile];