linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
       [not found]   ` <CAHC9VhT-Thu6KppC-MWzqkB7R1CaQA9DWXOQnG0b2uS9+rvzoA@mail.gmail.com>
@ 2018-09-13  6:26     ` Tetsuo Handa
  2018-09-13  7:12       ` peter enderborg
  2018-09-13 19:23       ` Paul Moore
  0 siblings, 2 replies; 8+ messages in thread
From: Tetsuo Handa @ 2018-09-13  6:26 UTC (permalink / raw)
  To: Paul Moore
  Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel,
	peter.enderborg, Stephen Smalley, syzkaller-bugs, linux-mm

On 2018/09/13 12:02, Paul Moore wrote:
> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>> syzbot is hitting warning at str_read() [1] because len parameter can
>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
>> this case.
>>
>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
>>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
>> ---
>>  security/selinux/ss/policydb.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
>> index e9394e7..f4eadd3 100644
>> --- a/security/selinux/ss/policydb.c
>> +++ b/security/selinux/ss/policydb.c
>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>>         if ((len == 0) || (len == (u32)-1))
>>                 return -EINVAL;
>>
>> -       str = kmalloc(len + 1, flags);
>> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
>>         if (!str)
>>                 return -ENOMEM;
> 
> Thanks for the patch.
> 
> My eyes are starting to glaze over a bit chasing down all of the
> different kmalloc() code paths trying to ensure that this always does
> the right thing based on size of the allocation and the different slab
> allocators ... are we sure that this will always return NULL when (len
> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
> configurations?
> 

Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
ZERO_SIZE_PTR) due to (len == (u32)-1) check above.

The only concern would be whether you want allocation failure messages.
I assumed you don't need it because we are returning -ENOMEM to the caller.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
  2018-09-13  6:26     ` [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() Tetsuo Handa
@ 2018-09-13  7:12       ` peter enderborg
  2018-09-13 11:11         ` Michal Hocko
  2018-09-13 19:28         ` Paul Moore
  2018-09-13 19:23       ` Paul Moore
  1 sibling, 2 replies; 8+ messages in thread
From: peter enderborg @ 2018-09-13  7:12 UTC (permalink / raw)
  To: Tetsuo Handa, Paul Moore
  Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel,
	Stephen Smalley, syzkaller-bugs, linux-mm

On 09/13/2018 08:26 AM, Tetsuo Handa wrote:
> On 2018/09/13 12:02, Paul Moore wrote:
>> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>> syzbot is hitting warning at str_read() [1] because len parameter can
>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
>>> this case.
>>>
>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
>>>
>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
>>> ---
>>>  security/selinux/ss/policydb.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
>>> index e9394e7..f4eadd3 100644
>>> --- a/security/selinux/ss/policydb.c
>>> +++ b/security/selinux/ss/policydb.c
>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>>>         if ((len == 0) || (len == (u32)-1))
>>>                 return -EINVAL;
>>>
>>> -       str = kmalloc(len + 1, flags);
>>> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
>>>         if (!str)
>>>                 return -ENOMEM;
>> Thanks for the patch.
>>
>> My eyes are starting to glaze over a bit chasing down all of the
>> different kmalloc() code paths trying to ensure that this always does
>> the right thing based on size of the allocation and the different slab
>> allocators ... are we sure that this will always return NULL when (len
>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
>> configurations?
>>
> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
> ZERO_SIZE_PTR) due to (len == (u32)-1) check above.
>
> The only concern would be whether you want allocation failure messages.
> I assumed you don't need it because we are returning -ENOMEM to the caller.
>
Would it not be better with

    char *str;

    if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE))
        return -EINVAL;

    str = kmalloc(len + 1, flags);
    if (!str)
        return -ENOMEM;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
  2018-09-13  7:12       ` peter enderborg
