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 <[email protected]>
Signed-off-by: Zheng Zengkai <[email protected]>
---
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];
--
2.20.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 <[email protected]>
> Signed-off-by: Zheng Zengkai <[email protected]>
Nacked-by: Tetsuo Handa <[email protected]>
> ---
> 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];
>
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 <[email protected]>
>> Signed-off-by: Zheng Zengkai <[email protected]>
> Nacked-by: Tetsuo Handa <[email protected]>
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];
>>
> .
>
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.
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!
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.