| 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
| ||
|
Date: Fri, 1 Feb 2019 18:57:32 -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>,
Andrew Cooper <andrew.cooper3@...rix.com>
Subject: Re: [PATCH v5 05/13] x86/fsgsbase/64: Enable FSGSBASE instructions in
the helper functions
On Fri, Feb 1, 2019 at 12:54 PM Chang S. Bae <chang.seok.bae@...el.com> wrote:
>
> The helper functions will switch on faster accesses to FSBASE and GSBASE
> when the FSGSBASE feature is enabled.
>
> Accessing user GSBASE needs a couple of SWAPGS operations. It is avoidable
> if the user GSBASE is saved at kernel entry, being updated as changes, and
> restored back at kernel exit. However, it seems to spend more cycles for
> savings and restorations. Little or no benefit was measured from
> experiments.
>
> Also, introduce __{rd,wr}gsbase_inactive() as helpers to access user GSBASE
> with SWAPGS. Note, for Xen PV, paravirt hooks can be added, since it may
> allow a very efficient but different implementation.
>
> [ Use NOKPROBE_SYMBOL instead of __kprobes ]
^^^ This line looks like it shold be deleted.
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
> Cc: Any Lutomirski <luto@...nel.org>
> Cc: H. Peter Anvin <hpa@...or.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Andrew Cooper <andrew.cooper3@...rix.com>
> ---
> arch/x86/include/asm/fsgsbase.h | 27 +++++++-------
> arch/x86/kernel/process_64.c | 62 +++++++++++++++++++++++++++++++--
> 2 files changed, 72 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/include/asm/fsgsbase.h b/arch/x86/include/asm/fsgsbase.h
> index fdd1177499b4..aefd53767a5d 100644
> --- a/arch/x86/include/asm/fsgsbase.h
> +++ b/arch/x86/include/asm/fsgsbase.h
> @@ -49,35 +49,32 @@ static __always_inline void wrgsbase(unsigned long gsbase)
> asm volatile("wrgsbase %0" :: "r" (gsbase) : "memory");
> }
>
> +#include <asm/cpufeature.h>
> +
> /* Helper functions for reading/writing FS/GS base */
>
> static inline unsigned long x86_fsbase_read_cpu(void)
> {
> unsigned long fsbase;
>
> - rdmsrl(MSR_FS_BASE, fsbase);
> + if (static_cpu_has(X86_FEATURE_FSGSBASE))
> + fsbase = rdfsbase();
> + else
> + rdmsrl(MSR_FS_BASE, fsbase);
>
> return fsbase;
> }
>
> -static inline unsigned long x86_gsbase_read_cpu_inactive(void)
> -{
> - unsigned long gsbase;
> -
> - rdmsrl(MSR_KERNEL_GS_BASE, gsbase);
> -
> - return gsbase;
> -}
> -
> static inline void x86_fsbase_write_cpu(unsigned long fsbase)
> {
> - wrmsrl(MSR_FS_BASE, fsbase);
> + if (static_cpu_has(X86_FEATURE_FSGSBASE))
> + wrfsbase(fsbase);
> + else
> + wrmsrl(MSR_FS_BASE, fsbase);
> }
>
> -static inline void x86_gsbase_write_cpu_inactive(unsigned long gsbase)
> -{
> - wrmsrl(MSR_KERNEL_GS_BASE, gsbase);
> -}
> +extern unsigned long x86_gsbase_read_cpu_inactive(void);
> +extern void x86_gsbase_write_cpu_inactive(unsigned long gsbase);
>
> #endif /* CONFIG_X86_64 */
>
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 6a62f4af9fcf..ebc55ed31fe7 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -160,6 +160,42 @@ enum which_selector {
> GS
> };
>
> +/*
> + * Interrupts are disabled here. Out of line to be protected
> + * from kprobes. It is not used on Xen paravirt. When paravirt
> + * support is needed, it needs to be renamed with native_ prefix.
> + */
> +static noinline unsigned long __rdgsbase_inactive(void)
> +{
> + unsigned long gsbase, flags;
> +
> + local_irq_save(flags);
> + native_swapgs();
> + gsbase = rdgsbase();
> + native_swapgs();
> + local_irq_restore(flags);
> +
> + return gsbase;
> +}
> +NOKPROBE_SYMBOL(__rdgsbase_inactive);
> +
> +/*
> + * Interrupts are disabled here. Out of line to be protected
> + * from kprobes. It is not used on Xen paravirt. When paravirt
> + * support is needed, it needs to be renamed with native_ prefix.
> + */
> +static noinline void __wrgsbase_inactive(unsigned long gsbase)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + native_swapgs();
> + wrgsbase(gsbase);
> + native_swapgs();
> + local_irq_restore(flags);
> +}
> +NOKPROBE_SYMBOL(__wrgsbase_inactive);
> +
> /*
> * Saves the FS or GS base for an outgoing thread if FSGSBASE extensions are
> * not available. The goal is to be reasonably fast on non-FSGSBASE systems.
> @@ -338,13 +374,34 @@ static unsigned long x86_fsgsbase_read_task(struct task_struct *task,
> return base;
> }
>
> +unsigned long x86_gsbase_read_cpu_inactive(void)
> +{
> + unsigned long gsbase;
> +
> + if (static_cpu_has(X86_FEATURE_FSGSBASE))
> + gsbase = __rdgsbase_inactive();
> + else
> + rdmsrl(MSR_KERNEL_GS_BASE, gsbase);
> +
> + return gsbase;
> +}
> +
> +void x86_gsbase_write_cpu_inactive(unsigned long gsbase)
> +{
> + if (static_cpu_has(X86_FEATURE_FSGSBASE))
> + __wrgsbase_inactive(gsbase);
> + else
> + wrmsrl(MSR_KERNEL_GS_BASE, gsbase);
> +}
> +
> unsigned long x86_fsbase_read_task(struct task_struct *task)
> {
> unsigned long fsbase;
>
> if (task == current)
> fsbase = x86_fsbase_read_cpu();
> - else if (task->thread.fsindex == 0)
> + else if (static_cpu_has(X86_FEATURE_FSGSBASE) ||
> + (task->thread.fsindex == 0))
> fsbase = task->thread.fsbase;
> else
> fsbase = x86_fsgsbase_read_task(task, task->thread.fsindex);
> @@ -358,7 +415,8 @@ unsigned long x86_gsbase_read_task(struct task_struct *task)
>
> if (task == current)
> gsbase = x86_gsbase_read_cpu_inactive();
> - else if (task->thread.gsindex == 0)
> + else if (static_cpu_has(X86_FEATURE_FSGSBASE) ||
> + (task->thread.gsindex == 0))
> gsbase = task->thread.gsbase;
> else
> gsbase = x86_fsgsbase_read_task(task, task->thread.gsindex);
These last two hunks changes do not belong in this patch. Presumably
they belong in patch 6.
--Andy
> --
> 2.19.1
>
Powered by blists - more mailing lists