Re: [PATCH v2 7/9] x86/svm: VMEntry/Exit logic for MSR_SPEC_CTRL

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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