linux-kernel - Re: [PATCH 16/29] x86/bugs: Disable Retpoline when IBT

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]

Date:   Fri, 18 Feb 2022 18:15:30 -0800
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     x86@...nel.org, joao@...rdrivepizza.com, hjl.tools@...il.com,
        andrew.cooper3@...rix.com, linux-kernel@...r.kernel.org,
        ndesaulniers@...gle.com, keescook@...omium.org,
        samitolvanen@...gle.com, mark.rutland@....com,
        alyssa.milburn@...el.com
Subject: Re: [PATCH 16/29] x86/bugs: Disable Retpoline when IBT

On Fri, Feb 18, 2022 at 05:49:18PM +0100, Peter Zijlstra wrote:
> Retpoline and IBT are mutually exclusive. IBT relies on indirect
> branches (JMP/CALL *%reg) while retpoline avoids them by design.
> 
> Demote to LFENCE on IBT enabled hardware.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  arch/x86/kernel/cpu/bugs.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -937,6 +937,11 @@ static void __init spectre_v2_select_mit
>  	    boot_cpu_data.x86_vendor == X86_VENDOR_HYGON) {
>  	retpoline_amd:
>  		if (!boot_cpu_has(X86_FEATURE_LFENCE_RDTSC)) {
> +			if (IS_ENABLED(CONFIG_X86_IBT) &&
> +			    boot_cpu_has(X86_FEATURE_IBT)) {
> +				pr_err("Spectre mitigation: LFENCE not serializing, generic retpoline not available due to IBT, switching to none\n");
> +				return;
> +			}
>  			pr_err("Spectre mitigation: LFENCE not serializing, switching to generic retpoline\n");
>  			goto retpoline_generic;
>  		}
> @@ -945,6 +950,26 @@ static void __init spectre_v2_select_mit
>  		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
>  	} else {
>  	retpoline_generic:
> +		/*
> +		 *  Full retpoline is incompatible with IBT, demote to LFENCE.
> +		 */
> +		if (IS_ENABLED(CONFIG_X86_IBT) &&
> +		    boot_cpu_has(X86_FEATURE_IBT)) {
> +			switch (cmd) {
> +			case SPECTRE_V2_CMD_FORCE:
> +			case SPECTRE_V2_CMD_AUTO:
> +			case SPECTRE_V2_CMD_RETPOLINE:
> +				/* silent for auto select */
> +				break;
> +
> +			default:
> +				/* warn when 'demoting' an explicit selection */
> +				pr_warn("Spectre mitigation: Switching to LFENCE due to IBT\n");
> +				break;

This code is confusing, not helped by the fact that the existing code
already looks like spaghetti.

Assuming IBT systems also have eIBRS (right?), I don't think the above
SPECTRE_V2_CMD_{FORCE,AUTO} cases would be possible.

AFAICT, if execution reached the retpoline_generic label, the user
specified either RETPOLINE or RETPOLINE_GENERIC.

I'm not sure it makes sense to put RETPOLINE in the "silent" list.  If
the user boots an Intel system with spectre_v2=retpoline on the cmdline,
they're probably expecting a traditional retpoline and should be warned
if that changes, especially if it's a "demotion".

In that case the switch statement isn't even needed.  It can instead
just unconditinoally print the warning.


Also, why "demote" retpoline to LFENCE rather than attempting to
"promote" it to eIBRS?  Maybe there's a good reason but it probably at
least deserves some mention in the commit log.

-- 
Josh