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 721BEC43470 for ; Wed, 7 Apr 2021 19:36:11 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E809E610E6 for ; Wed, 7 Apr 2021 19:36:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E809E610E6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 60ED46B0073; Wed, 7 Apr 2021 15:36:10 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5BEC26B0078; Wed, 7 Apr 2021 15:36:10 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 438A36B007D; Wed, 7 Apr 2021 15:36:10 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0192.hostedemail.com [216.40.44.192]) by kanga.kvack.org (Postfix) with ESMTP id 298366B0073 for ; Wed, 7 Apr 2021 15:36:10 -0400 (EDT) Received: from smtpin25.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id DB2C9362E for ; Wed, 7 Apr 2021 19:36:09 +0000 (UTC) X-FDA: 78006576858.25.BC784BB Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by imf26.hostedemail.com (Postfix) with ESMTP id 8713740002DA for ; Wed, 7 Apr 2021 19:36:05 +0000 (UTC) IronPort-SDR: sZWCKtOMiSAGQPEPTzs4gghSfg0NWBHjU3J6IymC3T89fklTOQ3Ikt3FFathlfIc0ktRd2XiN9 Y6/MtKvR8Log== X-IronPort-AV: E=McAfee;i="6000,8403,9947"; a="173462830" X-IronPort-AV: E=Sophos;i="5.82,203,1613462400"; d="scan'208";a="173462830" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 Apr 2021 12:36:06 -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 X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 8713740002DA X-Stat-Signature: afbmtm37po14jfo3zjuwajdpkgib11yg Received-SPF: none (intel.com>: No applicable sender policy available) receiver=imf26; identity=mailfrom; envelope-from=""; helo=mga17.intel.com; client-ip=192.55.52.151 X-HE-DKIM-Result: none/none X-HE-Tag: 1617824165-347067 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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