@ 2018-09-13 11:11         ` Michal Hocko
  2018-09-13 12:55           ` peter enderborg
  2018-09-13 19:28         ` Paul Moore
  1 sibling, 1 reply; 8+ messages in thread
From: Michal Hocko @ 2018-09-13 11:11 UTC (permalink / raw)
  To: peter enderborg
  Cc: Tetsuo Handa, Paul Moore, selinux, syzbot+ac488b9811036cea7ea0,
	Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs,
	linux-mm

On Thu 13-09-18 09:12:04, peter enderborg wrote:
> On 09/13/2018 08:26 AM, Tetsuo Handa wrote:
> > On 2018/09/13 12:02, Paul Moore wrote:
> >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
> >> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >>> syzbot is hitting warning at str_read() [1] because len parameter can
> >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
> >>> this case.
> >>>
> >>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
> >>>
> >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
> >>> ---
> >>>  security/selinux/ss/policydb.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> >>> index e9394e7..f4eadd3 100644
> >>> --- a/security/selinux/ss/policydb.c
> >>> +++ b/security/selinux/ss/policydb.c
> >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
> >>>         if ((len == 0) || (len == (u32)-1))
> >>>                 return -EINVAL;
> >>>
> >>> -       str = kmalloc(len + 1, flags);
> >>> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
> >>>         if (!str)
> >>>                 return -ENOMEM;
> >> Thanks for the patch.
> >>
> >> My eyes are starting to glaze over a bit chasing down all of the
> >> different kmalloc() code paths trying to ensure that this always does
> >> the right thing based on size of the allocation and the different slab
> >> allocators ... are we sure that this will always return NULL when (len
> >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
> >> configurations?
> >>
> > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
> > ZERO_SIZE_PTR) due to (len == (u32)-1) check above.
> >
> > The only concern would be whether you want allocation failure messages.
> > I assumed you don't need it because we are returning -ENOMEM to the caller.
> >
> Would it not be better with
> 
>     char *str;
> 
>     if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE))
>         return -EINVAL;
> 
>     str = kmalloc(len + 1, flags);
>     if (!str)
>         return -ENOMEM;

I strongly suspect that you want kvmalloc rather than kmalloc here. The
larger the request the more likely is the allocation to fail.

I am not familiar with the code but I assume this is a root only
interface so we don't have to worry about nasty users scenario.

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
  2018-09-13 11:11         ` Michal Hocko
@ 2018-09-13 12:55           ` peter enderborg
  2018-09-13 13:03             ` Dmitry Vyukov
  2018-09-13 19:35             ` Paul Moore
  0 siblings, 2 replies; 8+ messages in thread
From: peter enderborg @ 2018-09-13 12:55 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Tetsuo Handa, Paul Moore, selinux, syzbot+ac488b9811036cea7ea0,
	Eric Paris, linux-kernel, Stephen Smalley, syzkaller-bugs,
	linux-mm

On 09/13/2018 01:11 PM, Michal Hocko wrote:
> On Thu 13-09-18 09:12:04, peter enderborg wrote:
>> On 09/13/2018 08:26 AM, Tetsuo Handa wrote:
>>> On 2018/09/13 12:02, Paul Moore wrote:
>>>> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
>>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>>>> syzbot is hitting warning at str_read() [1] because len parameter can
>>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
>>>>> this case.
>>>>>
>>>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
>>>>>
>>>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
>>>>> ---
>>>>>  security/selinux/ss/policydb.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
>>>>> index e9394e7..f4eadd3 100644
>>>>> --- a/security/selinux/ss/policydb.c
>>>>> +++ b/security/selinux/ss/policydb.c
>>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>>>>>         if ((len == 0) || (len == (u32)-1))
>>>>>                 return -EINVAL;
>>>>>
>>>>> -       str = kmalloc(len + 1, flags);
>>>>> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
>>>>>         if (!str)
>>>>>                 return -ENOMEM;
>>>> Thanks for the patch.
>>>>
>>>> My eyes are starting to glaze over a bit chasing down all of the
>>>> different kmalloc() code paths trying to ensure that this always does
>>>> the right thing based on size of the allocation and the different slab
>>>> allocators ... are we sure that this will always return NULL when (len
>>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
>>>> configurations?
>>>>
>>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
>>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above.
>>>
>>> The only concern would be whether you want allocation failure messages.
>>> I assumed you don't need it because we are returning -ENOMEM to the caller.
>>>
>> Would it not be better with
>>
>>     char *str;
>>
>>     if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE))
>>         return -EINVAL;
>>
>>     str = kmalloc(len + 1, flags);
>>     if (!str)
>>         return -ENOMEM;
> I strongly suspect that you want kvmalloc rather than kmalloc here. The
> larger the request the more likely is the allocation to fail.
>
> I am not familiar with the code but I assume this is a root only
> interface so we don't have to worry about nasty users scenario.
>
I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option.

And some of the calls are GFP_ATOMC.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
  2018-09-13 12:55           ` peter enderborg
