| Message ID | 20181130172858.6821-1-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM nVMX: MSRs should not be stored if VM-entry fails during or after loading guest state | expand |
On Fri, Nov 30, 2018 at 9:53 AM Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > If a bit-pattern for the MSR-store address is so chosen that the > address is 16-bit aligned and is less than the processor's physical > width, but is a junk, VMENTRY ultimately fails even though we > pass the checks in check_vmentry_prereqs(). In such a case, the high bit > in "exit reason" will be set, denoting a VM-entry failure. But since we > call nested_vmx_store_msr() in nested_vmx_vmexit() without checking the > "exit reason", we may end up storing the MSRs at an undefined location in > memory thereby clobbering something else. This patch fixes the problem by > calling nested_vmx_store_msr() only when "exit reason" is not VM-entry > failure. This is not a very good description of the problem. Perhaps... Per the SDM, volume 3, section 26.7: VM-entry Failures During or After Loading Guest State, "no MSRs are saved into the VM-exit MSR-store area" when bit 31 of the exit reason is set.
On Fri, Nov 30, 2018 at 02:22:03PM -0800, Jim Mattson wrote: > On Fri, Nov 30, 2018 at 9:53 AM Krish Sadhukhan > <krish.sadhukhan@oracle.com> wrote: > > > > If a bit-pattern for the MSR-store address is so chosen that the > > address is 16-bit aligned and is less than the processor's physical > > width, but is a junk, VMENTRY ultimately fails even though we > > pass the checks in check_vmentry_prereqs(). In such a case, the high bit > > in "exit reason" will be set, denoting a VM-entry failure. But since we > > call nested_vmx_store_msr() in nested_vmx_vmexit() without checking the > > "exit reason", we may end up storing the MSRs at an undefined location in > > memory thereby clobbering something else. This patch fixes the problem by > > calling nested_vmx_store_msr() only when "exit reason" is not VM-entry > > failure. > > This is not a very good description of the problem. Agreed. And the changelog also fails to mention the flows where we're not emulating a true VMExit, i.e. exit_reason==-1. Not storing the MSRs in those cases is correct, but it should be addressed in the changelog. > Perhaps... > > Per the SDM, volume 3, section 26.7: VM-entry Failures During or After > Loading Guest State, "no MSRs are saved into the VM-exit MSR-store > area" when bit 31 of the exit reason is set. Something like this would also be a good comment in the code, maybe with a bit of elaboration as to why, e.g.: /* * VM-exits due to VM-entry failures that occur during or after * loading guest state do not process the MSR store list. * There's no need to store the guest's MSR values as the guest * can't have changed its MSRs (never executed an instruction). */ if (nested_vmx_store_msr(vcpu, vmcs12->vm_exit_msr_store_addr, vmcs12->vm_exit_msr_store_count)) nested_vmx_abort(vcpu, VMX_ABORT_SAVE_GUEST_MSR_FAIL);
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 4555077..aecd461 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -13827,6 +13827,12 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, * L2 to IDT_VECTORING_INFO_FIELD. */ vmcs12_save_pending_event(vcpu, vmcs12); + + if (nested_vmx_store_msr(vcpu, + vmcs12->vm_exit_msr_store_addr, + vmcs12->vm_exit_msr_store_count)) + nested_vmx_abort(vcpu, + VMX_ABORT_SAVE_GUEST_MSR_FAIL); } /* @@ -14159,10 +14165,6 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason, * immutable. */ nested_flush_cached_shadow_vmcs12(vcpu, vmcs12); - - if (nested_vmx_store_msr(vcpu, vmcs12->vm_exit_msr_store_addr, - vmcs12->vm_exit_msr_store_count)) - nested_vmx_abort(vcpu, VMX_ABORT_SAVE_GUEST_MSR_FAIL); } else { /* * The only expected VM-instruction error is "VM entry with