Messages in this thread | | | Subject | Re: [PATCH v24 25/30] x86/cet/shstk: Handle signals for shadow stack | From | "Yu, Yu-cheng" <> | Date | Wed, 7 Apr 2021 12:36:04 -0700 |
| |
On 4/6/2021 3:50 PM, Andy Lutomirski wrote: > On Thu, Apr 1, 2021 at 3:11 PM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote: >> >> When shadow stack is enabled, a task's shadow stack states must be saved >> along with the signal context and later restored in sigreturn. However, >> currently there is no systematic facility for extending a signal context. >> >> Introduce a signal context extension struct 'sc_ext', which is used to save >> shadow stack restore token address and WAIT_ENDBR status[1]. The extension >> is located above the fpu states, plus alignment. >> >> Introduce routines for the allocation, save, and restore for sc_ext: >> - fpu__alloc_sigcontext_ext(), >> - save_extra_state_to_sigframe(), >> - get_extra_state_from_sigframe(), >> - restore_extra_state(). >> >> [1] WAIT_ENDBR will be introduced later in the Indirect Branch Tracking >> series, but add that into sc_ext now to keep the struct stable in case >> the IBT series is applied later. > > Please don't. Instead, please figure out how that structure gets > extended for real, and organize your patches to demonstrate that the > extension works. > >> >> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com> >> Cc: Kees Cook <keescook@chromium.org> >> --- >> v24: >> - Split out shadow stack token routines to a separate patch. >> - Put signal frame save/restore routines to fpu/signal.c and re-name accordingly. >> >> arch/x86/ia32/ia32_signal.c | 16 +++ >> arch/x86/include/asm/cet.h | 2 + >> arch/x86/include/asm/fpu/internal.h | 2 + >> arch/x86/include/uapi/asm/sigcontext.h | 9 ++ >> arch/x86/kernel/fpu/signal.c | 143 +++++++++++++++++++++++++ >> arch/x86/kernel/signal.c | 9 ++ >> 6 files changed, 181 insertions(+) >>
[...]
>> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h >> index 844d60eb1882..cf2d55db3be4 100644 >> --- a/arch/x86/include/uapi/asm/sigcontext.h >> +++ b/arch/x86/include/uapi/asm/sigcontext.h >> @@ -196,6 +196,15 @@ struct _xstate { >> /* New processor state extensions go here: */ >> }; >> >> +/* >> + * Located at the end of sigcontext->fpstate, aligned to 8. >> + */ >> +struct sc_ext { >> + unsigned long total_size; >> + unsigned long ssp; >> + unsigned long wait_endbr; >> +}; > > We need some proper documentation and an extensibility story for this. > This won't be the last time we extend the signal state. Keep in mind > that the FPU state is very likely to become genuinely variable sized > due to AVX-512 and AMX. >
Right now, on the signal stack, we have:
- siginfo, ucontext, - fpu states (xsave state),
We might not want to change ucontext. The concern is breaking existing app's.
Fpu states are all user states (vs. ssp, wait_endbr are supervisor states). Therefore, we cannot put ssp and wait_endbr in fpu states. Fpu states can grow to whatever size (AVX-512 etc.), the extension is always above it if the user stack has room. If the user stack does not have enough room, fpu__aloc_mathframe() fails.
The struct sc_ext has a simple 'total_size' field for error checking. To extend it, newer fields are always added to the end and total_size keeps track of it. I will put more comments about this.
> We also have the ability to extend ucontext, I believe, and I'd like > some analysis of why we want to put ssp and wait_endbr into the FPU > context instead of the ucontext. >
[...]
>> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c >> index a4ec65317a7f..2e56f2fe8be0 100644 >> --- a/arch/x86/kernel/fpu/signal.c >> +++ b/arch/x86/kernel/fpu/signal.c
[...]
>> + >> +/* >> + * Called from __fpu__restore_sig() and XSAVES buffer is protected by >> + * set_thread_flag(TIF_NEED_FPU_LOAD) in the slow path. >> + */ >> +void restore_extra_state(struct sc_ext *sc_ext) >> +{ >> +#ifdef CONFIG_X86_CET >> + struct cet_status *cet = ¤t->thread.cet; >> + struct cet_user_state *cet_user_state; >> + u64 msr_val = 0; >> + >> + if (!cpu_feature_enabled(X86_FEATURE_CET)) >> + return; >> + >> + cet_user_state = get_xsave_addr(¤t->thread.fpu.state.xsave, >> + XFEATURE_CET_USER); >> + if (!cet_user_state) >> + return; >> + >> + if (cet->shstk_size) { > > Is fpregs_lock() needed?
This path is already protected.
> >> + if (test_thread_flag(TIF_NEED_FPU_LOAD)) >> + cet_user_state->user_ssp = sc_ext->ssp; >> + else >> + wrmsrl(MSR_IA32_PL3_SSP, sc_ext->ssp); > > wrmsrl_safe() please. > >> + >> + msr_val |= CET_SHSTK_EN; >> + } >> + >> + if (test_thread_flag(TIF_NEED_FPU_LOAD)) >> + cet_user_state->user_cet = msr_val; >> + else >> + wrmsrl(MSR_IA32_U_CET, msr_val); >> +#endif > > I don't understand. Why are you recomputing MSR_IA32_U_CET here? > > As another general complaint about this patch set, there's > cet->shstk_size and there's MSR_IA32_U_CET (and its copy in the fpu > state), and they seem to be used somewhat interchangably. Why are > both needed? Could there be some new helpers to help manage them all > in a unified way? >
Indeed, shadow stack/IBT states are cached in the thread header. Their MSRs and XSAVES states are accessed only when necessary. The signal restore path has been optimized in the past and I hope not to put in code that negates past work.
I agree with your other comments for the patch and will update in the next revision.
Thanks, Yu-cheng
| |