@ 2018-09-13 13:03             ` Dmitry Vyukov
  2018-09-13 19:35             ` Paul Moore
  1 sibling, 0 replies; 8+ messages in thread
From: Dmitry Vyukov @ 2018-09-13 13:03 UTC (permalink / raw)
  To: peter enderborg
  Cc: Michal Hocko, Tetsuo Handa, Paul Moore, SELinux,
	syzbot+ac488b9811036cea7ea0, Eric Paris, LKML, Stephen Smalley,
	syzkaller-bugs, linux-mm

On Thu, Sep 13, 2018 at 2:55 PM, peter enderborg
<peter.enderborg@sony.com> wrote:
>>>>>> syzbot is hitting warning at str_read() [1] because len parameter can
>>>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
>>>>>> this case.
>>>>>>
>>>>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
>>>>>>
>>>>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>>>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
>>>>>> ---
>>>>>>  security/selinux/ss/policydb.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
>>>>>> index e9394e7..f4eadd3 100644
>>>>>> --- a/security/selinux/ss/policydb.c
>>>>>> +++ b/security/selinux/ss/policydb.c
>>>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
>>>>>>         if ((len == 0) || (len == (u32)-1))
>>>>>>                 return -EINVAL;
>>>>>>
>>>>>> -       str = kmalloc(len + 1, flags);
>>>>>> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
>>>>>>         if (!str)
>>>>>>                 return -ENOMEM;
>>>>> Thanks for the patch.
>>>>>
>>>>> My eyes are starting to glaze over a bit chasing down all of the
>>>>> different kmalloc() code paths trying to ensure that this always does
>>>>> the right thing based on size of the allocation and the different slab
>>>>> allocators ... are we sure that this will always return NULL when (len
>>>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
>>>>> configurations?
>>>>>
>>>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
>>>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above.
>>>>
>>>> The only concern would be whether you want allocation failure messages.
>>>> I assumed you don't need it because we are returning -ENOMEM to the caller.
>>>>
>>> Would it not be better with
>>>
>>>     char *str;
>>>
>>>     if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE))
>>>         return -EINVAL;
>>>
>>>     str = kmalloc(len + 1, flags);
>>>     if (!str)
>>>         return -ENOMEM;
>> I strongly suspect that you want kvmalloc rather than kmalloc here. The
>> larger the request the more likely is the allocation to fail.
>>
>> I am not familiar with the code but I assume this is a root only
>> interface so we don't have to worry about nasty users scenario.
>>
> I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option.
>
> And some of the calls are GFP_ATOMC.

Then another option is to introduce reasonable application-specific
limit and not rely on kmalloc-anything at all. We did this for some
instances of this warning too. One advantage of it is that it prevents
users from doing silly things (or maybe will discover bugs in
user-space code better, why are they asking for megs here?). Another
advantage is that what works on one version of kernel will continue to
work on another version of kernel. Today it's possible that a policy
works on one kernel with 4MB kmalloc limit, but breaks on another with
2MB limit. Ideally exact value of KMALLOC_MAX_SIZE does not affect
anything in user-space.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
  2018-09-13  6:26     ` [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() Tetsuo Handa
  2018-09-13  7:12       ` peter enderborg
