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

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

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

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