Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest | From | Krish Sadhukhan <> | Date | Thu, 19 Mar 2020 11:56:02 -0700 |
| |
On 3/19/20 3:05 AM, Paolo Bonzini wrote: > On 18/03/20 19:40, Krish Sadhukhan wrote: >> On 3/18/20 5:41 AM, Paolo Bonzini wrote: >>> EFER is set for L2 using svm_set_efer, which hardcodes EFER_SVME to 1 >>> and hides >>> an incorrect value for EFER.SVME in the L1 VMCB. Perform the check >>> manually >>> to detect invalid guest state. >>> >>> Reported-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >>> --- >>> arch/x86/kvm/svm.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >>> index 08568ae9f7a1..2125c6ae5951 100644 >>> --- a/arch/x86/kvm/svm.c >>> +++ b/arch/x86/kvm/svm.c >>> @@ -3558,6 +3558,9 @@ static bool nested_svm_vmrun_msrpm(struct >>> vcpu_svm *svm) >>> static bool nested_vmcb_checks(struct vmcb *vmcb) >>> { >>> + if ((vmcb->save.efer & EFER_SVME) == 0) >>> + return false; >>> + >>> if ((vmcb->control.intercept & (1ULL << INTERCEPT_VMRUN)) == 0) >>> return false; >>> >> Ah! This now tells me that I forgot the KVM fix that was supposed to >> accompany my patchset. > Heh, indeed. I was puzzled for a second after applying it, then decided > I would just fix it myself. :) > >> Do we need this check in software ? I wasn't checking the bit in KVM and >> instead I was just making sure that L0 sets that bit based on the >> setting in nested vmcb: Thanks ! > The only effect of the function below over svm_set_efer is to guarantee > a vmrun error to happen. Doing the test in nested_vmcb_checks is more > consistent with other must-be-one bits such as the VMRUN intercept, and > it's also a smaller patch.
Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Paolo > >> +static void nested_svm_set_efer(struct kvm_vcpu *vcpu, u64 >> nested_vmcb_efer) >> +{ >> + svm_set_efer(vcpu, nested_vmcb_efer); >> + >> + if (!(nested_vmcb_efer & EFER_SVME)) >> + to_svm(vcpu)->vmcb->save.efer &= ~EFER_SVME; >> +} >> + >> static int is_external_interrupt(u32 info) >> { >> info &= SVM_EVTINJ_TYPE_MASK | SVM_EVTINJ_VALID; >> @@ -3554,7 +3562,7 @@ static void enter_svm_guest_mode(struct vcpu_svm >> *svm, u64 >> svm->vmcb->save.gdtr = nested_vmcb->save.gdtr; >> svm->vmcb->save.idtr = nested_vmcb->save.idtr; >> kvm_set_rflags(&svm->vcpu, nested_vmcb->save.rflags); >> - svm_set_efer(&svm->vcpu, nested_vmcb->save.efer); >> + nested_svm_set_efer(&svm->vcpu, nested_vmcb->save.efer); >> svm_set_cr0(&svm->vcpu, nested_vmcb->save.cr0); >> svm_set_cr4(&svm->vcpu, nested_vmcb->save.cr4); >> if (npt_enabled) { >>
| ![\](/images/icornerr.gif) |