* [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest @ 2020-03-18 12:41 Paolo Bonzini 2020-03-18 14:53 ` Vitaly Kuznetsov 2020-03-18 18:40 ` Krish Sadhukhan 0 siblings, 2 replies; 5+ messages in thread From: Paolo Bonzini @ 2020-03-18 12:41 UTC (permalink / raw) To: linux-kernel, kvm 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; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest 2020-03-18 12:41 [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest Paolo Bonzini @ 2020-03-18 14:53 ` Vitaly Kuznetsov 2020-03-18 18:40 ` Krish Sadhukhan 1 sibling, 0 replies; 5+ messages in thread From: Vitaly Kuznetsov @ 2020-03-18 14:53 UTC (permalink / raw) To: Paolo Bonzini; +Cc: linux-kernel, kvm Paolo Bonzini <pbonzini@redhat.com> writes: > 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; Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> -- Vitaly ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest 2020-03-18 12:41 [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest Paolo Bonzini 2020-03-18 14:53 ` Vitaly Kuznetsov @ 2020-03-18 18:40 ` Krish Sadhukhan 2020-03-19 10:05 ` Paolo Bonzini 1 sibling, 1 reply; 5+ messages in thread From: Krish Sadhukhan @ 2020-03-18 18:40 UTC (permalink / raw) To: Paolo Bonzini, linux-kernel, kvm 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. 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: +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) { ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest 2020-03-18 18:40 ` Krish Sadhukhan @ 2020-03-19 10:05 ` Paolo Bonzini 2020-03-19 18:56 ` Krish Sadhukhan 0 siblings, 1 reply; 5+ messages in thread From: Paolo Bonzini @ 2020-03-19 10:05 UTC (permalink / raw) To: Krish Sadhukhan, linux-kernel, kvm 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: 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. 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) { > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest 2020-03-19 10:05 ` Paolo Bonzini @ 2020-03-19 18:56 ` Krish Sadhukhan 0 siblings, 0 replies; 5+ messages in thread From: Krish Sadhukhan @ 2020-03-19 18:56 UTC (permalink / raw) To: Paolo Bonzini, linux-kernel, kvm 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) { >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-19 18:56 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-18 12:41 [PATCH] KVM: nSVM: check for EFER.SVME=1 before entering guest Paolo Bonzini 2020-03-18 14:53 ` Vitaly Kuznetsov 2020-03-18 18:40 ` Krish Sadhukhan 2020-03-19 10:05 ` Paolo Bonzini 2020-03-19 18:56 ` Krish Sadhukhan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).