Re: [PATCH] x86/svm: Separate STI and VMRUN instructions in svm_asm_do_resume()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.02.2025 18:40, Andrew Cooper wrote:
> On 17/02/2025 4:51 pm, Jan Beulich wrote:
>> On 17.02.2025 17:12, Andrew Cooper wrote:
>>> There is a corner case in the VMRUN instruction where its INTR_SHADOW state
>>> leaks into guest state if a VMExit occurs before the VMRUN is complete.  An
>>> example of this could be taking #NPF due to event injection.
>> Ouch.
> 
> Yeah.  Intel go out of their way to make VM{LAUNCH,RESUME} fail if
> they're executed in a shadow.
> 
>>
>>> --- a/xen/arch/x86/hvm/svm/entry.S
>>> +++ b/xen/arch/x86/hvm/svm/entry.S
>>> @@ -57,6 +57,14 @@ __UNLIKELY_END(nsvm_hap)
>>>  
>>>          clgi
>>>  
>>> +        /*
>>> +         * Set EFLAGS.IF, after CLGI covers us from real interrupts, but 
>>> not
>>> +         * immediately prior to VMRUN.  AMD CPUs leak Xen's INTR_SHADOW 
>>> from
>>> +         * the STI into guest state if a VMExit occurs during VMEntry
>>> +         * (e.g. taking #NPF during event injecting.)
>>> +         */
>>> +        sti
>>> +
>>>          /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
>>>          /* SPEC_CTRL_EXIT_TO_SVM       Req: b=curr %rsp=regs/cpuinfo, 
>>> Clob: acd */
>>>          .macro svm_vmentry_spec_ctrl
>> I'm mildly worried to see it moved this high up. Any exception taken in
>> this exit code would consider the system to have interrupts enabled, when
>> we have more restrictive handling for the IF=0 case. Could we meet in the
>> middle and have STI before we start popping registers off the stack, but
>> after all the speculation machinery?
> 
> Any exception taken here is fatal, and going to fail in weird ways. 
> e.g. we don't clean up GIF before entering the crash kernel.
> 
> But yes, we probably should take steps to avoid the interrupted context
> from looking even more weird than usual.
> 
> I'll put it above the line of pops.  They're going to turn into a single
> macro when I can dust off that series.

Then:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan