| 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
| ||
|
Message-ID:
<LV3PR12MB9265CC8CA5922E081A5BDAD394812@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Mon, 28 Apr 2025 20:55:13 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Borislav Petkov <bp@...en8.de>
CC: Thomas Gleixner <tglx@...utronix.de>, Peter Zijlstra
<peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>, Pawan Gupta
<pawan.kumar.gupta@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>, Dave
Hansen <dave.hansen@...ux.intel.com>, "x86@...nel.org" <x86@...nel.org>, "H .
Peter Anvin" <hpa@...or.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 10/16] x86/bugs: Restructure retbleed mitigation
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Borislav Petkov <bp@...en8.de>
> Sent: Monday, April 28, 2025 2:00 PM
> To: Kaplan, David <David.Kaplan@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>; Peter Zijlstra <peterz@...radead.org>;
> Josh Poimboeuf <jpoimboe@...nel.org>; Pawan Gupta
> <pawan.kumar.gupta@...ux.intel.com>; Ingo Molnar <mingo@...hat.com>; Dave
> Hansen <dave.hansen@...ux.intel.com>; x86@...nel.org; H . Peter Anvin
> <hpa@...or.com>; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v5 10/16] x86/bugs: Restructure retbleed mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Fri, Apr 18, 2025 at 11:17:15AM -0500, David Kaplan wrote:
> > @@ -187,11 +189,6 @@ void __init cpu_select_mitigations(void)
> > /* Select the proper CPU mitigations before patching alternatives: */
> > spectre_v1_select_mitigation();
> > spectre_v2_select_mitigation();
> > - /*
> > - * retbleed_select_mitigation() relies on the state set by
> > - * spectre_v2_select_mitigation(); specifically it wants to know about
> > - * spectre_v2=ibrs.
> > - */
> > retbleed_select_mitigation();
> > /*
> > * spectre_v2_user_select_mitigation() relies on the state set
> > by @@ -219,12 +216,14 @@ void __init cpu_select_mitigations(void)
> > * After mitigations are selected, some may need to update their
> > * choices.
> > */
> > + retbleed_update_mitigation();
>
> Is there any particular reason for the retbleed update function to go first...
>
> > mds_update_mitigation();
> > taa_update_mitigation();
> > mmio_update_mitigation();
> > rfds_update_mitigation();
>
> ... before those?
It's really just following the same order as the order that the select functions are called, which largely matches the order in existing code.
>
> I'm under the assumption that the new scheme would get rid of this magical
> ordering requirement between the mitigations...
While this is mostly true, there's still a few dependencies with the update functions which are clearly noted with comments.
>
> Your commit message is alluding to that but we need to specify this clearly for future
> cleanups/changes here.
>
> > spectre_v1_apply_mitigation();
> > + retbleed_apply_mitigation();
>
> This too.
>
> > mds_apply_mitigation();
> > taa_apply_mitigation();
> > mmio_apply_mitigation();
>
> ...
>
> > -do_cmd_auto:
> > - case RETBLEED_CMD_AUTO:
> > + if (retbleed_mitigation == RETBLEED_MITIGATION_AUTO) {
> > + /* Intel mitigation selected in
> > + retbleed_update_mitigation() */
> > if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> > boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> > if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
> > @@ -1212,18 +1187,65 @@ static void __init retbleed_select_mitigation(void)
> > else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
> > boot_cpu_has(X86_FEATURE_IBPB))
> > retbleed_mitigation =
> > RETBLEED_MITIGATION_IBPB;
> > + else
> > + retbleed_mitigation =
> > + RETBLEED_MITIGATION_NONE;
> > }
> > + }
> > +}
>
> I'd flip that outer check in order to save an indentation level here:
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index
> 9d6ce4a167be..207a472d1a6e 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1182,18 +1182,19 @@ static void __init retbleed_select_mitigation(void)
> break;
> }
>
> - if (retbleed_mitigation == RETBLEED_MITIGATION_AUTO) {
> - /* Intel mitigation selected in retbleed_update_mitigation() */
> - if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> - boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> - if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
> - retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> - else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
> - boot_cpu_has(X86_FEATURE_IBPB))
> - retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
> - else
> - retbleed_mitigation = RETBLEED_MITIGATION_NONE;
> - }
> + if (retbleed_mitigation != RETBLEED_MITIGATION_AUTO)
> + return;
> +
> + /* Intel mitigation selected in retbleed_update_mitigation() */
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
> + boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
> + if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
> + retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
> + else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
> + boot_cpu_has(X86_FEATURE_IBPB))
> + retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
> + else
> + retbleed_mitigation = RETBLEED_MITIGATION_NONE;
> }
> }
>
Yeah this makes sense.
Thanks --David Kaplan
Powered by blists - more mailing lists