On 31/01/2022 12:55, Jan Beulich wrote: > On 28.01.2022 14:29, Andrew Cooper wrote: >> --- a/xen/arch/x86/hvm/svm/entry.S >> +++ b/xen/arch/x86/hvm/svm/entry.S >> @@ -55,11 +55,12 @@ __UNLIKELY_END(nsvm_hap) >> mov %rsp, %rdi >> call svm_vmenter_helper >> >> - mov VCPU_arch_msrs(%rbx), %rax >> - mov VCPUMSR_spec_ctrl_raw(%rax), %eax >> + clgi >> >> /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */ >> - /* SPEC_CTRL_EXIT_TO_SVM (nothing currently) */ >> + /* SPEC_CTRL_EXIT_TO_SVM Req: Clob: >> C */ >> + ALTERNATIVE "", STR(mov %rbx, %rdi; mov %rsp, %rsi), >> X86_FEATURE_SC_MSR_HVM >> + ALTERNATIVE "", STR(call vmentry_spec_ctrl), X86_FEATURE_SC_MSR_HVM > Both this and ... > >> @@ -86,8 +86,10 @@ __UNLIKELY_END(nsvm_hap) >> >> GET_CURRENT(bx) >> >> - /* SPEC_CTRL_ENTRY_FROM_SVM Req: b=curr %rsp=regs/cpuinfo, Clob: >> ac */ >> + /* SPEC_CTRL_ENTRY_FROM_SVM Req: Clob: >> C */ >> ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM >> + ALTERNATIVE "", STR(mov %rsp, %rdi), X86_FEATURE_SC_MSR_HVM >> + ALTERNATIVE "", STR(call vmexit_spec_ctrl), X86_FEATURE_SC_MSR_HVM >> /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */ > ... this now effectively violate what the warning comment says, as there > is a RET involved in the C call. If this is not a problem for some reason, > I'd like to ask that the comments be updated accordingly.
The `ret` note pertains to two things: 1) RSB underflows falling back to indirect predictions 2) SpectreRSB executing more rets than calls Aspect 2 is largely theoretical, but can happen with an out of bounds write which hits the return address on the stack in an otherwise regular call tree. Once DO_OVERWRITE_RSB is complete, there are no user RSB entries to consume. I know this gets complicated with the RAS[:32] flushing which is part of why the behaviour is up for consideration, but even the current code completes the full flush before a ret is executed. Aspect 1 is a feature seemingly unique to Intel CPUs, and we have to set MSR_SPEC_CTRL.IBRS to 1 before indirect predictions are "safe". That said, I stand by the comments as they are. They're there for other code to remember to be careful. I think it is entirely reasonable to expect the internals of the speculative safety logic to know how to stay safe. I'll see how it looks with the helpers inlined. That's the easiest way of fixing this issue. ~Andrew
