Re: [PATCH v3 06/35] x86/bugs: Restructure mmio mitigation

[Josh Poimboeuf]

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@xxxxxxx>
> ---
>  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.

> +	/* Microcode will be checked in mmio_update_mitigation(). */
> +	if (mmio_mitigation == MMIO_MITIGATION_AUTO)
> +		mmio_mitigation = MMIO_MITIGATION_VERW;
> +
> +}

Extra whitespace.

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

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

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().

-- 
Josh