linux-kernel - Re: [PATCH v3 25/39] 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]

Message-ID: <20220304173939.rea2ud2h45l2gcpj@treble>
Date:   Fri, 4 Mar 2022 09:39:39 -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, mbenes@...e.cz, rostedt@...dmis.org,
        mhiramat@...nel.org, alexei.starovoitov@...il.com
Subject: Re: [PATCH v3 25/39] x86/bugs: Disable Retpoline when IBT

On Thu, Mar 03, 2022 at 12:23:46PM +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 |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -891,6 +891,7 @@ static void __init spectre_v2_select_mit
>  {
>  	enum spectre_v2_mitigation_cmd cmd = spectre_v2_parse_cmdline();
>  	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
> +	bool silent_demote = false;
>  
>  	/*
>  	 * If the CPU is not affected and the command line mode is NONE or AUTO
> @@ -906,6 +907,7 @@ static void __init spectre_v2_select_mit
>  
>  	case SPECTRE_V2_CMD_FORCE:
>  	case SPECTRE_V2_CMD_AUTO:
> +		silent_demote = true;
>  		if (boot_cpu_has(X86_FEATURE_IBRS_ENHANCED)) {
>  			mode = SPECTRE_V2_IBRS_ENHANCED;
>  			/* Force it so VMEXIT will restore correctly */
> @@ -938,6 +940,7 @@ static void __init spectre_v2_select_mit
>  	retpoline_amd:
>  		if (!boot_cpu_has(X86_FEATURE_LFENCE_RDTSC)) {
>  			pr_err("Spectre mitigation: LFENCE not serializing, switching to generic retpoline\n");
> +			silent_demote = false;
>  			goto retpoline_generic;
>  		}
>  		mode = SPECTRE_V2_RETPOLINE_AMD;
> @@ -947,6 +950,16 @@ static void __init spectre_v2_select_mit
>  	retpoline_generic:
>  		mode = SPECTRE_V2_RETPOLINE_GENERIC;
>  		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
> +
> +		/*
> +		 * ROP defeats IBT, make sure not to use Retpolines and IBT together.
> +		 */
> +		if (HAS_KERNEL_IBT && cpu_feature_enabled(X86_FEATURE_IBT)) {
> +			if (!silent_demote)
> +				pr_warn("Spectre mitigation: Switching to LFENCE due to IBT\n");
> +			mode = SPECTRE_V2_RETPOLINE_AMD;
> +			setup_force_cpu_cap(X86_FEATURE_RETPOLINE_AMD);
> +		}

This is better.  But AFAIK, the 'silent_demote' case should only happen
on a hypothetically weird/broken virt setup, right?  Why silence the
warning?  It still seems legit.  If you have IBT on non-eIBRS Intel, and
you get silently demoted from retpoline to lfence, it presumably opens
up some Spectre v2 attack vectors, despite IBT's implicit promise of
providing *more* protection overall.

And actually, after thinking about it, my most preferred approach would
be to do the converse of this patch: only enable IBT if
!X86_FEATURE_RETPOLINE.  And do a real warning, like "your setup is
broken, IBT is disabled".  Maybe even make it a real WARN() so we could
find out if it's a real possibility.  Since this feature is much newer
than retpoline, that would probably be the simplest and least surprising
option.

-- 
Josh