Messages in this thread Patch in this message | | | Date | Thu, 11 Apr 2024 20:57:40 -0700 | From | Josh Poimboeuf <> | Subject | Re: [PATCH 5/7] x86/bugs: Only harden syscalls when needed |
| |
On Thu, Apr 11, 2024 at 05:15:22PM -0700, Pawan Gupta wrote: > > + * Do either a direct or an indirect call, depending on whether indirect calls > > + * are considered safe. > > + */ > > +#define __do_syscall(table, func_direct, nr, regs) \ > > +({ \ > > + unsigned long __rax, __rdi, __rsi; \ > > + \ > > + asm_inline volatile( \ > > + ALTERNATIVE("call " __stringify(func_direct) "\n\t", \ > > + ANNOTATE_RETPOLINE_SAFE \ > > + "call *%[func_ptr]\n\t", \ > > This will likely not insert the lfence before the indirect call in > spectre_v2=eibrs,lfence mode. As X86_FEATURE_INDIRECT_SAFE is not > cleared when eIBRS is enabled, this will not be converted to direct > call.
Hm, I think the problem here is that SPECTRE_V2_EIBRS_LFENCE confusingly sets X86_FEATURE_RETPOLINE. So the following bit unintentionally takes effect:
/* Retpoline mitigates against BHI unless the CPU has RRSBA behavior */ if (cpu_feature_enabled(X86_FEATURE_RETPOLINE)) { spec_ctrl_disable_kernel_rrsba(); if (rrsba_disabled) return; }
If RRSBA gets disabled (which is likely), bhi_select_mitigation() returns early and X86_FEATURE_INDIRECT_SAFE doesn't get cleared.
"LFENCE; CALL" is most definitely not a retpoline, so it's weird for SPECTRE_V2_EIBRS_LFENCE to be setting X86_FEATURE_RETPOLINE. We should fix that.
Honestly, I think SPECTRE_V2_EIBRS_LFENCE is obsolete anyway. It was originally intended to be a BHI mitigation, but the real-world benchmarks I've seen are showing it to be quite a bit slower than the actual BHI mitigations.
Plus it's only a partial fix because the speculative window after the branch can still be big enough to do multiple loads.
For similar reasons I'm thinking we should also remove the non-eIBRS version (SPECTRE_V2_LFENCE).
I'll make some patches to do that, with warnings printed if somebody tries to use them. They can just fall back to the (more secure and generally faster) defaults.
> [...] > > @@ -1720,6 +1744,7 @@ static void __init spectre_v2_select_mitigation(void) > > > > case SPECTRE_V2_CMD_RETPOLINE_LFENCE: > > pr_err(SPECTRE_V2_LFENCE_MSG); > > + setup_clear_cpu_cap(X86_FEATURE_INDIRECT_SAFE); > > I don't know if it intentional, this seems to be the duplicate of > X86_FEATURE_INDIRECT_SAFE clear later in SPECTRE_V2_LFENCE mode. Also it > seems a bit odd to do this here in SPECTRE_V2_CMD handling.
Yeah, I accidentally left that in from an earlier implementation. It's harmless but I'll clean that up too with a new patch unless Ingo wants to remove that line.
-- Josh
| |