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().
I can not figure out how the bug happened when there is concurrent
execution of stack_erasing_sysctl().
>
> 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?
Yeah, we can see the same issue here.
https://lkml.org/lkml/2020/8/22/105.
I agree with you. Maybe a fix on the higher level is a good choice in
kernel/sysctl.c. If someone also agrees with this solution, I can do
this work.
>
> [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
