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

[Muchun Song]

On Mon, Sep 28, 2020 at 2:32 PM Alexander Popov <alex.popov@xxxxxxxxx> wrote:
>
> On 22.09.2020 08:59, Muchun Song wrote:
> > On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <alex.popov@xxxxxxxxx> wrote:
> >>
> >> On 07.09.2020 16:53, Muchun Song wrote:
> >>> 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.
> >>
> >> Hi Muchun,
> >>
> >> I don't think that the proposed copying of struct ctl_table to local variable is
> >> a good fix of that issue. There might be other bugs caused by concurrent
> >> execution of stack_erasing_sysctl().
> >
> > Hi Alexander,
> >
> > Yeah, we can fix this issue on a higher level in kernel/sysctl.c. But
> > we will rework some kernel/sysctl.c base code. Because the commit:
> >
> >     964c9dff0091 ("stackleak: Allow runtime disabling of kernel stack erasing")
> >
> > is introduced from linux-4.20. So we should backport this fix patch to the other
> > stable tree. Be the safe side, we can apply this patch to only fix the
> > stack_erasing_sysctl. In this case, the impact of backport is minimal.
> >
> > In the feature, we can fix the issue(another patch) like this on a higher
> > level in kernel/sysctl.c and only apply it in the later kernel version. Is
> > this OK?
>
> Muchun, I would recommend:
>   1) fixing the reason of the issue in kernel/sysctl.c
> or
>   2) use some locking in stack_erasing_sysctl() to fix the issue locally.

Yeah, this is work.

>
> Honestly, I don't like this "dup_table" approach in the patch below. It doesn't
> remove the data race.

Alexander, I don't understand where the race is? I think that the duplicate is
enough. But If you prefer using the lock to protect the data. I also
can do that.

>
> Thank you!
> Alexander
>
> >> I would recommend using some locking instead.
> >>
> >> But you say there are other similar issues. Should it be fixed on higher level
> >> in kernel/sysctl.c?
> >>
> >> [Adding more knowing people to CC]
> >>
> >> Thanks!
> >>
> >>>> 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
> >>>>
> >>>>>> 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