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

Josh Poimboeuf
2022-03-04T17:39:49Z

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@infradead.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