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

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

[Muchun Song]

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

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

[Muchun Song]

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

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

[Alexander Popov]

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?

Muchun, could you elaborate how CPU1's stack is corrupted and how you detected
that? Thanks!

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

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

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

[Muchun Song]

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

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

[Alexander Popov]

On 07.09.2020 16:53, Muchun Song 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.

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

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

[Muchun Song]

On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <alex.popov@linux.com> wrote:
>
> On 07.09.2020 16:53, Muchun Song 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.
>
> 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@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

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

[Muchun Song]

On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <alex.popov@linux.com> wrote:
>
> On 07.09.2020 16:53, Muchun Song 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.
>
> 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?

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

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

[Alexander Popov]

On 22.09.2020 08:59, Muchun Song wrote:
> On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <alex.popov@linux.com> wrote:
>>
>> On 07.09.2020 16:53, Muchun Song 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.
>>
>> 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.

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

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

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@linux.com> wrote:
>
> On 22.09.2020 08:59, Muchun Song wrote:
> > On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <alex.popov@linux.com> wrote:
> >>
> >> On 07.09.2020 16:53, Muchun Song 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.
> >>
> >> 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@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