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@xxxxxxxxxx> 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);
