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=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,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 5B0F2C433B4 for ; Mon, 17 May 2021 20:55:23 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 0015661244 for ; Mon, 17 May 2021 20:55:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0015661244 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 454D66B0081; Mon, 17 May 2021 16:55:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 42D446B0083; Mon, 17 May 2021 16:55:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2F63E6B0087; Mon, 17 May 2021 16:55:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0183.hostedemail.com [216.40.44.183]) by kanga.kvack.org (Postfix) with ESMTP id EC3DD6B0081 for ; Mon, 17 May 2021 16:55:21 -0400 (EDT) Received: from smtpin30.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 976D2B9FF for ; Mon, 17 May 2021 20:55:21 +0000 (UTC) X-FDA: 78151928442.30.B017CDD Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by imf23.hostedemail.com (Postfix) with ESMTP id 73861A00039E for ; Mon, 17 May 2021 20:55:17 +0000 (UTC) IronPort-SDR: rNQg/YS0gnnK6o2jkufq3tXHqu74wArprdT8RgVofCGpFwUk7njnFBn0Q1x9iyRTeCvwYngUmv Yce6O0yoZ0fQ== X-IronPort-AV: E=McAfee;i="6200,9189,9987"; a="197477638" X-IronPort-AV: E=Sophos;i="5.82,307,1613462400"; d="scan'208";a="197477638" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2021 13:55:14 -0700 IronPort-SDR: TZcXoTiIbYniVOmf1LN1Ev8wIo3ZErYhJYjAMAmVqWeQUZ6eIC8Vwm+cxtyTHYjB5Jkzk7NDh5 UipVJ/gBm2sA== X-IronPort-AV: E=Sophos;i="5.82,307,1613462400"; d="scan'208";a="541464404" Received: from yyu32-mobl1.amr.corp.intel.com (HELO [10.251.147.139]) ([10.251.147.139]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2021 13:55:13 -0700 Subject: Re: [PATCH v26 24/30] x86/cet/shstk: Introduce shadow stack token setup/verify routines To: Borislav Petkov Cc: x86@kernel.org, "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org, Arnd Bergmann , Andy Lutomirski , Balbir Singh , 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: <20210427204315.24153-1-yu-cheng.yu@intel.com> <20210427204315.24153-25-yu-cheng.yu@intel.com> From: "Yu, Yu-cheng" Message-ID: Date: Mon, 17 May 2021 13:55:01 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Authentication-Results: imf23.hostedemail.com; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=intel.com (policy=none); spf=none (imf23.hostedemail.com: domain of yu-cheng.yu@intel.com has no SPF policy when checking 192.55.52.93) smtp.mailfrom=yu-cheng.yu@intel.com X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 73861A00039E X-Stat-Signature: isfmug9spn9iw961a7tz8cuafd3cniiz X-HE-Tag: 1621284917-952718 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 5/17/2021 12:45 AM, Borislav Petkov wrote: > On Tue, Apr 27, 2021 at 01:43:09PM -0700, Yu-cheng Yu wrote: >> +static inline int write_user_shstk_32(u32 __user *addr, u32 val) >> +{ >> + WARN_ONCE(1, "%s used but not supported.\n", __func__); >> + return -EFAULT; >> +} >> +#endif > > What is that supposed to catch? Any concrete (mis-)use cases? > If 32-bit apps are not supported, there should be no need of 32-bit shadow stack write, otherwise there is a bug. [...] >> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c >> index d387df84b7f1..48a0c87414ef 100644 >> --- a/arch/x86/kernel/shstk.c >> +++ b/arch/x86/kernel/shstk.c >> @@ -20,6 +20,7 @@ >> #include >> #include >> #include >> +#include >> >> static void start_update_msrs(void) >> { >> @@ -176,3 +177,128 @@ void shstk_disable(void) >> >> shstk_free(current); >> } >> + >> +static unsigned long _get_user_shstk_addr(void) > > What's the "_" prefix in the name supposed to denote? > > Ditto for the other functions with "_" prefix you're adding. > These are static functions. I thought that would make the static scope clear. I can remove "_". >> +{ >> + struct fpu *fpu = ¤t->thread.fpu; >> + unsigned long ssp = 0; >> + >> + fpregs_lock(); >> + >> + if (fpregs_state_valid(fpu, smp_processor_id())) { >> + rdmsrl(MSR_IA32_PL3_SSP, ssp); >> + } else { >> + struct cet_user_state *p; >> + >> + p = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER); >> + if (p) >> + ssp = p->user_ssp; >> + } >> + >> + fpregs_unlock(); > > <---- newline here. > >> + return ssp; >> +} >> + >> +#define TOKEN_MODE_MASK 3UL >> +#define TOKEN_MODE_64 1UL >> +#define IS_TOKEN_64(token) (((token) & TOKEN_MODE_MASK) == TOKEN_MODE_64) >> +#define IS_TOKEN_32(token) (((token) & TOKEN_MODE_MASK) == 0) > > Why do you have to look at the second, busy bit, too in order to > determine the mode? > If the busy bit is set, it is only for SAVEPREVSSP, and invalid as a normal restore token. > Also, you don't need most of those defines - see below. > >> +/* >> + * Create a restore token on the shadow stack. A token is always 8-byte >> + * and aligned to 8. >> + */ >> +static int _create_rstor_token(bool ia32, unsigned long ssp, >> + unsigned long *token_addr) >> +{ >> + unsigned long addr; >> + >> + *token_addr = 0; > > What for? Callers should check this function's retval and then interpret > the validity of token_addr and it should not unconditionally write into > it. > Ok. >> + >> + if ((!ia32 && !IS_ALIGNED(ssp, 8)) || !IS_ALIGNED(ssp, 4)) > > Flip this logic: > > if ((ia32 && !IS_ALIGNED(ssp, 4)) || !IS_ALIGNED(ssp, 8)) > >> + return -EINVAL; >> + >> + addr = ALIGN_DOWN(ssp, 8) - 8; > > Yah, so this is weird. Why does the restore token need to be at -8 > instead on the shadow stack address itself? With the lower two bits masked out, the restore token must point directly above itself. > > Looking at > > Figure 18-2. RSTORSSP to Switch to New Shadow Stack > Figure 18-3. SAVEPREVSSP to Save a Restore Point > > in the SDM, it looks like unnecessarily more complex than it should be. > But maybe there's some magic I'm missing. > >> + >> + /* Is the token for 64-bit? */ >> + if (!ia32) >> + ssp |= TOKEN_MODE_64; > > |= BIT(0); > Ok, then, we don't use #define's. I will put in comments about what it is doing, and fix the rest. Thanks, Yu-cheng