[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240412062844.p5j33tmqjggladgl@desk>
Date: Thu, 11 Apr 2024 23:28:44 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
Linus Torvalds <torvalds@...ux-foundation.org>,
Daniel Sneddon <daniel.sneddon@...ux.intel.com>,
Thomas Gleixner <tglx@...utronix.de>,
Alexandre Chartre <alexandre.chartre@...cle.com>,
Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
Peter Zijlstra <peterz@...radead.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Sean Christopherson <seanjc@...gle.com>,
Andrew Cooper <andrew.cooper3@...rix.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Nikolay Borisov <nik.borisov@...e.com>,
KP Singh <kpsingh@...nel.org>, Waiman Long <longman@...hat.com>,
Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH 5/7] x86/bugs: Only harden syscalls when needed
On Wed, Apr 10, 2024 at 10:40:49PM -0700, Josh Poimboeuf wrote:
> Syscall hardening (i.e., converting the syscall indirect branch to a
> series of direct branches) may cause performance regressions in certain
> scenarios. Only use the syscall hardening when indirect branches are
> considered unsafe.
>
> Fixes: 1e3ad78334a6 ("x86/syscall: Don't force use of indirect calls for system calls")
> Signed-off-by: Josh Poimboeuf <jpoimboe@...nel.org>
> ---
> arch/x86/entry/common.c | 30 +++++++++++++++++++++++++---
> arch/x86/entry/syscall_32.c | 11 +---------
> arch/x86/entry/syscall_64.c | 8 +-------
> arch/x86/entry/syscall_x32.c | 7 ++++++-
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/include/asm/syscall.h | 8 +++++++-
> arch/x86/kernel/cpu/bugs.c | 32 +++++++++++++++++++++++++++++-
> 7 files changed, 74 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 6de50b80702e..80d432d2fe44 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -39,6 +39,28 @@
>
> #ifdef CONFIG_X86_64
>
> +/*
> + * Do either a direct or an indirect call, depending on whether indirect calls
> + * are considered safe.
> + */
> +#define __do_syscall(table, func_direct, nr, regs) \
> +({ \
> + unsigned long __rax, __rdi, __rsi; \
> + \
> + asm_inline volatile( \
> + ALTERNATIVE("call " __stringify(func_direct) "\n\t", \
> + ANNOTATE_RETPOLINE_SAFE \
> + "call *%[func_ptr]\n\t", \
> + X86_FEATURE_INDIRECT_SAFE) \
> + : "=D" (__rdi), "=S" (__rsi), "=a" (__rax), \
> + ASM_CALL_CONSTRAINT \
> + : "0" (regs), "1" (nr), [func_ptr] "r" (table[nr]) \
> + : "rdx", "rcx", "r8", "r9", "r10", "r11", \
> + "cc", "memory"); \
> + \
> + __rax; \
> +})
This is a nice implementation, but I think we can avoid the complexity
by using cpu_feature_enabled(). As cpu_feature_enabled() is also runtime
patched, atleast the likely() path should be comparable to this. Please
let me know if you have any concerns with this approach.
---
diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
index 6de50b80702e..7c5332b83246 100644
--- a/arch/x86/entry/common.c
+++ b/arch/x86/entry/common.c
@@ -49,7 +49,11 @@ static __always_inline bool do_syscall_x64(struct pt_regs *regs, int nr)
if (likely(unr < NR_syscalls)) {
unr = array_index_nospec(unr, NR_syscalls);
- regs->ax = x64_sys_call(regs, unr);
+ if (likely(cpu_feature_enabled(X86_FEATURE_INDIRECT_SAFE)))
+ regs->ax = sys_call_table[unr](regs);
+ else
+ regs->ax = x64_sys_call(regs, unr);
+
return true;
}
return false;
@@ -66,7 +70,11 @@ static __always_inline bool do_syscall_x32(struct pt_regs *regs, int nr)
if (IS_ENABLED(CONFIG_X86_X32_ABI) && likely(xnr < X32_NR_syscalls)) {
xnr = array_index_nospec(xnr, X32_NR_syscalls);
- regs->ax = x32_sys_call(regs, xnr);
+ if (likely(cpu_feature_enabled(X86_FEATURE_INDIRECT_SAFE)))
+ regs->ax = x32_sys_call_table[xnr](regs);
+ else
+ regs->ax = x32_sys_call(regs, xnr);
+
return true;
}
return false;
@@ -162,7 +170,10 @@ static __always_inline void do_syscall_32_irqs_on(struct pt_regs *regs, int nr)
if (likely(unr < IA32_NR_syscalls)) {
unr = array_index_nospec(unr, IA32_NR_syscalls);
- regs->ax = ia32_sys_call(regs, unr);
+ if (likely(cpu_feature_enabled(X86_FEATURE_INDIRECT_SAFE)))
+ regs->ax = ia32_sys_call_table[unr](regs);
+ else
+ regs->ax = ia32_sys_call(regs, unr);
} else if (nr != -1) {
regs->ax = __ia32_sys_ni_syscall(regs);
}
Powered by blists - more mailing lists