[PATCH] KVM: SVM: fix calls to is_intercept

[PATCH] KVM: SVM: fix calls to is_intercept

[Paolo Bonzini]

is_intercept takes an INTERCEPT_* constant, not SVM_EXIT_*; because
of this, the compiler was removing the body of the conditionals,
as if is_intercept returned 0.

This unveils a latent bug: when clearing the VINTR intercept,
int_ctl must also be changed in the L1 VMCB (svm->nested.hsave),
just like the intercept itself is also changed in the L1 VMCB.
Otherwise V_IRQ remains set and, due to the VINTR intercept being clear,
we get a spurious injection of a vector 0 interrupt on the next
L2->L1 vmexit.

Reported-by: Qian Cai <cai@lca.pw>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
	Vitaly, can you give this a shot with Hyper-V?  I have already
	placed it on kvm/queue, it passes both svm.flat and KVM-on-KVM
	smoke tests.

 arch/x86/kvm/svm/nested.c | 2 +-
 arch/x86/kvm/svm/svm.c    | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 8a6db11dcb43..6bceafb19108 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -258,7 +258,7 @@ void sync_nested_vmcb_control(struct vcpu_svm *svm)
 	/* Only a few fields of int_ctl are written by the processor.  */
 	mask = V_IRQ_MASK | V_TPR_MASK;
 	if (!(svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) &&
-	    is_intercept(svm, SVM_EXIT_VINTR)) {
+	    is_intercept(svm, INTERCEPT_VINTR)) {
 		/*
 		 * In order to request an interrupt window, L0 is usurping
 		 * svm->vmcb->control.int_ctl and possibly setting V_IRQ
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 9e333b91ff78..c8f5e87615d5 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -1378,6 +1378,8 @@ static void svm_clear_vintr(struct vcpu_svm *svm)
 	/* Drop int_ctl fields related to VINTR injection.  */
 	svm->vmcb->control.int_ctl &= mask;
 	if (is_guest_mode(&svm->vcpu)) {
+		svm->nested.hsave->control.int_ctl &= mask;
+
 		WARN_ON((svm->vmcb->control.int_ctl & V_TPR_MASK) !=
 			(svm->nested.ctl.int_ctl & V_TPR_MASK));
 		svm->vmcb->control.int_ctl |= svm->nested.ctl.int_ctl & ~mask;
@@ -1999,7 +2001,7 @@ void svm_set_gif(struct vcpu_svm *svm, bool value)
 		 */
 		if (vgif_enabled(svm))
 			clr_intercept(svm, INTERCEPT_STGI);
-		if (is_intercept(svm, SVM_EXIT_VINTR))
+		if (is_intercept(svm, INTERCEPT_VINTR))
 			svm_clear_vintr(svm);
 
 		enable_gif(svm);
-- 
2.26.2

Re: [PATCH] KVM: SVM: fix calls to is_intercept

[Vitaly Kuznetsov]

Paolo Bonzini <pbonzini@redhat.com> writes:

> is_intercept takes an INTERCEPT_* constant, not SVM_EXIT_*; because
> of this, the compiler was removing the body of the conditionals,
> as if is_intercept returned 0.
>
> This unveils a latent bug: when clearing the VINTR intercept,
> int_ctl must also be changed in the L1 VMCB (svm->nested.hsave),
> just like the intercept itself is also changed in the L1 VMCB.
> Otherwise V_IRQ remains set and, due to the VINTR intercept being clear,
> we get a spurious injection of a vector 0 interrupt on the next
> L2->L1 vmexit.
>
> Reported-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	Vitaly, can you give this a shot with Hyper-V?  I have already
> 	placed it on kvm/queue, it passes both svm.flat and KVM-on-KVM
> 	smoke tests.

Quickly smoke-tested this with WS2016/2019 BIOS/UEFI and the patch
doesn't seem to break anything. I'm having issues trying to launch a
Gen2 (UEFI) VM in Hyper-V (Gen1 works OK) but the behavior looks exactly
the same pre- and post-patch.

-- 
Vitaly

Re: [PATCH] KVM: SVM: fix calls to is_intercept

[Maxim Levitsky]

On Mon, 2020-06-08 at 14:51 +0200, Vitaly Kuznetsov wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > is_intercept takes an INTERCEPT_* constant, not SVM_EXIT_*; because
> > of this, the compiler was removing the body of the conditionals,
> > as if is_intercept returned 0.
> > 
> > This unveils a latent bug: when clearing the VINTR intercept,
> > int_ctl must also be changed in the L1 VMCB (svm->nested.hsave),
> > just like the intercept itself is also changed in the L1 VMCB.
> > Otherwise V_IRQ remains set and, due to the VINTR intercept being
> > clear,
> > we get a spurious injection of a vector 0 interrupt on the next
> > L2->L1 vmexit.
> > 
> > Reported-by: Qian Cai <cai@lca.pw>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> > 	Vitaly, can you give this a shot with Hyper-V?  I have already
> > 	placed it on kvm/queue, it passes both svm.flat and KVM-on-KVM
> > 	smoke tests.
> 
> Quickly smoke-tested this with WS2016/2019 BIOS/UEFI and the patch
> doesn't seem to break anything. I'm having issues trying to launch a
> Gen2 (UEFI) VM in Hyper-V (Gen1 works OK) but the behavior looks
> exactly
> the same pre- and post-patch.
> 

And if I understand correctly that bug didn't affect anything I tested
because your recent patches started to avoid the usage of the interrupt
window unless L1 clears the usage of the interrupt intercept which is
rare.

Looks correct to me, and I guess this could have being avoided have C
enforced the enumeration types.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH] KVM: SVM: fix calls to is_intercept

[Paolo Bonzini]

On 09/06/20 09:30, Maxim Levitsky wrote:
> And if I understand correctly that bug didn't affect anything I tested
> because your recent patches started to avoid the usage of the interrupt
> window unless L1 clears the usage of the interrupt intercept which is
> rare.
> 
> Looks correct to me, and I guess this could have being avoided have C
> enforced the enumeration types.

Yes, another possibility could be to unify SVM_EXIT_* and INTERCEPT_*
enums.  For example we could have something like

	union {
		u32 all[5];
		struct {
			u32 cr, dr, exceptions;
		};
	} intercept;

and use __set/clear/test_bit_le() in set/clr/is_intercept.

Paolo