Linux-Fsdevel Archive on lore.kernel.org help / color / mirror / Atom feed
From: Muchun Song <firstname.lastname@example.org> To: email@example.com Cc: Kees Cook <firstname.lastname@example.org>, Masami Hiramatsu <email@example.com>, Steven Rostedt <firstname.lastname@example.org>, email@example.com, LKML <firstname.lastname@example.org>, Luis Chamberlain <email@example.com>, Iurii Zaikin <firstname.lastname@example.org>, email@example.com, Mike Kravetz <firstname.lastname@example.org> Subject: Re: [External] Re: [PATCH v2] stackleak: Fix a race between stack erasing sysctl handlers Date: Tue, 22 Sep 2020 13:59:52 +0800 [thread overview] Message-ID: <CAMZfGtVhrgvWqCG140e7S5wn00ocS5L_t=KFNpbsfXhc293rSg@mail.gmail.com> (raw) In-Reply-To: <email@example.com> On Mon, Sep 14, 2020 at 9:56 PM Alexander Popov <firstname.lastname@example.org> wrote: > > On 07.09.2020 16:53, Muchun Song wrote: > > On Mon, Sep 7, 2020 at 7:24 PM Alexander Popov <email@example.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 <firstname.lastname@example.org> 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@example.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
next prev parent reply other threads:[~2020-09-22 6:00 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <firstname.lastname@example.org> [not found] ` <CAMZfGtWtAYNexRq1xf=5At1+YJ+_TtN=F6bVnm9EPuqRnMuroA@mail.gmail.com> [not found] ` <email@example.com> [not found] ` <CAMZfGtXsXWtHh_G0TWm=DxG_5xT6kN_BbfqNgoQvTRu89FJihA@mail.gmail.com> 2020-09-14 13:56 ` [External] Re: [PATCH v2] stackleak: Fix a race between stack erasing sysctl handlers Alexander Popov 2020-09-14 14:09 ` Muchun Song 2020-09-22 5:59 ` Muchun Song [this message] 2020-09-28 6:32 ` Alexander Popov 2020-09-28 7:30 ` Muchun Song
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAMZfGtVhrgvWqCG140e7S5wn00ocS5L_t=KFNpbsfXhc293rSg@mail.gmail.com' \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).