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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c288fd4-2ef7-ca47-1f3b-e4167944b235@linux.com>
Date:   Mon, 7 Sep 2020 14:24:08 +0300
From:   Alexander Popov <alex.popov@...ux.com>
To:     Muchun Song <songmuchun@...edance.com>,
        Kees Cook <keescook@...omium.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        miguel.ojeda.sandonis@...il.com
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] stackleak: Fix a race between stack erasing sysctl
 handlers

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@...edance.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?

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@...edance.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
>>
> 
>