| Message ID | 27fe2029a2ef8bc0909e53e7e4c3f5b437242627.1743617897.git.jpoimboe@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/bugs: RSB mitigation fixes and documentation | expand |
On 2.04.25 г. 21:19 ч., Josh Poimboeuf wrote: > IBPB is expected to clear the RSB. However, if X86_BUG_IBPB_NO_RET is > set, that doesn't happen. Make indirect_branch_prediction_barrier() > take that into account by calling __write_ibpb() which already does the > right thing. I find this changelog somewhat dubious. So zen < 4 basically have IBPB_NO_RET, your patch 2 in this series makes using SBPB for cores which have SRSO_NO or if the mitigation is disabled. So if you have a core which is zen <4 and doesn't use SBPB then what happens? > > Fixes: 50e4b3b94090 ("x86/entry: Have entry_ibpb() invalidate return predictions") > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > arch/x86/include/asm/nospec-branch.h | 6 +++--- > arch/x86/kernel/cpu/bugs.c | 1 - > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h > index bbac79cad04c..f99b32f014ec 100644 > --- a/arch/x86/include/asm/nospec-branch.h > +++ b/arch/x86/include/asm/nospec-branch.h > @@ -514,11 +514,11 @@ void alternative_msr_write(unsigned int msr, u64 val, unsigned int feature) > : "memory"); > } > > -extern u64 x86_pred_cmd; > - > static inline void indirect_branch_prediction_barrier(void) > { > - alternative_msr_write(MSR_IA32_PRED_CMD, x86_pred_cmd, X86_FEATURE_IBPB); > + asm_inline volatile(ALTERNATIVE("", "call __write_ibpb", X86_FEATURE_IBPB) > + : ASM_CALL_CONSTRAINT > + :: "rax", "rcx", "rdx", "memory"); > } > > /* The Intel SPEC CTRL MSR base value cache */ > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index c8b8dc829046..9f9637cff7a3 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -59,7 +59,6 @@ DEFINE_PER_CPU(u64, x86_spec_ctrl_current); > EXPORT_PER_CPU_SYMBOL_GPL(x86_spec_ctrl_current); > > u32 x86_pred_cmd __ro_after_init = PRED_CMD_IBPB; > -EXPORT_SYMBOL_GPL(x86_pred_cmd); > > static u64 __ro_after_init x86_arch_cap_msr; >
On Fri, Apr 04, 2025 at 05:45:37PM +0300, Nikolay Borisov wrote: > > > On 2.04.25 г. 21:19 ч., Josh Poimboeuf wrote: > > IBPB is expected to clear the RSB. However, if X86_BUG_IBPB_NO_RET is > > set, that doesn't happen. Make indirect_branch_prediction_barrier() > > take that into account by calling __write_ibpb() which already does the > > right thing. > > I find this changelog somewhat dubious. So zen < 4 basically have > IBPB_NO_RET, your patch 2 in this series makes using SBPB for cores which > have SRSO_NO or if the mitigation is disabled. So if you have a core which > is zen <4 and doesn't use SBPB then what happens? I'm afraid I don't understand the question. In that case write_ibpb() uses IBPB and manually clears the RSB.
On 4.04.25 г. 18:17 ч., Josh Poimboeuf wrote: > On Fri, Apr 04, 2025 at 05:45:37PM +0300, Nikolay Borisov wrote: >> >> >> On 2.04.25 г. 21:19 ч., Josh Poimboeuf wrote: >>> IBPB is expected to clear the RSB. However, if X86_BUG_IBPB_NO_RET is >>> set, that doesn't happen. Make indirect_branch_prediction_barrier() >>> take that into account by calling __write_ibpb() which already does the >>> right thing. >> >> I find this changelog somewhat dubious. So zen < 4 basically have >> IBPB_NO_RET, your patch 2 in this series makes using SBPB for cores which >> have SRSO_NO or if the mitigation is disabled. So if you have a core which >> is zen <4 and doesn't use SBPB then what happens? > > I'm afraid I don't understand the question. In that case write_ibpb() > uses IBPB and manually clears the RSB. > Actually isn't this patch a noop. The old code simply wrote the value of x86_pred_cmd to the IA32-PRED_CMD register iff FEATURE_IBPB was set. So x86_pred_cmd might contain either PRED_CMD_IBPB or PRED_CMD_SBPB, meaning the correct value was written. With your change you now call __write_ibpb() which does effectively the same thing.
On Sat, Apr 05, 2025 at 01:56:59AM +0300, Nikolay Borisov wrote: > On 4.04.25 г. 18:17 ч., Josh Poimboeuf wrote: > > On Fri, Apr 04, 2025 at 05:45:37PM +0300, Nikolay Borisov wrote: > > > > > > > > > On 2.04.25 г. 21:19 ч., Josh Poimboeuf wrote: > > > > IBPB is expected to clear the RSB. However, if X86_BUG_IBPB_NO_RET is > > > > set, that doesn't happen. Make indirect_branch_prediction_barrier() > > > > take that into account by calling __write_ibpb() which already does the > > > > right thing. > > > > > > I find this changelog somewhat dubious. So zen < 4 basically have > > > IBPB_NO_RET, your patch 2 in this series makes using SBPB for cores which > > > have SRSO_NO or if the mitigation is disabled. So if you have a core which > > > is zen <4 and doesn't use SBPB then what happens? > > > > I'm afraid I don't understand the question. In that case write_ibpb() > > uses IBPB and manually clears the RSB. > > > > Actually isn't this patch a noop. The old code simply wrote the value of > x86_pred_cmd to the IA32-PRED_CMD register iff FEATURE_IBPB was set. So > x86_pred_cmd might contain either PRED_CMD_IBPB or PRED_CMD_SBPB, meaning > the correct value was written. > > With your change you now call __write_ibpb() which does effectively the same > thing. Hm, are you getting SBPB and IBPB_NO_RET mixed up? They're completely separate and distinct: - SBPB is an AMD feature which is just like IBPB, except it doesn't flush branch type predictions. It can be used when the SRSO mitigation isn't needed. That was fixed by the previous patch. - AMD has a bug on older CPUs where IBPB doesn't flush the RSB. Such CPUs have X86_BUG_IBPB_NO_RET set. That's fixed with this patch due to the fact that write_ibpb() has this: /* Make sure IBPB clears return stack preductions too. */ FILL_RETURN_BUFFER %rax, RSB_CLEAR_LOOPS, X86_BUG_IBPB_NO_RET So you're right in that this patch doesn't change SBPB behavior. But that's not what it intends to do :-)
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h index bbac79cad04c..f99b32f014ec 100644 --- a/arch/x86/include/asm/nospec-branch.h +++ b/arch/x86/include/asm/nospec-branch.h @@ -514,11 +514,11 @@ void alternative_msr_write(unsigned int msr, u64 val, unsigned int feature) : "memory"); } -extern u64 x86_pred_cmd; - static inline void indirect_branch_prediction_barrier(void) { - alternative_msr_write(MSR_IA32_PRED_CMD, x86_pred_cmd, X86_FEATURE_IBPB); + asm_inline volatile(ALTERNATIVE("", "call __write_ibpb", X86_FEATURE_IBPB) + : ASM_CALL_CONSTRAINT + :: "rax", "rcx", "rdx", "memory"); } /* The Intel SPEC CTRL MSR base value cache */ diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index c8b8dc829046..9f9637cff7a3 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -59,7 +59,6 @@ DEFINE_PER_CPU(u64, x86_spec_ctrl_current); EXPORT_PER_CPU_SYMBOL_GPL(x86_spec_ctrl_current); u32 x86_pred_cmd __ro_after_init = PRED_CMD_IBPB; -EXPORT_SYMBOL_GPL(x86_pred_cmd); static u64 __ro_after_init x86_arch_cap_msr;
IBPB is expected to clear the RSB. However, if X86_BUG_IBPB_NO_RET is set, that doesn't happen. Make indirect_branch_prediction_barrier() take that into account by calling __write_ibpb() which already does the right thing. Fixes: 50e4b3b94090 ("x86/entry: Have entry_ibpb() invalidate return predictions") Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- arch/x86/include/asm/nospec-branch.h | 6 +++--- arch/x86/kernel/cpu/bugs.c | 1 - 2 files changed, 3 insertions(+), 4 deletions(-)