@ 2018-09-13 19:23       ` Paul Moore
  1 sibling, 0 replies; 8+ messages in thread
From: Paul Moore @ 2018-09-13 19:23 UTC (permalink / raw)
  To: penguin-kernel
  Cc: selinux, syzbot+ac488b9811036cea7ea0, Eric Paris, linux-kernel,
	peter.enderborg, Stephen Smalley, syzkaller-bugs, linux-mm

On Thu, Sep 13, 2018 at 2:26 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> On 2018/09/13 12:02, Paul Moore wrote:
> > On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
> > <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >> syzbot is hitting warning at str_read() [1] because len parameter can
> >> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
> >> this case.
> >>
> >> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
> >>
> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
> >> ---
> >>  security/selinux/ss/policydb.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> >> index e9394e7..f4eadd3 100644
> >> --- a/security/selinux/ss/policydb.c
> >> +++ b/security/selinux/ss/policydb.c
> >> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
> >>         if ((len == 0) || (len == (u32)-1))
> >>                 return -EINVAL;
> >>
> >> -       str = kmalloc(len + 1, flags);
> >> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
> >>         if (!str)
> >>                 return -ENOMEM;
> >
> > Thanks for the patch.
> >
> > My eyes are starting to glaze over a bit chasing down all of the
> > different kmalloc() code paths trying to ensure that this always does
> > the right thing based on size of the allocation and the different slab
> > allocators ... are we sure that this will always return NULL when (len
> > + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
> > configurations?
>
> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
> ZERO_SIZE_PTR) due to (len == (u32)-1) check above.
>
> The only concern would be whether you want allocation failure messages.
> I assumed you don't need it because we are returning -ENOMEM to the caller.

I'm not to worried about the failure messages, returning -ENOMEM
should be sufficient in this case.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
  2018-09-13  7:12       ` peter enderborg
  2018-09-13 11:11         ` Michal Hocko
@ 2018-09-13 19:28         ` Paul Moore
  1 sibling, 0 replies; 8+ messages in thread
From: Paul Moore @ 2018-09-13 19:28 UTC (permalink / raw)
  To: peter.enderborg
  Cc: penguin-kernel, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris,
	linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm

On Thu, Sep 13, 2018 at 3:12 AM peter enderborg
<peter.enderborg@sony.com> wrote:
> On 09/13/2018 08:26 AM, Tetsuo Handa wrote:
> > On 2018/09/13 12:02, Paul Moore wrote:
> >> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
> >> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >>> syzbot is hitting warning at str_read() [1] because len parameter can
> >>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
> >>> this case.
> >>>
> >>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
> >>>
> >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
> >>> ---
> >>>  security/selinux/ss/policydb.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> >>> index e9394e7..f4eadd3 100644
> >>> --- a/security/selinux/ss/policydb.c
> >>> +++ b/security/selinux/ss/policydb.c
> >>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
> >>>         if ((len == 0) || (len == (u32)-1))
> >>>                 return -EINVAL;
> >>>
> >>> -       str = kmalloc(len + 1, flags);
> >>> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
> >>>         if (!str)
> >>>                 return -ENOMEM;
> >> Thanks for the patch.
> >>
> >> My eyes are starting to glaze over a bit chasing down all of the
> >> different kmalloc() code paths trying to ensure that this always does
> >> the right thing based on size of the allocation and the different slab
> >> allocators ... are we sure that this will always return NULL when (len
> >> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
> >> configurations?
> >>
> > Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
> > ZERO_SIZE_PTR) due to (len == (u32)-1) check above.
> >
> > The only concern would be whether you want allocation failure messages.
> > I assumed you don't need it because we are returning -ENOMEM to the caller.
> >
> Would it not be better with
>
>     char *str;
>
>     if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE))
>         return -EINVAL;
>
>     str = kmalloc(len + 1, flags);
>     if (!str)
>         return -ENOMEM;

As long as it's safe, I'd rather leave the maximum allocation limit as
a kmalloc internal and let kmalloc return NULL if we try too large of
an allocation.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read()
  2018-09-13 12:55           ` peter enderborg
  2018-09-13 13:03             ` Dmitry Vyukov
