| 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: <20250424201918.GAaAqcxqFV0raTOOKP@renoirsky.local>
Date: Thu, 24 Apr 2025 22:19:18 +0200
From: Borislav Petkov <bp@...en8.de>
To: David Kaplan <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 03/16] x86/bugs: Restructure MMIO mitigation
On Fri, Apr 18, 2025 at 11:17:08AM -0500, David Kaplan wrote:
> +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.
> */
> - if (boot_cpu_has(X86_FEATURE_CLEAR_CPU_BUF))
> + if (verw_mitigation_selected) {
> + setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> static_branch_disable(&cpu_buf_vm_clear);
> - else
> + } else {
> static_branch_enable(&cpu_buf_vm_clear);
> + }
Sorry, but I'm still not happy about this.
After this patch, we have:
/*
* Enable CPU buffer clear mitigation for host and VMM, if also affected
* by MDS or TAA.
*/
if (boot_cpu_has_bug(X86_BUG_MDS) || taa_vulnerable())
verw_mitigation_selected = true;
in the select function.
The comment is wrong. The code does: enable the VERW mitigation for
MMIO if affected by MDS or TAA. verw_mitigation_selected doesn't have
any bearing on whether this should be a host or VMM mitigation - as its
name says, a VERW mitigation has been selected.
Then in the apply function:
/*
* Only enable the VMM mitigation if the CPU buffer clear mitigation is
* not being used.
*/
if (verw_mitigation_selected) {
setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
static_branch_disable(&cpu_buf_vm_clear);
} else {
static_branch_enable(&cpu_buf_vm_clear);
}
Comment is again wrong. verw_mitigation_selected doesn't mean the CPU
buffer clear mitigation is not being used.
Yes yes, it boils down to the same thing in the end but reading it confusing
as hell. verw_mitigation_selected means what its name is: a VERW
mitigation has been selected and nothing else.
Looking at the old code - that you can actually follow:
---
/*
* 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);
/*
* 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);
---
because verw_mitigation_selected didn't exist.
And maybe it shouldn't be used here because that variable simply doesn't
fit here with its meaning.
Now, if this variable and the static key were called:
if (full_verw_mitigation_selected)
setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
static_branch_disable(&clear_cpu_buf_vm);
} else {
static_branch_enable(&clear_cpu_buf_vm);
}
then the code makes total sense all of a sudden.
A full VERW mitigation means CLEAR_CPU_BUF, while the VMM only means,
well, clear_cpu_buf_vm_only.
Renaming the var is probably unnecessary churn but you can fix the
comments and still rename the key:
---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index c97ded4d55e5..4a5bd6214508 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -157,8 +157,8 @@ DEFINE_STATIC_KEY_FALSE(switch_mm_cond_l1d_flush);
* X86_FEATURE_CLEAR_CPU_BUF, and should only be enabled when KVM-only
* mitigation is required.
*/
-DEFINE_STATIC_KEY_FALSE(cpu_buf_vm_clear);
-EXPORT_SYMBOL_GPL(cpu_buf_vm_clear);
+DEFINE_STATIC_KEY_FALSE(clear_cpu_buf_vm);
+EXPORT_SYMBOL_GPL(clear_cpu_buf_vm);
void __init cpu_select_mitigations(void)
{
@@ -528,10 +528,7 @@ static void __init mmio_select_mitigation(void)
if (mmio_mitigation == MMIO_MITIGATION_OFF)
return;
- /*
- * Enable CPU buffer clear mitigation for host and VMM, if also affected
- * by MDS or TAA.
- */
+ /* Enable full VERW mitigation if also affected by MDS or TAA. */
if (boot_cpu_has_bug(X86_BUG_MDS) || taa_vulnerable())
verw_mitigation_selected = true;
}
@@ -568,14 +565,14 @@ static void __init mmio_apply_mitigation(void)
return;
/*
- * Only enable the VMM mitigation if the CPU buffer clear mitigation is
- * not being used.
+ * Full VERW mitigation selection enables host and VMENTER buffer clearing,
+ * otherwise buffer clearing only on VMENTER.
*/
if (verw_mitigation_selected) {
setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
- static_branch_disable(&cpu_buf_vm_clear);
+ static_branch_disable(&clear_cpu_buf_vm);
} else {
- static_branch_enable(&cpu_buf_vm_clear);
+ static_branch_enable(&clear_cpu_buf_vm);
}
/*
@@ -681,7 +678,7 @@ static void __init md_clear_update_mitigation(void)
taa_select_mitigation();
}
/*
- * MMIO_MITIGATION_OFF is not checked here so that cpu_buf_vm_clear
+ * MMIO_MITIGATION_OFF is not checked here so that clear_cpu_buf_vm
* gets updated correctly as per X86_FEATURE_CLEAR_CPU_BUF state.
*/
if (boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA)) {
---
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Powered by blists - more mailing lists