On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
Ping guys. Thanks.
On Mon, Sep 7, 2020 at 9:53 PM Muchun Song <[email protected]> wrote:
>
> On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >> ---
> > >> 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
On 07.09.2020 16:53, Muchun Song wrote:
> On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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
On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <[email protected]> wrote:
>
> On 07.09.2020 16:53, Muchun Song wrote:
> > On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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
On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <[email protected]> wrote:
>
> On 07.09.2020 16:53, Muchun Song wrote:
> > On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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
On 22.09.2020 08:59, Muchun Song wrote:
> On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <[email protected]> wrote:
>>
>> On 07.09.2020 16:53, Muchun Song wrote:
>>> On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> 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
>
>
>
On Mon, Sep 28, 2020 at 2:32 PM Alexander Popov <[email protected]> wrote:
>
> On 22.09.2020 08:59, Muchun Song wrote:
> > On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <[email protected]> wrote:
> >>
> >> On 07.09.2020 16:53, Muchun Song wrote:
> >>> On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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