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