linux-kernel - Re: [PATCH v5 10/16] x86/bugs: Restructure retbleed 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: <20250428185939.GBaA_QG-bWQ7WQ3vlY@fat_crate.local>
Date: Mon, 28 Apr 2025 20:59:39 +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 10/16] x86/bugs: Restructure retbleed mitigation

On Fri, Apr 18, 2025 at 11:17:15AM -0500, David Kaplan wrote:
> @@ -187,11 +189,6 @@ void __init cpu_select_mitigations(void)
>  	/* Select the proper CPU mitigations before patching alternatives: */
>  	spectre_v1_select_mitigation();
>  	spectre_v2_select_mitigation();
> -	/*
> -	 * retbleed_select_mitigation() relies on the state set by
> -	 * spectre_v2_select_mitigation(); specifically it wants to know about
> -	 * spectre_v2=ibrs.
> -	 */
>  	retbleed_select_mitigation();
>  	/*
>  	 * spectre_v2_user_select_mitigation() relies on the state set by
> @@ -219,12 +216,14 @@ void __init cpu_select_mitigations(void)
>  	 * After mitigations are selected, some may need to update their
>  	 * choices.
>  	 */
> +	retbleed_update_mitigation();

Is there any particular reason for the retbleed update function to go first...

>  	mds_update_mitigation();
>  	taa_update_mitigation();
>  	mmio_update_mitigation();
>  	rfds_update_mitigation();

... before those?

I'm under the assumption that the new scheme would get rid of this magical
ordering requirement between the mitigations...

Your commit message is alluding to that but we need to specify this clearly
for future cleanups/changes here.

>  	spectre_v1_apply_mitigation();
> +	retbleed_apply_mitigation();

This too.

>  	mds_apply_mitigation();
>  	taa_apply_mitigation();
>  	mmio_apply_mitigation();

...

> -do_cmd_auto:
> -	case RETBLEED_CMD_AUTO:
> +	if (retbleed_mitigation == RETBLEED_MITIGATION_AUTO) {
> +		/* Intel mitigation selected in retbleed_update_mitigation() */
>  		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
>  		    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>  			if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
> @@ -1212,18 +1187,65 @@ static void __init retbleed_select_mitigation(void)
>  			else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
>  				 boot_cpu_has(X86_FEATURE_IBPB))
>  				retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
> +			else
> +				retbleed_mitigation = RETBLEED_MITIGATION_NONE;
>  		}
> +	}
> +}

I'd flip that outer check in order to save an indentation level here:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 9d6ce4a167be..207a472d1a6e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1182,18 +1182,19 @@ static void __init retbleed_select_mitigation(void)
 		break;
 	}
 
-	if (retbleed_mitigation == RETBLEED_MITIGATION_AUTO) {
-		/* Intel mitigation selected in retbleed_update_mitigation() */
-		if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
-		    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
-			if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
-				retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
-			else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
-				 boot_cpu_has(X86_FEATURE_IBPB))
-				retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
-			else
-				retbleed_mitigation = RETBLEED_MITIGATION_NONE;
-		}
+	if (retbleed_mitigation != RETBLEED_MITIGATION_AUTO)
+		return;
+
+	/* Intel mitigation selected in retbleed_update_mitigation() */
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD ||
+	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
+		if (IS_ENABLED(CONFIG_MITIGATION_UNRET_ENTRY))
+			retbleed_mitigation = RETBLEED_MITIGATION_UNRET;
+		else if (IS_ENABLED(CONFIG_MITIGATION_IBPB_ENTRY) &&
+			 boot_cpu_has(X86_FEATURE_IBPB))
+			retbleed_mitigation = RETBLEED_MITIGATION_IBPB;
+		else
+			retbleed_mitigation = RETBLEED_MITIGATION_NONE;
 	}
 }
 
Thx.

-- 
Regards/Gruss,
    Boris.

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