| 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:
<LV3PR12MB92653F0B1D1D814A47591DF594F22@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Mon, 10 Feb 2025 17:27:01 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Brendan Jackman <jackmanb@...gle.com>
CC: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.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 v3 10/35] x86/bugs: Restructure gds mitigation
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Brendan Jackman <jackmanb@...gle.com>
> Sent: Monday, February 10, 2025 11:06 AM
> To: Kaplan, David <David.Kaplan@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>; Borislav Petkov <bp@...en8.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 v3 10/35] x86/bugs: Restructure gds mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, 8 Jan 2025 at 21:28, David Kaplan <david.kaplan@....com> wrote:
> >
> > Restructure gds mitigation to use select/apply functions to create
> > consistent vulnerability handling.
> >
> > Define new AUTO mitigation for gds.
> >
> > Signed-off-by: David Kaplan <david.kaplan@....com>
> > ---
> > arch/x86/kernel/cpu/bugs.c | 24 +++++++++++++++++++-----
> > 1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index fedd693b2218..58ac99b74bd3 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -76,6 +76,7 @@ static void __init srbds_apply_mitigation(void);
> > static void __init l1d_flush_select_mitigation(void);
> > static void __init srso_select_mitigation(void); static void __init
> > gds_select_mitigation(void);
> > +static void __init gds_apply_mitigation(void);
> >
> > /* The base value of the SPEC_CTRL MSR without task-specific bits set
> > */
> > u64 x86_spec_ctrl_base;
> > @@ -220,6 +221,7 @@ void __init cpu_select_mitigations(void)
> > mmio_apply_mitigation();
> > rfds_apply_mitigation();
> > srbds_apply_mitigation();
> > + gds_apply_mitigation();
> > }
> >
> > /*
> > @@ -811,6 +813,7 @@ early_param("l1d_flush", l1d_flush_parse_cmdline);
> >
> > enum gds_mitigations {
> > GDS_MITIGATION_OFF,
> > + GDS_MITIGATION_AUTO,
> > GDS_MITIGATION_UCODE_NEEDED,
> > GDS_MITIGATION_FORCE,
> > GDS_MITIGATION_FULL,
> > @@ -819,7 +822,7 @@ enum gds_mitigations { };
> >
> > static enum gds_mitigations gds_mitigation __ro_after_init =
> > - IS_ENABLED(CONFIG_MITIGATION_GDS) ? GDS_MITIGATION_FULL :
> GDS_MITIGATION_OFF;
> > + IS_ENABLED(CONFIG_MITIGATION_GDS) ? GDS_MITIGATION_AUTO
> :
> > + GDS_MITIGATION_OFF;
> >
> > static const char * const gds_strings[] = {
> > [GDS_MITIGATION_OFF] = "Vulnerable",
> > @@ -860,6 +863,7 @@ void update_gds_msr(void)
> > case GDS_MITIGATION_FORCE:
> > case GDS_MITIGATION_UCODE_NEEDED:
> > case GDS_MITIGATION_HYPERVISOR:
> > + case GDS_MITIGATION_AUTO:
> > return;
> > }
> >
> > @@ -883,13 +887,16 @@ static void __init gds_select_mitigation(void)
> >
> > if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) {
> > gds_mitigation = GDS_MITIGATION_HYPERVISOR;
> > - goto out;
> > + return;
> > }
> >
> > if (cpu_mitigations_off())
> > gds_mitigation = GDS_MITIGATION_OFF;
> > /* Will verify below that mitigation _can_ be disabled */
> >
> > + if (gds_mitigation == GDS_MITIGATION_AUTO)
> > + gds_mitigation = GDS_MITIGATION_FULL;
> > +
> > /* No microcode */
> > if (!(x86_arch_cap_msr & ARCH_CAP_GDS_CTRL)) {
> > if (gds_mitigation == GDS_MITIGATION_FORCE) { @@
> > -902,7 +909,7 @@ static void __init gds_select_mitigation(void)
> > } else {
> > gds_mitigation = GDS_MITIGATION_UCODE_NEEDED;
> > }
> > - goto out;
> > + return;
> > }
> >
> > /* Microcode has mitigation, use it */ @@ -923,9 +930,16 @@
> > static void __init gds_select_mitigation(void)
> > */
> > gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> > }
> > +}
> > +
> > +static void __init gds_apply_mitigation(void) {
> > + if (!boot_cpu_has_bug(X86_BUG_GDS))
> > + return;
> > +
> > + if (x86_arch_cap_msr & ARCH_CAP_GDS_CTRL)
> > + update_gds_msr();
>
> IMO it's a shame to be looking at MSR bits in here instead of just relying on the
> direct output of the select/update functions.
>
> I think in this case we can just remove the conditional since if
> !ARCH_CAP_GDS_CTRL then gds_mitigation must be FORCE or
> UCODE_NEEDED in which case update_gds_msr() is a nop.
>
> Now I make these comments I realise maybe my expectation about these three
> functions is not actually the same as yours. Here's how I envisaged your design:
>
> - select: Look around at the hardware and the cmdline and decide what we think we
> wanna do in fairly abstract terms. Record that result in *_mitigation.
>
> - update: Look around at the other mitigations and potentially change our mind (or
> perhaps just update *_mitigation to reflect mitigation that is being done regardless
> for other vulns, which also mitigate this vuln).
>
> - apply: Poke the hardware/cap flags/static keys/etc to actuate the decision we
> made in the previous steps.
>
> Let me know if that's aligned with your vision or not.
Yes, that is well-aligned. Basically select picks a mitigation based on the hardware and cmdline but not the mitigations of any other bugs. Then update re-evaluates that and may change our mind, and apply pokes the hardware.
On the ARCH_CAP_GDS_CTRL, I thought that check is really just to check if the MSR is present, before we try to read it.
--David Kaplan
Powered by blists - more mailing lists