Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200828031928.43584-1-songmuchun@bytedance.com>
 <CAMZfGtWtAYNexRq1xf=5At1+YJ+_TtN=F6bVnm9EPuqRnMuroA@mail.gmail.com>
 <8c288fd4-2ef7-ca47-1f3b-e4167944b235@linux.com> <CAMZfGtXsXWtHh_G0TWm=DxG_5xT6kN_BbfqNgoQvTRu89FJihA@mail.gmail.com>
In-Reply-To: <CAMZfGtXsXWtHh_G0TWm=DxG_5xT6kN_BbfqNgoQvTRu89FJihA@mail.gmail.com>
From:   Muchun Song <songmuchun@bytedance.com>
Date:   Fri, 11 Sep 2020 11:53:16 +0800
Message-ID: <CAMZfGtW44zaQYQTG3twx82adZH-MTd=B5zESAeFCz-zYki=EYA@mail.gmail.com>
Subject: Re: [External] Re: [PATCH v2] stackleak: Fix a race between stack
 erasing sysctl handlers
To:     alex.popov@linux.com, Kees Cook <keescook@chromium.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        miguel.ojeda.sandonis@gmail.com
Cc:     LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Ping guys. Thanks.

On Mon, Sep 7, 2020 at 9:53 PM Muchun Song <songmuchun@bytedance.com> 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.
>
> >
> > 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
>
>
>
>
> >
> > 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@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
> > >>
> > >
> > >
> >
>
>
> --
> Yours,
> Muchun


-- 
Yours,
Muchun