Message ID | 20230220182717.uzrym2gtavlbjbxo@treble |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp1456278wrn; Mon, 20 Feb 2023 10:40:30 -0800 (PST) X-Google-Smtp-Source: AK7set9JNbstXCP4d+4FxavUhEGovC9mTR/1wjxoYxWKgyPn0f9wZWWfdAMsjXVsTJqI5FOOheBd X-Received: by 2002:a17:902:ecc7:b0:19a:9642:dee8 with SMTP id a7-20020a170902ecc700b0019a9642dee8mr2123573plh.0.1676918429988; Mon, 20 Feb 2023 10:40:29 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1676918429; cv=none; d=google.com; s=arc-20160816; b=O9CzNzBGIKKeo3FrkoKG4mA8C9Sup1FRfUoNGBSTuBLAeV4KEIfxmTqx8FnOt9ClrX yRhECq/QAQRUGnroSMs5hAZt3LuGRs9jY62RPueuBiSgB1lhMwbpgk/56dNmzsAYWVIU OTwPmDna2i/VN0b9SBPEXap5ck3wikyMgLUPAh9aUzI1Sh/bj4by6GkuaeonrfHpxEUn B8OsIG6UfRZRBcorg+StWj18kAKwUb55eklFYjIxYg/G+YLQ4tJPa4NUBny9r8nv8jc/ T6CGoDAtxqx2Ue4GkaQAelXuOj9s/NN036dxY+v/JNNpAzDMh4OtPxnp/wfWhOn9W1oq 9ddA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=QwK69fMYvGcpB+jT+B7o4v+2+4oWZyRlEOMEBr6OXS4=; b=04yuxLbGoCZXTyq8ALFA9uRM0BhAOpNmx4SkoER2WT/leaTFbITynJzhCad5OZ2QbG 544GZob48DkxbW7HY19FHJerDiXachdoJWj8fW6rkiq5qTx1JJhxqWlLKQkbAqcq5poO LG5Ekol7AksPWuyq8RTC0ubYjAFgivi4BufzCC2U0ntQ0FqzW+ydnQWqKjXHk2YyGHGQ flbH5uNtVljUx4oYgdHKlYBxp8v0KEEpOqRR6cNKEQvWKu0DkjwEaJWn4HFUSeasTEco VD1OmGBq/ea2j1vzZQqgTlGwMUsxUV8ptsNrbH9iBLHyJ1x2rGeKIIfJ1ZWPGihQuLTk cUug== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="uqE9+1R/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id n17-20020a170902d2d100b00198d701adb4si3329949plc.615.2023.02.20.10.40.16; Mon, 20 Feb 2023 10:40:29 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b="uqE9+1R/"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231814AbjBTS1X (ORCPT <rfc822;kautuk.consul.80@gmail.com> + 99 others); Mon, 20 Feb 2023 13:27:23 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41236 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229885AbjBTS1W (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 20 Feb 2023 13:27:22 -0500 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 343A71C31A; Mon, 20 Feb 2023 10:27:21 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id A781860F00; Mon, 20 Feb 2023 18:27:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5FFABC4339B; Mon, 20 Feb 2023 18:27:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1676917640; bh=T7ZxLDOEHL5a48unqqdISt58VJOeJ4E3/WLn0nVRKCc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=uqE9+1R/bBNbWjA+oKlDucXB7DEGP9lhZkLY/MFrrjkLOemujnpYEywdzT2fT3Vxs bsbanDWV6wca2uXqpo5RtprA1GqJW91apcJeFNcL/5K6srwBSa1MxMuYXA02Y+dq79 mZxBvzDa9WbpU0AXUqn0TmmsbpXJA3SJhUAcyay5T7Zf9cAP6HfBTyQ+hzXhYpssgx exT1bItyrpzsWmCuZjz8luhWCeNiBFc/6/UwBIoctZ+P5fS32pz0VkHkegg6mOZW5/ JgA9M+MXtfFRdiM/uX0+QPLKY8+rt3qSR5witakoCgmE5YSyONhR78CcOunQnRMuzZ aC0vt5DD6alBw== Date: Mon, 20 Feb 2023 10:27:17 -0800 From: Josh Poimboeuf <jpoimboe@kernel.org> To: KP Singh <kpsingh@kernel.org> Cc: Borislav Petkov <bp@alien8.de>, linux-kernel@vger.kernel.org, pjt@google.com, evn@google.com, tglx@linutronix.de, mingo@redhat.com, dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, peterz@infradead.org, pawan.kumar.gupta@linux.intel.com, kim.phillips@amd.com, alexandre.chartre@oracle.com, daniel.sneddon@linux.intel.com, =?utf-8?b?Sm9zw6k=?= Oliveira <joseloliveira11@gmail.com>, Rodrigo Branco <rodrigo@kernelhacking.com>, Alexandra Sandulescu <aesa@google.com>, Jim Mattson <jmattson@google.com>, stable@vger.kernel.org Subject: [PATCH] x86/bugs: Allow STIBP with IBRS Message-ID: <20230220182717.uzrym2gtavlbjbxo@treble> References: <20230220120127.1975241-1-kpsingh@kernel.org> <20230220121350.aidsipw3kd4rsyss@treble> <CACYkzJ5L9MLuE5Jz+z5-NJCCrUqTbgKQkXSqnQnCfTD_WNA7_Q@mail.gmail.com> <CACYkzJ6n=-tobhX0ONQhjHSgmnNjWnNe_dZnEOGtD8Y6S3RHbA@mail.gmail.com> <20230220163442.7fmaeef3oqci4ee3@treble> <Y/Ox3MJZF1Yb7b6y@zn.tnic> <20230220175929.2laflfb2met6y3kc@treble> <CACYkzJ71xqzY6-wL+YShcL+d6ugzcdFHr6tbYWWE_ep52+RBZQ@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <CACYkzJ71xqzY6-wL+YShcL+d6ugzcdFHr6tbYWWE_ep52+RBZQ@mail.gmail.com> X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1758376419465202075?= X-GMAIL-MSGID: =?utf-8?q?1758376419465202075?= |
Series |
x86/bugs: Allow STIBP with IBRS
|
|
Commit Message
Josh Poimboeuf
Feb. 20, 2023, 6:27 p.m. UTC
IBRS is only enabled in kernel space. Since it's not enabled in user
space, user space isn't protected from indirect branch prediction
attacks from a sibling CPU thread.
Allow STIBP to be enabled to protect against such attacks.
Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS")
Reported-by: José Oliveira <joseloliveira11@gmail.com>
Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
---
arch/x86/kernel/cpu/bugs.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
Comments
On Mon, Feb 20, 2023 at 10:27 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > IBRS is only enabled in kernel space. Since it's not enabled in user > space, user space isn't protected from indirect branch prediction > attacks from a sibling CPU thread. > > Allow STIBP to be enabled to protect against such attacks. > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS") > Reported-by: José Oliveira <joseloliveira11@gmail.com> > Reported-by: Rodrigo Branco <rodrigo@kernelhacking.com> > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > arch/x86/kernel/cpu/bugs.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index 85168740f76a..b97c0d28e573 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -1124,14 +1124,19 @@ spectre_v2_parse_user_cmdline(void) > return SPECTRE_V2_USER_CMD_AUTO; > } > > -static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) > +static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) > { > - return mode == SPECTRE_V2_IBRS || > - mode == SPECTRE_V2_EIBRS || > + return mode == SPECTRE_V2_EIBRS || > mode == SPECTRE_V2_EIBRS_RETPOLINE || > mode == SPECTRE_V2_EIBRS_LFENCE; > } There are no comments here, this code is in dire need for some comments and explanation, I was trying something like: diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index bca0bd8f4846..3e04f9fa68a8 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1124,14 +1124,31 @@ spectre_v2_parse_user_cmdline(void) return SPECTRE_V2_USER_CMD_AUTO; } -static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) +static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { - return mode == SPECTRE_V2_IBRS || - mode == SPECTRE_V2_EIBRS || + return mode == SPECTRE_V2_EIBRS || mode == SPECTRE_V2_EIBRS_RETPOLINE || mode == SPECTRE_V2_EIBRS_LFENCE; } +/* + * In IBRS mode, the spectre_v2 mitigation is enabled only in kernel space with + * the IBRS bit being cleared on return to userspace due to performance + * overhead. + */ +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) +{ + return spectre_v2_in_eibrs_mode(mode) || mode == SPECTRE_V2_IBRS; +} + +/* + * User mode protections are only available in eIBRS mode. + */ +static inline bool spectre_v2_user_needs_stibp(enum spectre_v2_mitigation mode) +{ + return !spectre_v2_in_eibrs_mode(mode); +} + static void __init spectre_v2_user_select_mitigation(void) { @@ -1193,13 +1210,8 @@ spectre_v2_user_select_mitigation(void) "always-on" : "conditional"); } - /* - * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible, - * STIBP is not required. - */ - if (!boot_cpu_has(X86_FEATURE_STIBP) || - !smt_possible || - spectre_v2_in_ibrs_mode(spectre_v2_enabled)) + if (!boot_cpu_has(X86_FEATURE_STIBP) || !smt_possible || + !spectre_v2_user_needs_stibp(spectre_v2_enabled)) return; /* @@ -2327,7 +2339,7 @@ static ssize_t mmio_stale_data_show_state(char *buf) static char *stibp_state(void) { - if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) + if (!spectre_v2_user_needs_stibp(spectre_v2_enabled)) return ""; switch (spectre_v2_user_stibp) { Also Josh, is it okay for us to have a discussion and have me write the patch as a v2? Your current patch does not even credit me at all. Seems a bit unfair, but I don't really care. I was going to rev up the patch with your suggestions. > > +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) > +{ > + return spectre_v2_in_eibrs_mode(mode) || > + mode == SPECTRE_V2_IBRS; > +} > + > static void __init > spectre_v2_user_select_mitigation(void) > { > @@ -1194,12 +1199,12 @@ spectre_v2_user_select_mitigation(void) > } > > /* > - * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible, > + * If no STIBP, enhanced IBRS is enabled, or SMT impossible, > * STIBP is not required. > */ > if (!boot_cpu_has(X86_FEATURE_STIBP) || > !smt_possible || > - spectre_v2_in_ibrs_mode(spectre_v2_enabled)) > + spectre_v2_in_eibrs_mode(spectre_v2_enabled)) > return; > > /* > @@ -2327,9 +2332,6 @@ static ssize_t mmio_stale_data_show_state(char *buf) > > static char *stibp_state(void) > { > - if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) > - return ""; > - > switch (spectre_v2_user_stibp) { > case SPECTRE_V2_USER_NONE: > return ", STIBP: disabled"; > -- > 2.39.1 >
Drop stable@ again. On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote: > IBRS is only enabled in kernel space. Since it's not enabled in user > space, user space isn't protected from indirect branch prediction > attacks from a sibling CPU thread. > > Allow STIBP to be enabled to protect against such attacks. > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS") Yah, look at that one: commit 7c693f54c873691a4b7da05c7e0f74e67745d144 Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> Date: Tue Jun 14 23:15:55 2022 +0200 x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS Extend spectre_v2= boot option with Kernel IBRS. [jpoimboe: no STIBP with IBRS] ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ I'm assuming this was supposed to mean no STIBP in *kernel mode* when IBRS is selected? In user mode, STIBP should be selectable as we disable IBRS there. Close? If so, pls document it too while at it: Documentation/admin-guide/hw-vuln/spectre.rst because we will be wondering next time again. Like we wonder each time this madness is being touched. ;-( Thx.
On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote: > static char *stibp_state(void) > { > - if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) > + if (!spectre_v2_user_needs_stibp(spectre_v2_enabled)) > return ""; > > switch (spectre_v2_user_stibp) { > > Also Josh, is it okay for us to have a discussion and have me write > the patch as a v2? Your current patch does not even credit me at all. > Seems a bit unfair, but I don't really care. I was going to rev up the > patch with your suggestions. Well, frankly the patch needed a complete rewrite. The patch description was unclear about what the problem is and what's being fixed. The code was obtuse and the comments didn't help. I could tell by the other replies that I wasn't the only one confused. I can give you Reported-by, or did you have some other tag in mind?
On Mon, Feb 20, 2023 at 11:00 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote: > > static char *stibp_state(void) > > { > > - if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) > > + if (!spectre_v2_user_needs_stibp(spectre_v2_enabled)) > > return ""; > > > > switch (spectre_v2_user_stibp) { > > > > Also Josh, is it okay for us to have a discussion and have me write > > the patch as a v2? Your current patch does not even credit me at all. > > Seems a bit unfair, but I don't really care. I was going to rev up the > > patch with your suggestions. > > Well, frankly the patch needed a complete rewrite. The patch > description was unclear about what the problem is and what's being Josh, this is a complex issue, we are figuring it out together on the list. It's complex, that's why folks got it wrong in the first place. Calling the patch obtuse and unclear is unfair! > fixed. The code was obtuse and the comments didn't help. I could tell > by the other replies that I wasn't the only one confused. The patch you sent is not clear either, it implicitly ties in STIBP with eIBRS. There is no explanation anywhere that IBRS just means KERNEL_IBRS. > > I can give you Reported-by, or did you have some other tag in mind? > > -- > Josh
On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote: > Drop stable@ again. > > On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote: > > IBRS is only enabled in kernel space. Since it's not enabled in user > > space, user space isn't protected from indirect branch prediction > > attacks from a sibling CPU thread. > > > > Allow STIBP to be enabled to protect against such attacks. > > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS") > > Yah, look at that one: > > commit 7c693f54c873691a4b7da05c7e0f74e67745d144 > Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > Date: Tue Jun 14 23:15:55 2022 +0200 > > x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS > > Extend spectre_v2= boot option with Kernel IBRS. > > [jpoimboe: no STIBP with IBRS] > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > I'm assuming this was supposed to mean no STIBP in *kernel mode* when > IBRS is selected? No it was supposed to be "no STIBP with *eIBRS*". > In user mode, STIBP should be selectable as we disable IBRS there. > > Close? > > If so, pls document it too while at it: > > Documentation/admin-guide/hw-vuln/spectre.rst > > because we will be wondering next time again. > > Like we wonder each time this madness is being touched. ;-( As far as I can tell, that document was never updated to describe spectre_v2=ibrs in the first place. That would be a whole 'nother patch which I'm not volunteering for. Nice try ;-)
On Mon, Feb 20, 2023 at 11:09 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote: > > Drop stable@ again. > > > > On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote: > > > IBRS is only enabled in kernel space. Since it's not enabled in user > > > space, user space isn't protected from indirect branch prediction > > > attacks from a sibling CPU thread. > > > > > > Allow STIBP to be enabled to protect against such attacks. > > > > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS") > > > > Yah, look at that one: > > > > commit 7c693f54c873691a4b7da05c7e0f74e67745d144 > > Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > > Date: Tue Jun 14 23:15:55 2022 +0200 > > > > x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS > > > > Extend spectre_v2= boot option with Kernel IBRS. > > > > [jpoimboe: no STIBP with IBRS] > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > I'm assuming this was supposed to mean no STIBP in *kernel mode* when > > IBRS is selected? > > No it was supposed to be "no STIBP with *eIBRS*". > > > In user mode, STIBP should be selectable as we disable IBRS there. > > > > Close? > > > > If so, pls document it too while at it: > > > > Documentation/admin-guide/hw-vuln/spectre.rst > > > > because we will be wondering next time again. > > > > Like we wonder each time this madness is being touched. ;-( > > As far as I can tell, that document was never updated to describe > spectre_v2=ibrs in the first place. That would be a whole 'nother patch > which I'm not volunteering for. Nice try ;-) This should at least be documented in the code. Now it seems like it is not okay to work with people on the list and just send revisions bypassing them. This is not something we do in the kernel area I come from (an x86 favorite ;)). Please feel free to go with Josh's version (or its future revisions). If you want me to re-spin with some comments, happy to. If not, please do at least give me Reported-by here. > > -- > Josh
On Mon, Feb 20, 2023 at 11:04:56AM -0800, KP Singh wrote: > On Mon, Feb 20, 2023 at 11:00 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > > On Mon, Feb 20, 2023 at 10:33:56AM -0800, KP Singh wrote: > > > static char *stibp_state(void) > > > { > > > - if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) > > > + if (!spectre_v2_user_needs_stibp(spectre_v2_enabled)) > > > return ""; > > > > > > switch (spectre_v2_user_stibp) { > > > > > > Also Josh, is it okay for us to have a discussion and have me write > > > the patch as a v2? Your current patch does not even credit me at all. > > > Seems a bit unfair, but I don't really care. I was going to rev up the > > > patch with your suggestions. > > > > Well, frankly the patch needed a complete rewrite. The patch > > description was unclear about what the problem is and what's being > > Josh, this is a complex issue, we are figuring it out together on the > list. It's complex, that's why folks got it wrong in the first place. > Calling the patch obtuse and unclear is unfair! > > > fixed. The code was obtuse and the comments didn't help. I could tell > > by the other replies that I wasn't the only one confused. > > The patch you sent is not clear either, it implicitly ties in STIBP > with eIBRS. There is no explanation anywhere that IBRS just means > KERNEL_IBRS. Ok, so something like this on top? diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index b97c0d28e573..fb3079445700 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1201,6 +1201,10 @@ spectre_v2_user_select_mitigation(void) /* * If no STIBP, enhanced IBRS is enabled, or SMT impossible, * STIBP is not required. + * + * For legacy IBRS, STIBP may still be needed because IBRS is only + * enabled in kernel space, so user space isn't protected from indirect + * branch prediction attacks from a sibling CPU thread. */ if (!boot_cpu_has(X86_FEATURE_STIBP) || !smt_possible ||
On Mon, Feb 20, 2023 at 11:09:08AM -0800, Josh Poimboeuf wrote: > No it was supposed to be "no STIBP with *eIBRS*". Ok, good. > As far as I can tell, that document was never updated to describe > spectre_v2=ibrs in the first place. That would be a whole 'nother patch > which I'm not volunteering for. Nice try ;-) What do you think the maintainers are for? To clean up after people... ;-\ As a matter of fact, updating the documentation is already on my ever-growing TODO list. But for this patch I'd like to ask whoever of you does it, to add a one or two sentences about why we need to allow STIBP selection with IBRS. The rest of the documentation we'll flesh out in time. Thx.
On Mon, Feb 20, 2023 at 11:16:45AM -0800, KP Singh wrote: > > As far as I can tell, that document was never updated to describe > > spectre_v2=ibrs in the first place. That would be a whole 'nother patch > > which I'm not volunteering for. Nice try ;-) > > This should at least be documented in the code. > > Now it seems like it is not okay to work with people on the list and > just send revisions bypassing them. This is not something we do in the > kernel area I come from (an x86 favorite ;)). Please feel free to go > with Josh's version (or its future revisions). If you want me to > re-spin with some comments, happy to. If not, please do at least give > me Reported-by here. It's a common practice even outside of x86. I'd recommend not taking it personally. In the end it's all about what produces better code. And please don't take this the wrong way, but sometimes it takes a bad patch to inspire a better one. I mean that with no disrespect, I myself have sent many bad patches. And if you don't like my patch, fine, send a v2...
On Mon, Feb 20, 2023 at 11:35 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > On Mon, Feb 20, 2023 at 11:16:45AM -0800, KP Singh wrote: > > > As far as I can tell, that document was never updated to describe > > > spectre_v2=ibrs in the first place. That would be a whole 'nother patch > > > which I'm not volunteering for. Nice try ;-) > > > > This should at least be documented in the code. > > > > Now it seems like it is not okay to work with people on the list and > > just send revisions bypassing them. This is not something we do in the > > kernel area I come from (an x86 favorite ;)). Please feel free to go > > with Josh's version (or its future revisions). If you want me to > > re-spin with some comments, happy to. If not, please do at least give > > me Reported-by here. > > It's a common practice even outside of x86. I'd recommend not taking it > personally. In the end it's all about what produces better code. > > And please don't take this the wrong way, but sometimes it takes a bad > patch to inspire a better one. I mean that with no disrespect, I myself > have sent many bad patches. Appreciate the clarification! Thank you so much. > > And if you don't like my patch, fine, send a v2... I can also take a stab at the Documentation piece, this will make Boris happy too. Will send a v2 later today. > > -- > Josh
On Mon, Feb 20, 2023 at 11:09:08AM -0800, Josh Poimboeuf wrote: > On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote: > > Drop stable@ again. > > > > On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote: > > > IBRS is only enabled in kernel space. Since it's not enabled in user > > > space, user space isn't protected from indirect branch prediction > > > attacks from a sibling CPU thread. > > > > > > Allow STIBP to be enabled to protect against such attacks. > > > > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS") > > > > Yah, look at that one: > > > > commit 7c693f54c873691a4b7da05c7e0f74e67745d144 > > Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > > Date: Tue Jun 14 23:15:55 2022 +0200 > > > > x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS > > > > Extend spectre_v2= boot option with Kernel IBRS. > > > > [jpoimboe: no STIBP with IBRS] > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > I'm assuming this was supposed to mean no STIBP in *kernel mode* when > > IBRS is selected? > > No it was supposed to be "no STIBP with *eIBRS*". Maybe not, "no STIBP with eIBRS" was the state before the said patch. In an offlist discussion during Retbleed embargo(copied below), it appears to mean "no STIBP *in kernel* with IBRS". But anyways, we missed to consider userspace. (BTW replying late because yesterday was a holiday in my geo). --- > > Subject: [PATCH v5 26/30] x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS > > > > From: Peter Zijlstra <peterz@infradead.org> > > > > From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > > > > The "spectre_v2=" boot option is extended to enable Kernel IBRS. > > > > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > --- > > Documentation/admin-guide/kernel-parameters.txt | 1 > > arch/x86/include/asm/nospec-branch.h | 1 > > arch/x86/kernel/cpu/bugs.c | 29 ++++++++++++++++++++++-- > > 3 files changed, 29 insertions(+), 2 deletions(-) > > > > @@ -1163,6 +1182,10 @@ static void __init spectre_v2_select_mit > > case SPECTRE_V2_EIBRS: > > break; > > > > + case SPECTRE_V2_IBRS: > > + setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS); > > + break; > > Don't we also need to set SPEC_CTRL_IBRS in x86_spec_ctrl_base? Also, STIBP isn't needed with IBRS. Suggested changes: diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 344ab7c9a4e2..498cb36587a3 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -897,11 +897,13 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd) return SPECTRE_V2_USER_CMD_AUTO; } -static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) { - return (mode == SPECTRE_V2_EIBRS || - mode == SPECTRE_V2_EIBRS_RETPOLINE || - mode == SPECTRE_V2_EIBRS_LFENCE); + + return spectre_v2_mode == SPECTRE_V2_IBRS + spectre_v2_mode == SPECTRE_V2_EIBRS || + spectre_v2_mode == SPECTRE_V2_EIBRS_RETPOLINE || + spectre_v2_mode == SPECTRE_V2_EIBRS_LFENCE; } static void __init @@ -966,12 +968,12 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd) } /* - * If no STIBP, enhanced IBRS is enabled or SMT impossible, STIBP is not - * required. + * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible, + * STIBP is not required. */ if (!boot_cpu_has(X86_FEATURE_STIBP) || !smt_possible || - spectre_v2_in_eibrs_mode(spectre_v2_enabled)) + spectre_v2_in_ibrs_mode(spectre_v2_enabled)) return; /* @@ -1171,7 +1173,7 @@ static void __init spectre_v2_select_mitigation(void) if (mode == SPECTRE_V2_EIBRS && unprivileged_ebpf_enabled()) pr_err(SPECTRE_V2_EIBRS_EBPF_MSG); - if (spectre_v2_in_eibrs_mode(mode)) { + if (spectre_v2_in_ibrs_mode(mode)) { /* Force it so VMEXIT will restore correctly */ x86_spec_ctrl_base |= SPEC_CTRL_IBRS; wr_spec_ctrl(x86_spec_ctrl_base, true); @@ -1212,19 +1214,17 @@ static void __init spectre_v2_select_mitigation(void) pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n"); /* - * Retpoline means the kernel is safe because it has no indirect - * branches. Enhanced IBRS protects firmware too, so, enable restricted - * speculation around firmware calls only when Enhanced IBRS isn't - * supported or kernel IBRS isn't enabled. + * Retpoline protects the kernel, but doesn't protect firmware. IBRS + * and Enhanced IBRS protect firmware too, so enable IBRS around + * firmware calls only when IBRS / Enhanced IBRS aren't otherwise + * enabled. * * Use "mode" to check Enhanced IBRS instead of boot_cpu_has(), because * the user might select retpoline on the kernel command line and if * the CPU supports Enhanced IBRS, kernel might un-intentionally not * enable IBRS around firmware calls. */ - if (boot_cpu_has(X86_FEATURE_IBRS) && - !boot_cpu_has(X86_FEATURE_KERNEL_IBRS) && - !spectre_v2_in_eibrs_mode(mode)) { + if (boot_cpu_has(X86_FEATURE_IBRS) && !spectre_v2_in_ibrs_mode(mode)) { setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW); pr_info("Enabling Restricted Speculation for firmware calls\n"); } @@ -1937,7 +1937,7 @@ static ssize_t tsx_async_abort_show_state(char *buf) static char *stibp_state(void) { - if (spectre_v2_in_eibrs_mode(spectre_v2_enabled)) + if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) return ""; switch (spectre_v2_user_stibp) {
On Tue, Feb 21, 2023 at 5:20 PM Pawan Gupta <pawan.kumar.gupta@linux.intel.com> wrote: > > On Mon, Feb 20, 2023 at 11:09:08AM -0800, Josh Poimboeuf wrote: > > On Mon, Feb 20, 2023 at 07:34:59PM +0100, Borislav Petkov wrote: > > > Drop stable@ again. > > > > > > On Mon, Feb 20, 2023 at 10:27:17AM -0800, Josh Poimboeuf wrote: > > > > IBRS is only enabled in kernel space. Since it's not enabled in user > > > > space, user space isn't protected from indirect branch prediction > > > > attacks from a sibling CPU thread. > > > > > > > > Allow STIBP to be enabled to protect against such attacks. > > > > > > > > Fixes: 7c693f54c873 ("x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS") > > > > > > Yah, look at that one: > > > > > > commit 7c693f54c873691a4b7da05c7e0f74e67745d144 > > > Author: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > > > Date: Tue Jun 14 23:15:55 2022 +0200 > > > > > > x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS > > > > > > Extend spectre_v2= boot option with Kernel IBRS. > > > > > > [jpoimboe: no STIBP with IBRS] > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > I'm assuming this was supposed to mean no STIBP in *kernel mode* when > > > IBRS is selected? > > > > No it was supposed to be "no STIBP with *eIBRS*". > > Maybe not, "no STIBP with eIBRS" was the state before the said patch. > > In an offlist discussion during Retbleed embargo(copied below), it > appears to mean "no STIBP *in kernel* with IBRS". But anyways, we missed > to consider userspace. > > (BTW replying late because yesterday was a holiday in my geo). > > --- > > > Subject: [PATCH v5 26/30] x86/speculation: Add spectre_v2=ibrs option to support Kernel IBRS > > > > > > From: Peter Zijlstra <peterz@infradead.org> > > > > > > From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > > > > > > The "spectre_v2=" boot option is extended to enable Kernel IBRS. > > > > > > Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com> > > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > > --- > > > Documentation/admin-guide/kernel-parameters.txt | 1 > > > arch/x86/include/asm/nospec-branch.h | 1 > > > arch/x86/kernel/cpu/bugs.c | 29 ++++++++++++++++++++++-- > > > 3 files changed, 29 insertions(+), 2 deletions(-) > > > > > > @@ -1163,6 +1182,10 @@ static void __init spectre_v2_select_mit > > > case SPECTRE_V2_EIBRS: > > > break; > > > > > > + case SPECTRE_V2_IBRS: > > > + setup_force_cpu_cap(X86_FEATURE_KERNEL_IBRS); > > > + break; > > > > Don't we also need to set SPEC_CTRL_IBRS in x86_spec_ctrl_base? > > Also, STIBP isn't needed with IBRS. Suggested changes: > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index 344ab7c9a4e2..498cb36587a3 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -897,11 +897,13 @@ spectre_v2_parse_user_cmdline(enum spectre_v2_mitigation_cmd v2_cmd) > return SPECTRE_V2_USER_CMD_AUTO; > } > > -static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) > +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) > { > - return (mode == SPECTRE_V2_EIBRS || > - mode == SPECTRE_V2_EIBRS_RETPOLINE || > - mode == SPECTRE_V2_EIBRS_LFENCE); > + > + return spectre_v2_mode == SPECTRE_V2_IBRS > + spectre_v2_mode == SPECTRE_V2_EIBRS || > + spectre_v2_mode == SPECTRE_V2_EIBRS_RETPOLINE || > + spectre_v2_mode == SPECTRE_V2_EIBRS_LFENCE; > } > > static void __init > @@ -966,12 +968,12 @@ spectre_v2_user_select_mitigation(enum spectre_v2_mitigation_cmd v2_cmd) > } > > /* > - * If no STIBP, enhanced IBRS is enabled or SMT impossible, STIBP is not > - * required. > + * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible, > + * STIBP is not required. > */ > if (!boot_cpu_has(X86_FEATURE_STIBP) || > !smt_possible || > - spectre_v2_in_eibrs_mode(spectre_v2_enabled)) > + spectre_v2_in_ibrs_mode(spectre_v2_enabled)) > return; > > /* > @@ -1171,7 +1173,7 @@ static void __init spectre_v2_select_mitigation(void) > if (mode == SPECTRE_V2_EIBRS && unprivileged_ebpf_enabled()) > pr_err(SPECTRE_V2_EIBRS_EBPF_MSG); > > - if (spectre_v2_in_eibrs_mode(mode)) { > + if (spectre_v2_in_ibrs_mode(mode)) { Pawan can you review the v2 that I sent out here: https://lore.kernel.org/lkml/20230221184908.2349578-1-kpsingh@kernel.org/T/#t > /* Force it so VMEXIT will restore correctly */ > x86_spec_ctrl_base |= SPEC_CTRL_IBRS; > wr_spec_ctrl(x86_spec_ctrl_base, true); > @@ -1212,19 +1214,17 @@ static void __init spectre_v2_select_mitigation(void) > pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n"); > > /* > - * Retpoline means the kernel is safe because it has no indirect > - * branches. Enhanced IBRS protects firmware too, so, enable restricted > - * speculation around firmware calls only when Enhanced IBRS isn't > - * supported or kernel IBRS isn't enabled. > + * Retpoline protects the kernel, but doesn't protect firmware. IBRS > + * and Enhanced IBRS protect firmware too, so enable IBRS around > + * firmware calls only when IBRS / Enhanced IBRS aren't otherwise > + * enabled. > * > * Use "mode" to check Enhanced IBRS instead of boot_cpu_has(), because > * the user might select retpoline on the kernel command line and if > * the CPU supports Enhanced IBRS, kernel might un-intentionally not > * enable IBRS around firmware calls. > */ > - if (boot_cpu_has(X86_FEATURE_IBRS) && > - !boot_cpu_has(X86_FEATURE_KERNEL_IBRS) && > - !spectre_v2_in_eibrs_mode(mode)) { > + if (boot_cpu_has(X86_FEATURE_IBRS) && !spectre_v2_in_ibrs_mode(mode)) { > setup_force_cpu_cap(X86_FEATURE_USE_IBRS_FW); > pr_info("Enabling Restricted Speculation for firmware calls\n"); > } > @@ -1937,7 +1937,7 @@ static ssize_t tsx_async_abort_show_state(char *buf) > > static char *stibp_state(void) > { > - if (spectre_v2_in_eibrs_mode(spectre_v2_enabled)) > + if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) > return ""; > > switch (spectre_v2_user_stibp) {
On Tue, Feb 21, 2023 at 05:26:31PM -0800, KP Singh wrote: > Pawan can you review the v2 that I sent out here: > > https://lore.kernel.org/lkml/20230221184908.2349578-1-kpsingh@kernel.org/T/#t Sure, looking at it.
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 85168740f76a..b97c0d28e573 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1124,14 +1124,19 @@ spectre_v2_parse_user_cmdline(void) return SPECTRE_V2_USER_CMD_AUTO; } -static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) +static inline bool spectre_v2_in_eibrs_mode(enum spectre_v2_mitigation mode) { - return mode == SPECTRE_V2_IBRS || - mode == SPECTRE_V2_EIBRS || + return mode == SPECTRE_V2_EIBRS || mode == SPECTRE_V2_EIBRS_RETPOLINE || mode == SPECTRE_V2_EIBRS_LFENCE; } +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) +{ + return spectre_v2_in_eibrs_mode(mode) || + mode == SPECTRE_V2_IBRS; +} + static void __init spectre_v2_user_select_mitigation(void) { @@ -1194,12 +1199,12 @@ spectre_v2_user_select_mitigation(void) } /* - * If no STIBP, IBRS or enhanced IBRS is enabled, or SMT impossible, + * If no STIBP, enhanced IBRS is enabled, or SMT impossible, * STIBP is not required. */ if (!boot_cpu_has(X86_FEATURE_STIBP) || !smt_possible || - spectre_v2_in_ibrs_mode(spectre_v2_enabled)) + spectre_v2_in_eibrs_mode(spectre_v2_enabled)) return; /* @@ -2327,9 +2332,6 @@ static ssize_t mmio_stale_data_show_state(char *buf) static char *stibp_state(void) { - if (spectre_v2_in_ibrs_mode(spectre_v2_enabled)) - return ""; - switch (spectre_v2_user_stibp) { case SPECTRE_V2_USER_NONE: return ", STIBP: disabled";