| 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: <fe2dfd0b-6b2a-496e-b059-0600d2ae474c@suse.com>
Date: Wed, 2 Oct 2024 17:20:58 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Daniel Sneddon <daniel.sneddon@...ux.intel.com>,
Jonathan Corbet <corbet@....net>, Thomas Gleixner <tglx@...utronix.de>,
Borislav Petkov <bp@...en8.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
Cc: hpa@...or.com, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
pawan.kumar.gupta@...ux.intel.com
Subject: Re: [PATCH 6/6] x86/bugs: Clean-up verw mitigations
On 25.09.24 г. 1:31 ч., Daniel Sneddon wrote:
> The current md_clear routines duplicate a lot of code, and have to be
> called twice because if one of the mitigations gets enabled they all
> get enabled since it's the same instruction. This approach leads to
> code duplication and extra complexity.
>
> The only piece that really changes between the first call of
> *_select_mitigation() and the second is X86_FEATURE_CLEAR_CPU_BUF
> being set. Determine if this feature should be set prior to calling
> the _select_mitigation() routines. This not only means those functions
> only get called once, but it also simplifies them as well.
>
> Signed-off-by: Daniel Sneddon <daniel.sneddon@...ux.intel.com>
> ---
> arch/x86/kernel/cpu/bugs.c | 191 +++++++++++++++----------------------
> 1 file changed, 77 insertions(+), 114 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 45411880481c..412855391184 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -41,7 +41,6 @@ static void __init spectre_v2_user_select_mitigation(void);
> static void __init ssb_select_mitigation(void);
> static void __init l1tf_select_mitigation(void);
> static void __init mds_select_mitigation(void);
> -static void __init md_clear_update_mitigation(void);
> static void __init md_clear_select_mitigation(void);
> static void __init taa_select_mitigation(void);
> static void __init mmio_select_mitigation(void);
> @@ -244,21 +243,9 @@ static const char * const mds_strings[] = {
>
> static void __init mds_select_mitigation(void)
> {
> - if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off()) {
> - mds_mitigation = MDS_MITIGATION_OFF;
> - return;
> - }
> -
> - if (mds_mitigation == MDS_MITIGATION_FULL) {
> - if (!boot_cpu_has(X86_FEATURE_MD_CLEAR))
> - mds_mitigation = MDS_MITIGATION_VMWERV;
> -
> - setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> -
> - if (!boot_cpu_has(X86_BUG_MSBDS_ONLY) &&
> - (mds_nosmt || cpu_mitigations_auto_nosmt()))
> - cpu_smt_disable(false);
> - }
> + if (mds_mitigation == MDS_MITIGATION_FULL &&
> + !boot_cpu_has(X86_FEATURE_MD_CLEAR))
> + mds_mitigation = MDS_MITIGATION_VMWERV;
> }
>
> #undef pr_fmt
> @@ -284,35 +271,17 @@ static const char * const taa_strings[] = {
>
> static void __init taa_select_mitigation(void)
> {
> - if (!boot_cpu_has_bug(X86_BUG_TAA)) {
> - taa_mitigation = TAA_MITIGATION_OFF;
> - return;
> - }
> -
> /* TSX previously disabled by tsx=off */
> if (!boot_cpu_has(X86_FEATURE_RTM)) {
> taa_mitigation = TAA_MITIGATION_TSX_DISABLED;
> return;
> }
>
> - if (cpu_mitigations_off()) {
> - taa_mitigation = TAA_MITIGATION_OFF;
> + if (!boot_cpu_has(X86_FEATURE_MD_CLEAR)) {
> + taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> return;
> }
>
> - /*
> - * TAA mitigation via VERW is turned off if both
> - * tsx_async_abort=off and mds=off are specified.
> - */
> - if (taa_mitigation == TAA_MITIGATION_OFF &&
> - mds_mitigation == MDS_MITIGATION_OFF)
> - return;
> -
> - if (boot_cpu_has(X86_FEATURE_MD_CLEAR))
> - taa_mitigation = TAA_MITIGATION_VERW;
> - else
> - taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> -
> /*
> * VERW doesn't clear the CPU buffers when MD_CLEAR=1 and MDS_NO=1.
> * A microcode update fixes this behavior to clear CPU buffers. It also
> @@ -325,18 +294,6 @@ static void __init taa_select_mitigation(void)
> if ( (x86_arch_cap_msr & ARCH_CAP_MDS_NO) &&
> !(x86_arch_cap_msr & ARCH_CAP_TSX_CTRL_MSR))
> taa_mitigation = TAA_MITIGATION_UCODE_NEEDED;
> -
> - /*
> - * TSX is enabled, select alternate mitigation for TAA which is
> - * the same as MDS. Enable MDS static branch to clear CPU buffers.
> - *
> - * For guests that can't determine whether the correct microcode is
> - * present on host, enable the mitigation for UCODE_NEEDED as well.
> - */
> - setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> -
> - if (taa_nosmt || cpu_mitigations_auto_nosmt())
> - cpu_smt_disable(false);
> }
>
> #undef pr_fmt
> @@ -360,24 +317,6 @@ static const char * const mmio_strings[] = {
>
> static void __init mmio_select_mitigation(void)
> {
> - if (!boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
> - boot_cpu_has_bug(X86_BUG_MMIO_UNKNOWN) ||
> - cpu_mitigations_off()) {
> - mmio_mitigation = MMIO_MITIGATION_OFF;
> - return;
> - }
> -
> - if (mmio_mitigation == MMIO_MITIGATION_OFF)
> - return;
> -
> - /*
> - * 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.
> @@ -409,9 +348,6 @@ static void __init mmio_select_mitigation(void)
> mmio_mitigation = MMIO_MITIGATION_VERW;
> else
> mmio_mitigation = MMIO_MITIGATION_UCODE_NEEDED;
> -
> - if (mmio_nosmt || cpu_mitigations_auto_nosmt())
> - cpu_smt_disable(false);
> }
>
> #undef pr_fmt
> @@ -435,16 +371,7 @@ static const char * const rfds_strings[] = {
>
> static void __init rfds_select_mitigation(void)
> {
> - if (!boot_cpu_has_bug(X86_BUG_RFDS) || cpu_mitigations_off()) {
> - rfds_mitigation = RFDS_MITIGATION_OFF;
> - return;
> - }
> - if (rfds_mitigation == RFDS_MITIGATION_OFF)
> - return;
> -
> - if (x86_arch_cap_msr & ARCH_CAP_RFDS_CLEAR)
> - setup_force_cpu_cap(X86_FEATURE_CLEAR_CPU_BUF);
> - else
> + if (!(x86_arch_cap_msr & ARCH_CAP_RFDS_CLEAR))
> rfds_mitigation = RFDS_MITIGATION_UCODE_NEEDED;
> }
>
> @@ -485,41 +412,92 @@ static int __init clear_cpu_buffers_cmdline(char *str)
> }
> early_param("clear_cpu_buffers", clear_cpu_buffers_cmdline);
>
> -static void __init md_clear_update_mitigation(void)
> +static bool __init cpu_bug_needs_verw(void)
> {
> - if (cpu_mitigations_off())
> - return;
> + return boot_cpu_has_bug(X86_BUG_MDS) ||
> + boot_cpu_has_bug(X86_BUG_TAA) ||
> + boot_cpu_has_bug(X86_BUG_MMIO_STALE_DATA) ||
> + boot_cpu_has_bug(X86_BUG_RFDS);
> +}
>
> - if (!boot_cpu_has(X86_FEATURE_CLEAR_CPU_BUF))
> - goto out;
> +static bool __init verw_mitigations_disabled(void)
> +{
> + /*
> + * TODO: Create a single mitigation variable that will allow for setting
> + * the location of the mitigation, i.e.:
> + *
> + * kernel->user
> + * kvm->guest
> + * kvm->guest if device passthrough
> + * kernel->idle
> + */
> + return (mds_mitigation == MDS_MITIGATION_OFF &&
> + taa_mitigation == TAA_MITIGATION_OFF &&
> + mmio_mitigation == MMIO_MITIGATION_OFF &&
> + rfds_mitigation == RFDS_MITIGATION_OFF);
> +}
>
> +static void __init md_clear_select_mitigation(void)
> +{
> /*
> - * X86_FEATURE_CLEAR_CPU_BUF is now enabled. Update MDS, TAA and MMIO
> - * Stale Data mitigation, if necessary.
> + * If no CPU bug needs VERW, all VERW mitigations are disabled, or all
> + * mitigations are disabled we bail.
> */
> - if (mds_mitigation == MDS_MITIGATION_OFF &&
> - boot_cpu_has_bug(X86_BUG_MDS)) {
> + if (!cpu_bug_needs_verw() || verw_mitigations_disabled() ||
> + cpu_mitigations_off()) {
> + mds_mitigation = MDS_MITIGATION_OFF;
> + taa_mitigation = TAA_MITIGATION_OFF;
> + mmio_mitigation = MMIO_MITIGATION_OFF;
> + rfds_mitigation = RFDS_MITIGATION_OFF;
> + goto out;
> + }
> +
> + /* Check that at least one mitigation is using the verw mitigaiton.
> + * If the cpu doesn't have the correct ucode or if the BUG_* is mitigated
> + * by disabling a feature we won't want to use verw. Ignore MMIO
> + * for now since it depends on what the others choose.
> + */
> +
> + if (boot_cpu_has_bug(X86_BUG_MDS)) {
> mds_mitigation = MDS_MITIGATION_FULL;
> mds_select_mitigation();
> + } else {
> + mds_mitigation = MDS_MITIGATION_OFF;
> }
BUt with this logic if CONFIG_MITIGATION_MDS is deselected meaning
mds_mitigations will have the value MDS_MITIGATION_OFF, yet now you will
set it to _FULL thereby overriding the compile-time value of the user.
So shouldn't this condition be augmented to alsoo consider
CONFIG_MITIGATION_MDS compile time value?
<snip>
Powered by blists - more mailing lists