| 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:
<LV3PR12MB926503692B759DF39113626294FD2@LV3PR12MB9265.namprd12.prod.outlook.com>
Date: Tue, 11 Feb 2025 20:35:27 +0000
From: "Kaplan, David" <David.Kaplan@....com>
To: Josh Poimboeuf <jpoimboe@...nel.org>
CC: Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
Peter Zijlstra <peterz@...radead.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 06/35] x86/bugs: Restructure mmio mitigation
[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Josh Poimboeuf <jpoimboe@...nel.org>
> Sent: Monday, February 10, 2025 5:30 PM
> To: Kaplan, David <David.Kaplan@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>; Borislav Petkov <bp@...en8.de>; Peter
> Zijlstra <peterz@...radead.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 06/35] x86/bugs: Restructure mmio mitigation
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Wed, Jan 08, 2025 at 02:24:46PM -0600, David Kaplan wrote:
> > Restructure mmio mitigation to use select/update/apply functions to
> > create consistent vulnerability handling.
> >
> > Signed-off-by: David Kaplan <david.kaplan@....com>
> > ---
> > arch/x86/kernel/cpu/bugs.c | 60
> > ++++++++++++++++++++++++++++----------
> > 1 file changed, 44 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index 7beb2d6c43bb..a8da097ab2d5 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -68,6 +68,8 @@ static void __init taa_select_mitigation(void);
> > static void __init taa_update_mitigation(void); static void __init
> > taa_apply_mitigation(void); static void __init
> > mmio_select_mitigation(void);
> > +static void __init mmio_update_mitigation(void); static void __init
> > +mmio_apply_mitigation(void);
> > static void __init srbds_select_mitigation(void); static void __init
> > l1d_flush_select_mitigation(void);
> > static void __init srso_select_mitigation(void); @@ -190,6 +192,7 @@
> > void __init cpu_select_mitigations(void)
> > l1tf_select_mitigation();
> > mds_select_mitigation();
> > taa_select_mitigation();
> > + mmio_select_mitigation();
> > md_clear_select_mitigation();
> > srbds_select_mitigation();
> > l1d_flush_select_mitigation();
> > @@ -207,9 +210,11 @@ void __init cpu_select_mitigations(void)
> > */
> > mds_update_mitigation();
> > taa_update_mitigation();
> > + mmio_update_mitigation();
> >
> > mds_apply_mitigation();
> > taa_apply_mitigation();
> > + mmio_apply_mitigation();
> > }
> >
> > /*
> > @@ -510,6 +515,45 @@ static void __init mmio_select_mitigation(void)
> > return;
> > }
> >
> > + if (mmio_mitigation == MMIO_MITIGATION_OFF)
> > + return;
>
> Another seemingly pointless return, the only thing after this is the
> MMIO_MITIGATION_AUTO check.
Ack
>
> > + /* Microcode will be checked in mmio_update_mitigation(). */
> > + if (mmio_mitigation == MMIO_MITIGATION_AUTO)
> > + mmio_mitigation = MMIO_MITIGATION_VERW;
> > +
> > +}
>
> Extra whitespace.
Ack
>
> > +
> > +static void __init mmio_update_mitigation(void) {
> > + if (!boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
> cpu_mitigations_off())
> > + return;
> > +
> > + if (verw_mitigation_enabled())
> > + mmio_mitigation = MMIO_MITIGATION_VERW;
> > +
> > + if (mmio_mitigation == MMIO_MITIGATION_VERW) {
> > + /*
> > + * 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;
> > + }
> > +
> > + pr_info("%s\n", mmio_strings[mmio_mitigation]);
> > + if (boot_cpu_has_bug(X86_BUG_MMIO_UNKNOWN))
> > + pr_info("Unknown: No mitigations\n");
>
> Seems weird to print two messages for the X86_BUG_MMIO_UNKNOWN case?
>
> And note that if it gets enabled by verw_mitigation_enabled() it prints:
>
> MMIO Stale Data: Mitigation: Clear CPU buffers
> MMIO Stale Data: Unknown: No mitigations
>
> which is confusing at best :-)
>
> It should probably just print either one or the other, like it did before (and like
> mmio_stale_data_show_state() does).
Good catch, will fix.
>
> > @@ -538,21 +582,6 @@ static void __init mmio_select_mitigation(void)
> > if (!(x86_arch_cap_msr & ARCH_CAP_FBSDP_NO))
> > static_branch_enable(&mds_idle_clear);
>
> Right here it does the following:
>
> /*
> * Enable CPU buffer clear mitigation for host and VMM, if also affected
> * by MDS or TAA. Otherwise, enable mitigation for VMM only.
> */
> 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);
>
> Isn't that a cross-mitigation dependency? i.e. if
> X86_FEATURE_CLEAR_CPU_BUF gets enabled here then the other mitigations
> would need to update their mitigation reporting?
I don't think so, nobody should be looking at X86_FEATURE_CLEAR_CPU_BUF to determine their mitigation selection, they should only be looking at the other variables like taa_mitigation as done in the verw_mitigation_enabled() function.
>
> Maybe that check can be done in mmio_select_mitigation()?
>
>
> Then, after that there's:
>
> /*
> * X86_FEATURE_CLEAR_CPU_BUF could be enabled by other VERW
> based
> * mitigations, disable KVM-only mitigation in that case.
> */
> if (boot_cpu_has(X86_FEATURE_CLEAR_CPU_BUF))
> static_branch_disable(&mmio_stale_data_clear);
> else
> static_branch_enable(&mmio_stale_data_clear);
>
> which assumes this is called after the other VERW-enabling
> *_apply_mitigation() functions. It feels like this decision should be
> made in mmio_update_mitigation().
>
Hmm. So I think the only case that is relevant here is if the CPU is immune to MDS and TAA, but is vulnerable to MMIO and RFDS. rfds_apply_mitigation will force X86_FEATURE_CLEAR_CPU_BUF and in that case, we want to disable the static branch. If the CPU was vulnerable to MDS or TAA then MMIO would force X86_FEATURE_CLEAR_CPU_BUF on its own.
So I think that mmio_apply_mitigation could better handle this by checking rfds_mitigation to decide whether to disable the static branch.
Like:
/*
* Enable CPU buffer clear mitigation for host and VMM, if also affected
* by MDS or TAA.
*
* Only enable the VMM mitigation if the CPU buffer clear mitigation is
* not being used.
*/
if (rfds_mitigation == RFDS_MITIGATION_VERW ||
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);
static_branch_disable(&mmio_stale_data_clear);
} else
static_branch_enable(&mmio_stale_data_clear);
Does that sound right?
--David Kaplan
Powered by blists - more mailing lists