| 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:
<LV3PR12MB92652075B8A3EBE26317641C94D32@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Thu, 13 Mar 2025 14:19:01 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Borislav Petkov <bp@...en8.de>, Pawan Gupta
<pawan.kumar.gupta@...ux.intel.com>
CC: Thomas Gleixner <tglx@...utronix.de>, Peter Zijlstra
<peterz@...radead.org>, Josh Poimboeuf <jpoimboe@...nel.org>, 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>, Brendan
Jackman <jackmanb@...gle.com>, Derek Manwaring <derekmn@...zon.com>
Subject: RE: [PATCH v4 03/36] x86/bugs: Restructure mmio mitigation
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Borislav Petkov <bp@...en8.de>
> Sent: Thursday, March 13, 2025 4:36 AM
> To: Kaplan, David <David.Kaplan@....com>; Pawan Gupta
> <pawan.kumar.gupta@...ux.intel.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>; Peter Zijlstra <peterz@...radead.org>;
> Josh Poimboeuf <jpoimboe@...nel.org>; 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; Brendan Jackman
> <jackmanb@...gle.com>; Derek Manwaring <derekmn@...zon.com>
> Subject: Re: [PATCH v4 03/36] x86/bugs: Restructure mmio mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Mon, Mar 10, 2025 at 11:39:50AM -0500, David Kaplan wrote:
> > @@ -511,24 +516,60 @@ static void __init mmio_select_mitigation(void)
> > return;
> > }
> >
> > - if (mmio_mitigation == MMIO_MITIGATION_OFF)
> > - return;
> > + /* Microcode will be checked in mmio_update_mitigation(). */
> > + if (mmio_mitigation == MMIO_MITIGATION_AUTO)
> > + mmio_mitigation = MMIO_MITIGATION_VERW;
> >
> > /*
> > * Enable CPU buffer clear mitigation for host and VMM, if also affected
> > - * by MDS or TAA. Otherwise, enable mitigation for VMM only.
> > + * by MDS or TAA.
> > */
> > - if (boot_cpu_has_bug(X86_BUG_MDS) ||
> (boot_cpu_has_bug(X86_BUG_TAA) &&
> > - boot_cpu_has(X86_FEATURE_RTM)))
> > - setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> > + if (boot_cpu_has_bug(X86_BUG_MDS) || taa_vulnerable())
> > + verw_mitigation_selected = true; }
>
> So applied this reads strange:
>
> if (mmio_mitigation == MMIO_MITIGATION_AUTO)
> mmio_mitigation = MMIO_MITIGATION_VERW;
>
> if (boot_cpu_has_bug(X86_BUG_MDS) || taa_vulnerable())
> verw_mitigation_selected = true;
>
> I'd expect to see:
>
> if (mmio_mitigation == MMIO_MITIGATION_AUTO) {
> mmio_mitigation = MMIO_MITIGATION_VERW;
> verw_mitigation_selected = true;
> }
>
> if (boot_cpu_has_bug(X86_BUG_MDS) || taa_vulnerable())
> verw_mitigation_selected = true;
>
> because the above branch already selected MMIO_MITIGATION_VERW so we
> might as well set verw_mitigation_selected, right?
Based on my understanding from the existing code, X86_FEATURE_CLEAR_CPU_BUF is only to be used if the CPU is also affected by MDS or TAA (as noted by the comment). verw_mitigation_selected really means that X86_FEATURE_CLEAR_CPU_BUF is going to be set.
>
> > +static void __init mmio_update_mitigation(void) {
> > + if (!boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
> cpu_mitigations_off())
> > + return;
> > +
> > + if (verw_mitigation_selected)
> > + mmio_mitigation = MMIO_MITIGATION_VERW;
>
> ... and the above change would obviate this one.
>
> Looking at that verw_mitigation_selected switch - that seems like the higher prio
> thing we should be looking at first as in: *something* selected VERW mitigation so
> we must honor it.
>
> And then the *_select_mitigation() functions will simply use the respective
> *_mitigation variable to perhaps override it only when really needed.
>
> I think.
>
> Or maybe I'm missing an aspect.
>
> Because if we make verw_mitigation_selected the higher prio thing, we can remove
> some of that additional checking.
>
> Or?
Not quite sure I follow. verw_mitigation_selected can be set by any of the VERW-related bugs. The update function runs after all the select functions so it knows if anybody else wanted VERW. If so (and the CPU is vulnerable), we update the mmio_mitigation option. But maybe I missed what you were trying to say here.
>
> > +
> > + if (mmio_mitigation == MMIO_MITIGATION_VERW) {
>
> I.e., in mmio_update_mitigation(), the only check you need to do is:
>
> if (verw_mitigation_selected)
>
> and then adjust mmio_mitigation depending on microcode presence or not.
Maybe this gets back to the point above that MMIO_MITIGATION_VERW does not necessarily mean verw_mitigation_selected.
MMIO_MITIGATION_VERW can imply just the need for the static branches to clear stale data in the VMM.
>
> > + /*
> > + * Check if the system has the right microcode.
> > + *
> > + * CPU Fill buffer clear mitigation is enumerated by either an explicit
> > + * FB_CLEAR or by the presence of both MD_CLEAR and L1D_FLUSH
> on MDS
> > + * affected systems.
> > + */
> > + if (!((x86_arch_cap_msr & ARCH_CAP_FB_CLEAR) ||
> > + (boot_cpu_has(X86_FEATURE_MD_CLEAR) &&
> > + boot_cpu_has(X86_FEATURE_FLUSH_L1D) &&
> > + !(x86_arch_cap_msr & ARCH_CAP_MDS_NO))))
> > + mmio_mitigation = MMIO_MITIGATION_UCODE_NEEDED;
>
> ... as you do here.
>
> > + }
> > +
> > + if (boot_cpu_has_bug(X86_BUG_MMIO_UNKNOWN))
>
> Btw, that UNKNOWN thing is just silly. Looking at git history:
>
> 7df548840c49 ("x86/bugs: Add "unknown" reporting for MMIO Stale Data")
>
> this was added just so that it doesn't say "Not affected" about those CPUs but
> "unknown."
>
> But
>
> "Mitigation is not deployed when the status is unknown."
>
> so if it is only about reporting, I think we can synthesize the logic of this:
>
> if (!arch_cap_mmio_immune(x86_arch_cap_msr)) {
> if (cpu_matches(cpu_vuln_blacklist, MMIO))
> setup_force_cpu_bug(X86_BUG_MMIO_STALE_DATA);
> else if (!cpu_matches(cpu_vuln_whitelist, NO_MMIO))
> setup_force_cpu_bug(X86_BUG_MMIO_UNKNOWN);
> }
>
> into a separate function and get rid of that X86_BUG_MMIO_UNKNOWN thing.
>
> Pawan?
>
> I'll try to whack it later to see how ugly it gets.
>
> > + pr_info("Unknown: No mitigations\n");
> > + else
> > + pr_info("%s\n", mmio_strings[mmio_mitigation]); }
> > +
> > +static void __init mmio_apply_mitigation(void) {
> > + if (mmio_mitigation == MMIO_MITIGATION_OFF)
> > + return;
> >
> > /*
> > - * X86_FEATURE_CLEAR_CPU_BUF could be enabled by other VERW
> based
> > - * mitigations, disable KVM-only mitigation in that case.
> > + * Only enable the VMM mitigation if the CPU buffer clear mitigation is
> > + * not being used.
>
> So this comment doesn't fit with what the code now does...
>
> > */
> > - if (boot_cpu_has(X86_FEATURE_CLEAR_CPU_BUF))
> > + if (verw_mitigation_selected) {
>
> ... which is to check whether something enabled the VERW mitigation...
I think the comment does fit. The code is only enabling the static branch for KVM if X86_FEATURE_CLEAR_CPU_BUF is not being used.
>
> > + setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> > static_branch_disable(&mmio_stale_data_clear);
> > - else
> > + } else
> > static_branch_enable(&mmio_stale_data_clear);
>
> { } around the else branch too pls.
Ok
--David Kaplan
Powered by blists - more mailing lists