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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <76743437-24b3-7c33-2570-6100c8811165@intel.com>
Date:   Wed, 7 Apr 2021 12:36:04 -0700
From:   "Yu, Yu-cheng" <yu-cheng.yu@...el.com>
To:     Andy Lutomirski <luto@...nel.org>
Cc:     X86 ML <x86@...nel.org>, "H. Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        linux-arch <linux-arch@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Balbir Singh <bsingharora@...il.com>,
        Borislav Petkov <bp@...en8.de>,
        Cyrill Gorcunov <gorcunov@...il.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Eugene Syromiatnikov <esyr@...hat.com>,
        Florian Weimer <fweimer@...hat.com>,
        "H.J. Lu" <hjl.tools@...il.com>, Jann Horn <jannh@...gle.com>,
        Jonathan Corbet <corbet@....net>,
        Kees Cook <keescook@...omium.org>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Nadav Amit <nadav.amit@...il.com>,
        Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
        Peter Zijlstra <peterz@...radead.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Vedvyas Shanbhogue <vedvyas.shanbhogue@...el.com>,
        Dave Martin <Dave.Martin@....com>,
        Weijiang Yang <weijiang.yang@...el.com>,
        Pengfei Xu <pengfei.xu@...el.com>,
        Haitao Huang <haitao.huang@...el.com>
Subject: Re: [PATCH v24 25/30] x86/cet/shstk: Handle signals for shadow stack

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@...el.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@...el.com>
>> Cc: Kees Cook <keescook@...omium.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