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
