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

[Alexander Popov]

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@xxxxxxxxxxxxx> 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?

Muchun, could you elaborate how CPU1's stack is corrupted and how you detected
that? Thanks!

Best regards,
Alexander

>> 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@xxxxxxxxxxxxx>
>> ---
>> 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
>>
> 
>