Re: [PATCH v24 25/30] x86/cet/shstk: Handle signals for shadow stack

["Yu, Yu-cheng" <yu-cheng.yu@intel.com>]

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 = &current->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(&current->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