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