linux-kernel - Re: [PATCH v5 01/16] x86/bugs: Restructure MDS mitigation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250418204243.GQaAK5Q807BYWlABKV@fat_crate.local>
Date: Fri, 18 Apr 2025 22:42:43 +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 01/16] x86/bugs: Restructure MDS mitigation

On Fri, Apr 18, 2025 at 11:17:06AM -0500, David Kaplan wrote:
> @@ -284,6 +314,9 @@ enum rfds_mitigations {
>  static enum rfds_mitigations rfds_mitigation __ro_after_init =
>  	IS_ENABLED(CONFIG_MITIGATION_RFDS) ? RFDS_MITIGATION_AUTO : RFDS_MITIGATION_OFF;
>  
> +/* Set if any of MDS/TAA/MMIO/RFDS are going to enable VERW. */
> +static bool verw_mitigation_selected __ro_after_init;
> +

Yeah, pls pull that one up - see diff at the end.

>  static void __init mds_select_mitigation(void)
>  {
>  	if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off()) {
> @@ -294,12 +327,34 @@ static void __init mds_select_mitigation(void)
>  	if (mds_mitigation == MDS_MITIGATION_AUTO)
>  		mds_mitigation = MDS_MITIGATION_FULL;
>  
> +	if (mds_mitigation == MDS_MITIGATION_OFF)
> +		return;
> +
> +	verw_mitigation_selected = true;
> +}
> +
> +static void __init mds_update_mitigation(void)
> +{
> +	if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off())
> +		return;

Can we simply do

        if (mds_mitigation == MDS_MITIGATION_OFF)
                return;

here?

We already checked the X86_BUG and cpu_mitigations_off() in the select
function.

> +
> +	/* If TAA, MMIO, or RFDS are being mitigated, MDS gets mitigated too. */

A version of that comment is already over verw_mitigation_selected's
definition.

> +	if (verw_mitigation_selected)
> +		mds_mitigation = MDS_MITIGATION_FULL;

So we have this here now:

        if (mds_mitigation == MDS_MITIGATION_OFF)
                return;
                
        if (verw_mitigation_selected)
                mds_mitigation = MDS_MITIGATION_FULL;

or what you have:

	if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off())
		return;

	/* If TAA, MMIO, or RFDS are being mitigated, MDS gets mitigated too. */
	if (verw_mitigation_selected)
		mds_mitigation = MDS_MITIGATION_FULL;



Now, if the CPU is not affected by MDS, this second branch won't ever get set
because we will return earlier.

Which then means that "If TAA, MMIO, or RFDS are being mitigated, MDS gets
mitigated too" is not really true.

IOW, I'm wondering if this would be the more fitting order:

static void __init mds_update_mitigation(void)
{
        if (verw_mitigation_selected)
                mds_mitigation = MDS_MITIGATION_FULL;

        if (mds_mitigation == MDS_MITIGATION_OFF)
                return;

I.e., if *any* mitigation did set verw_mitigation_selected, even if the CPU is
not affected by MDS, it must set mds_mitigation to FULL too.

Hmmm?

---

All of the changes ontop:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 4295502ea082..61b9aaea8d09 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -87,6 +87,9 @@ static DEFINE_MUTEX(spec_ctrl_mutex);
 
 void (*x86_return_thunk)(void) __ro_after_init = __x86_return_thunk;
 
+/* Set if any of MDS/TAA/MMIO/RFDS are going to enable VERW. */
+static bool verw_mitigation_selected __ro_after_init;
+
 /* Update SPEC_CTRL MSR and its cached copy unconditionally */
 static void update_spec_ctrl(u64 val)
 {
@@ -314,9 +317,6 @@ enum rfds_mitigations {
 static enum rfds_mitigations rfds_mitigation __ro_after_init =
 	IS_ENABLED(CONFIG_MITIGATION_RFDS) ? RFDS_MITIGATION_AUTO : RFDS_MITIGATION_OFF;
 
-/* Set if any of MDS/TAA/MMIO/RFDS are going to enable VERW. */
-static bool verw_mitigation_selected __ro_after_init;
-
 static void __init mds_select_mitigation(void)
 {
 	if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off()) {
@@ -324,24 +324,23 @@ static void __init mds_select_mitigation(void)
 		return;
 	}
 
-	if (mds_mitigation == MDS_MITIGATION_AUTO)
-		mds_mitigation = MDS_MITIGATION_FULL;
-
 	if (mds_mitigation == MDS_MITIGATION_OFF)
 		return;
 
+	if (mds_mitigation == MDS_MITIGATION_AUTO)
+		mds_mitigation = MDS_MITIGATION_FULL;
+
 	verw_mitigation_selected = true;
 }
 
 static void __init mds_update_mitigation(void)
 {
-	if (!boot_cpu_has_bug(X86_BUG_MDS) || cpu_mitigations_off())
-		return;
-
-	/* If TAA, MMIO, or RFDS are being mitigated, MDS gets mitigated too. */
 	if (verw_mitigation_selected)
 		mds_mitigation = MDS_MITIGATION_FULL;
 
+	if (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;

-- 
Regards/Gruss,
    Boris.

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