Re: [PATCH v13 8/8] x86/vsyscall/64: Fixup Shadow Stack and Indirect Branch Tracking for vsyscall emulation
From: Andy Lutomirski <luto@kernel.org>
Date: 2020-09-25 16:31:57
Also in:
linux-api, linux-arch, linux-mm, lkml
On Fri, Sep 25, 2020 at 7:58 AM Yu-cheng Yu [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Vsyscall entry points are effectively branch targets. Mark them with ENDBR64 opcodes. When emulating the RET instruction, unwind shadow stack and reset IBT state machine. Signed-off-by: Yu-cheng Yu <redacted> --- v13: - Check shadow stack address is canonical. - Change from writing to MSRs to writing to CET xstate. arch/x86/entry/vsyscall/vsyscall_64.c | 34 +++++++++++++++++++++++ arch/x86/entry/vsyscall/vsyscall_emu_64.S | 9 ++++++ arch/x86/entry/vsyscall/vsyscall_trace.h | 1 + 3 files changed, 44 insertions(+)diff --git a/arch/x86/entry/vsyscall/vsyscall_64.c b/arch/x86/entry/vsyscall/vsyscall_64.c index 44c33103a955..315ee3572664 100644 --- a/arch/x86/entry/vsyscall/vsyscall_64.c +++ b/arch/x86/entry/vsyscall/vsyscall_64.c@@ -38,6 +38,9 @@ #include <asm/fixmap.h> #include <asm/traps.h> #include <asm/paravirt.h> +#include <asm/fpu/xstate.h> +#include <asm/fpu/types.h> +#include <asm/fpu/internal.h> #define CREATE_TRACE_POINTS #include "vsyscall_trace.h"@@ -286,6 +289,37 @@ bool emulate_vsyscall(unsigned long error_code, /* Emulate a ret instruction. */ regs->ip = caller; regs->sp += 8; + +#ifdef CONFIG_X86_CET + if (tsk->thread.cet.shstk_size || tsk->thread.cet.ibt_enabled) { + struct cet_user_state *cet; + struct fpu *fpu; + + fpu = &tsk->thread.fpu; + fpregs_lock(); + + if (!test_thread_flag(TIF_NEED_FPU_LOAD)) { + copy_fpregs_to_fpstate(fpu); + set_thread_flag(TIF_NEED_FPU_LOAD); + } + + cet = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); + if (!cet) { + fpregs_unlock(); + goto sigsegv;
I *think* your patchset tries to keep cet.shstk_size and cet.ibt_enabled in sync with the MSR, in which case it should be impossible to get here, but a comment and a warning would be much better than a random sigsegv. Shouldn't we have a get_xsave_addr_or_allocate() that will never return NULL but instead will mark the state as in use and set up the init state if the feature was previously not in use?