Re: [PATCH v4 03/36] x86/bugs: Restructure mmio mitigation

[Borislav Petkov]

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?

> +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?

> +
> +	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.

> +		/*
> +		 * 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...

> +		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.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette