linux-kernel - [PATCH] KVM: x86: SVM: fix nested PAUSE filtering

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 18 May 2022 10:27:09 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     kvm@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Maxim Levitsky <mlevitsk@...hat.com>, x86@...nel.org,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Subject: [PATCH] KVM: x86: SVM: fix nested PAUSE filtering

Commit 74fd41ed16fd
("KVM: x86: nSVM: support PAUSE filtering when L0 doesn't intercept PAUSE")

introduced passthrough support for nested pause filtering,
(when the host doesn't intercept PAUSE)
(either disabled with kvm module param, or disabled with
'-overcommit cpu-pm=on')

However the feature was exposed as supported by KVM cpuid unconditionally,
thus if the guest is launched with -cpu host, it could use it even when
the KVM can't really support it.

While this use case should be avoided by the management software
(e.g libvirt), a failback was made for this case to intercept
each PAUSE instruction.

Turns out that in some cases, such intercept can slow down the
nested guest so much that it can fail to boot.

Also it turns out that when the pause filtering is not supported,
the KVM doesn't intercept PAUSE at all, so before this patch,
this slowdown didn't exist.

To fix this, change the fallback strategy - ignore the guest threshold
values, but use/update the host threshold values, instead of using zeros.

Also fix a minor bug: on nested VM exit, when PAUSE filter counter
were copied back to vmcb01, a dirty bit was not set.

Finally a note on why it 'worked' before the problematic commit in
regard to nesting:

KVM was setting both thresholds to 0 in vmcb02, but very soon afterwards,
(after a first userspace VM exit), the shrink_ple_window was called
which would reset the pause_filter_count to the default value.

Thanks a lot to Suravee Suthikulpanit for debugging this!

Fixes: 74fd41ed16fd ("KVM: x86: nSVM: support PAUSE filtering when L0 doesn't intercept PAUSE")

Reported-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Tested-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
---
 arch/x86/kvm/svm/nested.c | 16 ++++++----------
 arch/x86/kvm/svm/svm.c    |  4 ++--
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index bed5e1692cef0..f209c1ca540c9 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -681,17 +681,10 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
 				svm->pause_threshold_enabled ?
 				svm->nested.ctl.pause_filter_thresh : 0;
 
-	} else if (!vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_PAUSE)) {
-		/* use host values when guest doesn't use them */
+	} else {
+		/* use host values otherwise */
 		vmcb02->control.pause_filter_count = vmcb01->control.pause_filter_count;
 		vmcb02->control.pause_filter_thresh = vmcb01->control.pause_filter_thresh;
-	} else {
-		/*
-		 * Intercept every PAUSE otherwise and
-		 * ignore both host and guest values
-		 */
-		vmcb02->control.pause_filter_count = 0;
-		vmcb02->control.pause_filter_thresh = 0;
 	}
 
 	nested_svm_transition_tlb_flush(vcpu);
@@ -951,8 +944,11 @@ int nested_svm_vmexit(struct vcpu_svm *svm)
 	vmcb12->control.event_inj         = svm->nested.ctl.event_inj;
 	vmcb12->control.event_inj_err     = svm->nested.ctl.event_inj_err;
 
-	if (!kvm_pause_in_guest(vcpu->kvm) && vmcb02->control.pause_filter_count)
+	if (!kvm_pause_in_guest(vcpu->kvm)) {
 		vmcb01->control.pause_filter_count = vmcb02->control.pause_filter_count;
+		vmcb_mark_dirty(vmcb01, VMCB_INTERCEPTS);
+
+	}
 
 	nested_svm_copy_common_state(svm->nested.vmcb02.ptr, svm->vmcb01.ptr);
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 3b49337998ec9..aa7b387e0b7c4 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -909,7 +909,7 @@ static void grow_ple_window(struct kvm_vcpu *vcpu)
 	struct vmcb_control_area *control = &svm->vmcb->control;
 	int old = control->pause_filter_count;
 
-	if (kvm_pause_in_guest(vcpu->kvm) || !old)
+	if (kvm_pause_in_guest(vcpu->kvm))
 		return;
 
 	control->pause_filter_count = __grow_ple_window(old,
@@ -930,7 +930,7 @@ static void shrink_ple_window(struct kvm_vcpu *vcpu)
 	struct vmcb_control_area *control = &svm->vmcb->control;
 	int old = control->pause_filter_count;
 
-	if (kvm_pause_in_guest(vcpu->kvm) || !old)
+	if (kvm_pause_in_guest(vcpu->kvm))
 		return;
 
 	control->pause_filter_count =
-- 
2.26.3