Re: [PATCH] KVM: VMX: Flush shadow VMCS on emergency reboot

[Chao Gao <chao.gao@intel.com>]

On Thu, Apr 10, 2025 at 02:55:29PM -0700, Sean Christopherson wrote:
>On Mon, Mar 24, 2025, Chao Gao wrote:
>> Ensure the shadow VMCS cache is evicted during an emergency reboot to
>> prevent potential memory corruption if the cache is evicted after reboot.
>
>I don't suppose Intel would want to go on record and state what CPUs would actually
>be affected by this bug.  My understanding is that Intel has never shipped a CPU
>that caches shadow VMCS state.

I am not sure. Would you like me to check internally?

However, SDM Chapter 26.11 includes a footnote stating:
"
As noted in Section 26.1, execution of the VMPTRLD instruction makes a VMCS is
active. In addition, VM entry makes active any shadow VMCS referenced by the
VMCS link pointer in the current VMCS. If a shadow VMCS is made active by VM
entry, it is necessary to execute VMCLEAR for that VMCS before allowing that
VMCS to become active on another logical processor.
"

To me, this suggests that shadow VMCS may be cached, and software shouldn't
assume the CPU won't cache it. But, I don't know if this is the reality or
if the statement is merely for hardware implementation flexibility.

>
>On a very related topic, doesn't SPR+ now flush the VMCS caches on VMXOFF?  If

Actually this behavior is not publicly documented.

>that's going to be the architectural behavior going forward, will that behavior
>be enumerated to software?  Regardless of whether there's software enumeration,
>I would like to have the emergency disable path depend on that behavior.  In part
>to gain confidence that SEAM VMCSes won't screw over kdump, but also in light of
>this bug.

I don't understand how we can gain confidence that SEAM VMCSes won't screw
over kdump.

If a VMM wants to leverage the VMXOFF behavior, software enumeration
might be needed for nested virtualization. Using CPU F/M/S (SPR+) to
enumerate a behavior could be problematic for virtualization. Right?

>
>If all past CPUs never cache shadow VMCS state, and all future CPUs flush the
>caches on VMXOFF, then this is a glorified NOP, and thus probably shouldn't be
>tagged for stable.

Agreed.

Sean, I am not clear whether you intend to fix this issue and, if so, how.
Could you clarify?

>
>> This issue was identified through code inspection, as __loaded_vmcs_clear()
>> flushes both the normal VMCS and the shadow VMCS.
>> 
>> Avoid checking the "launched" state during an emergency reboot, unlike the
>> behavior in __loaded_vmcs_clear(). This is important because reboot NMIs
>> can interfere with operations like copy_shadow_to_vmcs12(), where shadow
>> VMCSes are loaded directly using VMPTRLD. In such cases, if NMIs occur
>> right after the VMCS load, the shadow VMCSes will be active but the
>> "launched" state may not be set.
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>>  arch/x86/kvm/vmx/vmx.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index b70ed72c1783..dccd1c9939b8 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -769,8 +769,11 @@ void vmx_emergency_disable_virtualization_cpu(void)
>>  		return;
>>  
>>  	list_for_each_entry(v, &per_cpu(loaded_vmcss_on_cpu, cpu),
>> -			    loaded_vmcss_on_cpu_link)
>> +			    loaded_vmcss_on_cpu_link) {
>>  		vmcs_clear(v->vmcs);
>> +		if (v->shadow_vmcs)
>> +			vmcs_clear(v->shadow_vmcs);
>> +	}
>>  
>>  	kvm_cpu_vmxoff();
>>  }
>> -- 
>> 2.46.1
>>