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]

Date:   Mon, 7 Sep 2020 10:54:23 +0800
From:   Muchun Song <songmuchun@...edance.com>
To:     Kees Cook <keescook@...omium.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>, alex.popov@...ux.com,
        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

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


-- 
Yours,
Muchun