Re: [PATCH v3 25/39] x86/bugs: Disable Retpoline when IBT

[Josh Poimboeuf]

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