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@xxxxxxxxx>
>> ---
>> 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
>>
