Messages in this thread Patch in this message | | | Date | Thu, 11 Apr 2024 23:28:44 -0700 | From | Pawan Gupta <> | 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@kernel.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); }
| |