Re: [External] Re: [PATCH v2] stackleak: Fix a race between stack erasing sysctl handlers

[Alexander Popov <alex.popov@linux.com>]

On 22.09.2020 08:59, Muchun Song wrote:
> On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <alex.popov@linux.com> wrote:
>>
>> On 07.09.2020 16:53, Muchun Song wrote:
>>> On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <alex.popov@linux.com> wrote:
>>>>
>>>> On 07.09.2020 05:54, Muchun Song wrote:
>>>>> Hi all,
>>>>>
>>>>> Any comments or suggestions? Thanks.
>>>>>
>>>>> On Fri, Aug 28, 2020 at 11:19 AM Muchun Song <songmuchun@bytedance.com> wrote:
>>>>>>
>>>>>> There is a race between the assignment of `table->data` and write value
>>>>>> to the pointer of `table->data` in the __do_proc_doulongvec_minmax() on
>>>>>> the other thread.
>>>>>>
>>>>>>     CPU0:                                 CPU1:
>>>>>>                                           proc_sys_write
>>>>>>     stack_erasing_sysctl                    proc_sys_call_handler
>>>>>>       table->data = &state;                   stack_erasing_sysctl
>>>>>>                                                 table->data = &state;
>>>>>>       proc_doulongvec_minmax
>>>>>>         do_proc_doulongvec_minmax             sysctl_head_finish
>>>>>>           __do_proc_doulongvec_minmax           unuse_table
>>>>>>             i = table->data;
>>>>>>             *i = val;  // corrupt CPU1's stack
>>>>
>>>> Hello everyone!
>>>>
>>>> As I remember, I implemented stack_erasing_sysctl() very similar to other sysctl
>>>> handlers. Is that issue relevant for other handlers as well?
>>>
>>> Yeah, it's very similar. But the difference is that others use a
>>> global variable as the
>>> `table->data`, but here we use a local variable as the `table->data`.
>>> The local variable
>>> is allocated from the stack. So other thread could corrupt the stack
>>> like the diagram
>>> above.
>>
>> Hi Muchun,
>>
>> I don't think that the proposed copying of struct ctl_table to local variable is
>> a good fix of that issue. There might be other bugs caused by concurrent
>> execution of stack_erasing_sysctl().
> 
> Hi Alexander,
> 
> Yeah, we can fix this issue on a higher level in kernel/sysctl.c. But
> we will rework some kernel/sysctl.c base code. Because the commit:
> 
>     964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing")
> 
> is introduced from linux-4.20. So we should backport this fix patch to the other
> stable tree. Be the safe side, we can apply this patch to only fix the
> stack_erasing_sysctl. In this case, the impact of backport is minimal.
> 
> In the feature, we can fix the issue(another patch) like this on a higher
> level in kernel/sysctl.c and only apply it in the later kernel version. Is
> this OK?

Muchun, I would recommend:
  1) fixing the reason of the issue in kernel/sysctl.c
or
  2) use some locking in stack_erasing_sysctl() to fix the issue locally.

Honestly, I don't like this "dup_table" approach in the patch below. It doesn't
remove the data race.

Thank you!
Alexander

>> I would recommend using some locking instead.
>>
>> But you say there are other similar issues. Should it be fixed on higher level
>> in kernel/sysctl.c?
>>
>> [Adding more knowing people to CC]
>>
>> Thanks!
>>
>>>> Muchun, could you elaborate how CPU1's stack is corrupted and how you detected
>>>> that? Thanks!
>>>
>>> Why did I find this problem? Because I solve another problem which is
>>> very similar to
>>> this issue. You can reference the following fix patch. Thanks.
>>>
>>>   https://lkml.org/lkml/2020/8/22/105
>>>>
>>>>>> Fix this by duplicating the `table`, and only update the duplicate of
>>>>>> it.
>>>>>>
>>>>>> Fixes: 964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing")
>>>>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>>>>> ---
>>>>>> changelogs in v2:
>>>>>>  1. Add more details about how the race happened to the commit message.
>>>>>>
>>>>>>  kernel/stackleak.c | 11 ++++++++---
>>>>>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/kernel/stackleak.c b/kernel/stackleak.c
>>>>>> index a8fc9ae1d03d..fd95b87478ff 100644
>>>>>> --- a/kernel/stackleak.c
>>>>>> +++ b/kernel/stackleak.c
>>>>>> @@ -25,10 +25,15 @@ int stack_erasing_sysctl(struct ctl_table *table, int write,
>>>>>>         int ret = 0;
>>>>>>         int state = !static_branch_unlikely(&stack_erasing_bypass);
>>>>>>         int prev_state = state;
>>>>>> +       struct ctl_table dup_table = *table;
>>>>>>
>>>>>> -       table->data = &state;
>>>>>> -       table->maxlen = sizeof(int);
>>>>>> -       ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
>>>>>> +       /*
>>>>>> +        * In order to avoid races with __do_proc_doulongvec_minmax(), we
>>>>>> +        * can duplicate the @table and alter the duplicate of it.
>>>>>> +        */
>>>>>> +       dup_table.data = &state;
>>>>>> +       dup_table.maxlen = sizeof(int);
>>>>>> +       ret = proc_dointvec_minmax(&dup_table, write, buffer, lenp, ppos);
>>>>>>         state = !!state;
>>>>>>         if (ret || !write || state == prev_state)
>>>>>>                 return ret;
>>>>>> --
>>>>>> 2.11.0
> 
> 
>