x86/retpoline: Also fill return buffer after idle
Subsystems (2)
PATCH 1/1 x86/retpoline: Also fill return buffer after idle
From: Andi Kleen <[email protected]>
This is an extension of the earlier patch to fill the return buffer on context switch. It uses the assembler macros added earlier.
When we go into deeper idle states the return buffer could be cleared in MWAIT, but then another thread which wakes up earlier might be poisoning the indirect branch predictor. Then when the return buffer underflows there might an uncontrolled indirect branch.
To guard against this always fill the return buffer when exiting idle.
Needed on Skylake and some Broadwells.
--- arch/x86/entry/entry_32.S | 8 ++++++++ arch/x86/entry/entry_64.S | 8 ++++++++ arch/x86/include/asm/mwait.h | 11 ++++++++++- 3 files changed, 26 insertions(+), 1 deletion(-)
arch/x86/entry/entry_32.S +8 -0
@@ -1092,3 +1092,11 @@ ENTRY(rewind_stack_do_exit) call do_exit 1: jmp 1b END(rewind_stack_do_exit) + +ENTRY(fill_return_buffer) +#ifdef CONFIG_RETPOLINE + ALTERNATIVE "ret", "", X86_FEATURE_RETPOLINE + FILL_RETURN_BUFFER +#endif + ret +END(fill_return_buffer)
arch/x86/entry/entry_64.S +8 -0
@@ -1831,3 +1831,11 @@ ENTRY(rewind_stack_do_exit) call do_exit END(rewind_stack_do_exit) + +ENTRY(fill_return_buffer) +#ifdef CONFIG_RETPOLINE + ALTERNATIVE "ret", "", X86_FEATURE_RETPOLINE + FILL_RETURN_BUFFER +#endif + ret +END(fill_return_buffer)
arch/x86/include/asm/mwait.h +10 -1
@@ -87,6 +87,8 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx) :: "a" (eax), "c" (ecx)); } +extern __visible void fill_return_buffer(void); +/* * This uses new MONITOR/MWAIT instructions on P4 processors with PNI, * which can obviate IPI to trigger checking of need_resched.
@@ -107,8 +109,15 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) }__monitor ((void *)¤t_thread_info ()->flags,0 ,0 ); -if (!need_resched ()) +if (!need_resched ()) {__mwait (eax, ecx); +/* +* idle could have cleared the return buffer, +* so fill it to prevent uncontrolled +* speculation. +*/ +fill_return_buffer (); + } }current_clr_polling (); }
Signature · 1 line
2.14.3
Quoted 80 lines · Andi Kleen
From: Andi Kleen <[email protected]>
This is an extension of the earlier patch to fill the return buffer on context switch. It uses the assembler macros added earlier.
When we go into deeper idle states the return buffer could be cleared in MWAIT, but then another thread which wakes up earlier might be poisoning the indirect branch predictor. Then when the return buffer underflows there might an uncontrolled indirect branch.
To guard against this always fill the return buffer when exiting idle.
Needed on Skylake and some Broadwells.
Signed-off-by Andi Kleen <[email protected]>---
arch/x86/entry/entry_32.S | 8 ++++++++
arch/x86/entry/entry_64.S | 8 ++++++++
arch/x86/include/asm/mwait.h | 11 ++++++++++-3 files changed, 26 insertions(+), 1 deletion(-)arch/x86/entry/entry_32.S +0
-0@@ -1092,3 +1092,11 @@ ENTRY(rewind_stack_do_exit)call
do_exit
1: jmp 1b
END(rewind_stack_do_exit)arch/x86/entry/entry_32.S +8
-0+ +ENTRY(fill_return_buffer) +#ifdef CONFIG_RETPOLINE + ALTERNATIVE "ret", "", X86_FEATURE_RETPOLINE + FILL_RETURN_BUFFER +#endif + ret +END(fill_return_buffer)arch/x86/entry/entry_64.S +0
-0@@ -1831,3 +1831,11 @@ ENTRY(rewind_stack_do_exit)
calldo_exit
END(rewind_stack_do_exit)arch/x86/entry/entry_64.S +8
-0+ +ENTRY(fill_return_buffer) +#ifdef CONFIG_RETPOLINE + ALTERNATIVE "ret", "", X86_FEATURE_RETPOLINE + FILL_RETURN_BUFFER +#endif + ret +END(fill_return_buffer)arch/x86/include/asm/mwait.h +0
-0@@ -87,6 +87,8 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx) :: "a" (eax), "c" (ecx)); }arch/x86/include/asm/mwait.h +2
-0+extern __visible void fill_return_buffer(void); +/*
* This uses new MONITOR/MWAIT instructions on P4 processors with PNI,
* which can obviate IPI to trigger checking ofneed_resched.arch/x86/include/asm/mwait.h +0
-0@@ -107,8 +109,15 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) }
__monitor((void *)¤t_thread_info()->flags, 0, 0);arch/x86/include/asm/mwait.h +1
-1-if (!need_resched ()) +if (!need_resched ()) {
__mwait(eax, ecx);arch/x86/include/asm/mwait.h +7
-0+/* +* idle could have cleared the return buffer, +* so fill it to prevent uncontrolled +* speculation. +*/ +fill_return_buffer (); + }
Quoted 3 lines
}
current_clr_polling();
}
Probably doesn't matter right there but it's going to end up being used elsewhere with IBRS/IBPB, and the compiler is going to think it needs to save all the call-clobbered registers for that. Do we want to make it use inline asm instead?
Quoted 4 lines
Probably doesn't matter right there but it's going to end up being used elsewhere with IBRS/IBPB, and the compiler is going to think it needs to save all the call-clobbered registers for that. Do we want to make it use inline asm instead?
You mean KVM?
All the other places have lots of other calls too, so the registers are likely already clobbered.
-Andi
Quoted 9 lines · Andi Kleen
Quoted 4 lines
Probably doesn't matter right there but it's going to end up being used elsewhere with IBRS/IBPB, and the compiler is going to think it needs to save all the call-clobbered registers for that. Do we want to make it use inline asm instead?
You mean KVM?
All the other places have lots of other calls too, so the registers are likely already clobbered.
Fair enough.
arch/x86/include/asm/mwait.h static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) · line 119
Quoted 13 lines · Andi Kleen
arch/x86/include/asm/mwait.h +7
-1@@ -107,8 +109,15 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) }__monitor ((void *)¤t_thread_info ()->flags,0 ,0 ); -if (!need_resched ()) +if (!need_resched ()) {__mwait (eax, ecx); +/* +* idle could have cleared the return buffer, +* so fill it to prevent uncontrolled +* speculation. +*/ +fill_return_buffer ();
wouldn't something like:
if (static_cpu_has(X86_FEATURE_RETPOLINE)) fill_return_buffer();
be much saner? Then we avoid the entire call when not needed and you don't have to muck with the asm either.
Also, you forgot mwait_idle() in process.c
Quoted 7 lines
arch/x86/include/asm/mwait.h +1
-0+ } }current_clr_polling (); }Signature · 1 line
2.14.3
Quoted 18 lines · Peter Zijlstra
Quoted 8 lines · Andi Kleen
(quoted diff) +0
-0@@ -107,8 +109,15 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) }
__monitor((void *)¤t_thread_info()->flags, 0, 0);arch/x86/include/asm/mwait.h +1
-1-if (!need_resched ()) +if (!need_resched ()) {
__mwait(eax, ecx);arch/x86/include/asm/mwait.h static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) · line 119
Quoted 6 lines
arch/x86/include/asm/mwait.h +6
-0+/* +* idle could have cleared the return buffer, +* so fill it to prevent uncontrolled +* speculation. +*/ +fill_return_buffer ();wouldn't something like:
Quoted 5 lines
if (static_cpu_has(X86_FEATURE_RETPOLINE)) fill_return_buffer();be much saner? Then we avoid the entire call when not needed and you don't have to muck with the asm either.
Hm...
The background, of course, that that we need to be careful when doing things like this. If you end up with a conditional branch there, then processor can speculate right past it. There's a reason a lot of the IBRS-setting code has, effectively, an 'else lfence' in the cases where it isn't being done with ALTERNATIVEs.
We had a *beautiful* case of that in the early IBRS patch set, on the syscall path, where the conditional branch opened up a path for speculative execution all the way to the jmp *sys_call_table(…).
Now, as discussed on IRC, we can see that the current implementation of static_cpu_has using asm goto *is* generally doing the right thing and turning it into a straight unconditional jump over the fill_return_buffer() code. Clever GCC, have biscuit.
However, you are suggesting that we turn the static_cpu_has() trick from a "nice to have" optimisation which is all very well when it pans out, to something we *rely* on for secure operation of the system.
It never ends well when we rely on all versions of GCC optimising things precisely how we want.
If you can build in a sanity check to ensure that the build will *fail* when GCC doesn't do what we want, I suppose we could live with that. But we don't have such a sanity check at the moment, do we?
Quoted 1 line · David Woodhouse
Clever GCC, have biscuit.
Well, we requested this feature exactly because of this. It had better work.
Quoted 3 lines
However, you are suggesting that we turn the
static_cpu_has()trick from a "nice to have" optimisation which is all very well when it pans out, to something we *rely* on for secure operation of the system.
It must work, we 'rely' on it already. GCC doing something stupid there is a GCC bug. Any GCC bug is a royal pain, they happen, life goes on.
Quoted 6 lines
It never ends well when we rely on all versions of GCC optimising things precisely how we want.
If you can build in a sanity check to ensure that the build will *fail* when GCC doesn't do what we want, I suppose we could live with that. But we don't have such a sanity check at the moment, do we?
We have STATIC_KEYS_SELFTEST, which might or might not qualify.