| Message ID | 20201125121043.107662-1-zhengzengkai@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tomoyo: Avoid potential null pointer access | expand |
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]; >
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]; >> > . >
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.
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];
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(+)