From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0F949C43460 for ; Wed, 7 Apr 2021 19:36:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id CDB7461363 for ; Wed, 7 Apr 2021 19:36:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1355639AbhDGTgS (ORCPT ); Wed, 7 Apr 2021 15:36:18 -0400 Received: from mga09.intel.com ([134.134.136.24]:30011 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1345628AbhDGTgR (ORCPT ); Wed, 7 Apr 2021 15:36:17 -0400 IronPort-SDR: TdEZj9cmyPaIWVjf2/EKdRQ/MEP2ty4BQrWm5MmqCmz4urQoWL+LUS7UMnK6gpVtcNqSCTNszz yo9nMxPOyu5w== X-IronPort-AV: E=McAfee;i="6000,8403,9947"; a="193499952" X-IronPort-AV: E=Sophos;i="5.82,203,1613462400"; d="scan'208";a="193499952" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Apr 2021 12:36:07 -0700 IronPort-SDR: rAPVXak+nGDVnXg9E3NIe27rAWHjK35mnoyvbDaT6d99UCplYkjy2wqj9cOCpfHty8nh69qdk1 E3HjT8/jTPWA== X-IronPort-AV: E=Sophos;i="5.82,203,1613462400"; d="scan'208";a="448370025" Received: from yyu32-mobl1.amr.corp.intel.com (HELO [10.254.186.83]) ([10.254.186.83]) by fmsmga003-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Apr 2021 12:36:05 -0700 Subject: Re: [PATCH v24 25/30] x86/cet/shstk: Handle signals for shadow stack To: Andy Lutomirski Cc: X86 ML , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , LKML , "open list:DOCUMENTATION" , Linux-MM , linux-arch , Linux API , Arnd Bergmann , Balbir Singh , Borislav Petkov , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H.J. Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , "Ravi V. Shankar" , Vedvyas Shanbhogue , Dave Martin , Weijiang Yang , Pengfei Xu , Haitao Huang References: <20210401221104.31584-1-yu-cheng.yu@intel.com> <20210401221104.31584-26-yu-cheng.yu@intel.com> From: "Yu, Yu-cheng" Message-ID: <76743437-24b3-7c33-2570-6100c8811165@intel.com> Date: Wed, 7 Apr 2021 12:36:04 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-api@vger.kernel.org On 4/6/2021 3:50 PM, Andy Lutomirski wrote: > On Thu, Apr 1, 2021 at 3:11 PM Yu-cheng Yu 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 >> Cc: Kees Cook >> --- >> 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