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

[Yu, Yu-cheng]

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@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> > 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