linux-kernel - Re: [PATCH v5 07/13] x86/fsgsbase/64: When copying a thread, use the FSGSBASE instructions if available

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 2 Feb 2019 09:28:42 -0800
From:   Andy Lutomirski <luto@...nel.org>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>, Andi Kleen <ak@...ux.intel.com>,
        Markus T Metzger <markus.t.metzger@...el.com>,
        Ravi Shankar <ravi.v.shankar@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 07/13] x86/fsgsbase/64: When copying a thread, use the
 FSGSBASE instructions if available

On Fri, Feb 1, 2019 at 12:54 PM Chang S. Bae <chang.seok.bae@...el.com> wrote:
>
> Copy real FS/GSBASE values instead of approximation when FSGSBASE is
> enabled.
>
> Factoring out to save_fsgs() does not result in the same behavior because
> save_base_legacy() does not copy FS/GSBASE when the index is zero.
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: H. Peter Anvin <hpa@...or.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Andi Kleen <ak@...ux.intel.com>
> ---
>  arch/x86/kernel/process_64.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index d8ade9530fdb..648e43b58c69 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -477,10 +477,16 @@ int copy_thread_tls(unsigned long clone_flags, unsigned long sp,
>         p->thread.sp = (unsigned long) fork_frame;
>         p->thread.io_bitmap_ptr = NULL;
>
> -       savesegment(gs, p->thread.gsindex);
> -       p->thread.gsbase = p->thread.gsindex ? 0 : me->thread.gsbase;
>         savesegment(fs, p->thread.fsindex);
> -       p->thread.fsbase = p->thread.fsindex ? 0 : me->thread.fsbase;
> +       savesegment(gs, p->thread.gsindex);
> +       if (static_cpu_has(X86_FEATURE_FSGSBASE)) {
> +               p->thread.fsbase = rdfsbase();
> +               p->thread.gsbase = __rdgsbase_inactive();
> +       } else {
> +               /* save_base_legacy() does not set base when index is zero. */

After looking at this a bit, I propose that we just clean this up all
the way.  Can't this whole mess be changed to:

save_fsgs(me);
p->thread.fsindex = me->thread.fsindex;
p->thread.fsbase = me->thread.fsbase;
p->thread.gsindex = me->thread.gsindex;
p->thread.gsbase = me->thread.gsbase;

This will avoid all of the horrible tracing through the logic to
figure out why the code is correct.

Sure, it'll be a few cycles slower with FSGSBASE, but this isn't
really a fast path, and if we ever really care, we can optimize it
later.