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

[Muchun Song]

On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <alex.popov@xxxxxxxxx> 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@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?

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


--
Yours,
Muchun