@ 2018-09-13 19:35             ` Paul Moore
  1 sibling, 0 replies; 8+ messages in thread
From: Paul Moore @ 2018-09-13 19:35 UTC (permalink / raw)
  To: penguin-kernel, peter.enderborg
  Cc: mhocko, selinux, syzbot+ac488b9811036cea7ea0, Eric Paris,
	linux-kernel, Stephen Smalley, syzkaller-bugs, linux-mm

On Thu, Sep 13, 2018 at 8:55 AM peter enderborg
<peter.enderborg@sony.com> wrote:
> On 09/13/2018 01:11 PM, Michal Hocko wrote:
> > On Thu 13-09-18 09:12:04, peter enderborg wrote:
> >> On 09/13/2018 08:26 AM, Tetsuo Handa wrote:
> >>> On 2018/09/13 12:02, Paul Moore wrote:
> >>>> On Fri, Sep 7, 2018 at 12:43 PM Tetsuo Handa
> >>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >>>>> syzbot is hitting warning at str_read() [1] because len parameter can
> >>>>> become larger than KMALLOC_MAX_SIZE. We don't need to emit warning for
> >>>>> this case.
> >>>>>
> >>>>> [1] https://syzkaller.appspot.com/bug?id=7f2f5aad79ea8663c296a2eedb81978401a908f0
> >>>>>
> >>>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> >>>>> Reported-by: syzbot <syzbot+ac488b9811036cea7ea0@syzkaller.appspotmail.com>
> >>>>> ---
> >>>>>  security/selinux/ss/policydb.c | 2 +-
> >>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> >>>>> index e9394e7..f4eadd3 100644
> >>>>> --- a/security/selinux/ss/policydb.c
> >>>>> +++ b/security/selinux/ss/policydb.c
> >>>>> @@ -1101,7 +1101,7 @@ static int str_read(char **strp, gfp_t flags, void *fp, u32 len)
> >>>>>         if ((len == 0) || (len == (u32)-1))
> >>>>>                 return -EINVAL;
> >>>>>
> >>>>> -       str = kmalloc(len + 1, flags);
> >>>>> +       str = kmalloc(len + 1, flags | __GFP_NOWARN);
> >>>>>         if (!str)
> >>>>>                 return -ENOMEM;
> >>>> Thanks for the patch.
> >>>>
> >>>> My eyes are starting to glaze over a bit chasing down all of the
> >>>> different kmalloc() code paths trying to ensure that this always does
> >>>> the right thing based on size of the allocation and the different slab
> >>>> allocators ... are we sure that this will always return NULL when (len
> >>>> + 1) is greater than KMALLOC_MAX_SIZE for the different slab allocator
> >>>> configurations?
> >>>>
> >>> Yes, for (len + 1) cannot become 0 (which causes kmalloc() to return
> >>> ZERO_SIZE_PTR) due to (len == (u32)-1) check above.
> >>>
> >>> The only concern would be whether you want allocation failure messages.
> >>> I assumed you don't need it because we are returning -ENOMEM to the caller.
> >>>
> >> Would it not be better with
> >>
> >>     char *str;
> >>
> >>     if ((len == 0) || (len == (u32)-1) || (len >= KMALLOC_MAX_SIZE))
> >>         return -EINVAL;
> >>
> >>     str = kmalloc(len + 1, flags);
> >>     if (!str)
> >>         return -ENOMEM;
> > I strongly suspect that you want kvmalloc rather than kmalloc here. The
> > larger the request the more likely is the allocation to fail.
> >
> > I am not familiar with the code but I assume this is a root only
> > interface so we don't have to worry about nasty users scenario.
> >
> I don't think we get any big data there at all. Usually less than 32 bytes. However this data can be in fast path so a vmalloc is not an option.
>
> And some of the calls are GFP_ATOMC.

Based on all the comments it looks like Tetsuo's original patch is
probably the best fix right now.  I'm going to merge this into
selinux/next.

Tetsuo, thanks for the patch, and thanks to everyone else for the
comments/review.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-09-13 19:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <000000000000038dab0575476b73@google.com>
     [not found] ` <f3bcebc6-47a7-518e-70f7-c7e167621841@I-love.SAKURA.ne.jp>
     [not found]   ` <CAHC9VhT-Thu6KppC-MWzqkB7R1CaQA9DWXOQnG0b2uS9+rvzoA@mail.gmail.com>
2018-09-13  6:26     ` [PATCH] selinux: Add __GFP_NOWARN to allocation at str_read() Tetsuo Handa
2018-09-13  7:12       ` peter enderborg
2018-09-13 11:11         ` Michal Hocko
2018-09-13 12:55           ` peter enderborg
2018-09-13 13:03             ` Dmitry Vyukov
2018-09-13 19:35             ` Paul Moore
2018-09-13 19:28         ` Paul Moore
2018-09-13 19:23       ` Paul Moore

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).