linux-kernel - Re: [syzbot] [kvm?] WARNING in vmx_handle

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z60lxSqV1r257yW8@google.com>
Date: Wed, 12 Feb 2025 14:50:45 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: James Houghton <jthoughton@...gle.com>
Cc: syzbot+ac0bc3a70282b4d586cc@...kaller.appspotmail.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, hpa@...or.com, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, mingo@...hat.com, pbonzini@...hat.com, 
	syzkaller-bugs@...glegroups.com, tglx@...utronix.de, x86@...nel.org
Subject: Re: [syzbot] [kvm?] WARNING in vmx_handle_exit (2)

On Wed, Feb 12, 2025, James Houghton wrote:
> Here's what I think is going on (with the C repro anyway):
> 
> 1. KVM_RUN a nested VM, and eventually we end up with
>    nested_run_pending=1.
> 2. Exit KVM_RUN with EINTR (or any reason really, but I see EINTR in
>    repro attempts).
> 3. KVM_SET_REGS to set rflags to 0x1ac585, which has X86_EFLAGS_VM,
>    flipping it and setting vmx->emulation_required = true.
> 3. KVM_RUN again. vmx->emulation_required will stop KVM from clearing
>    nested_run_pending, and then we hit the
>    KVM_BUG_ON(nested_run_pending) in __vmx_handle_exit().
> 
> So I guess the KVM_BUG_ON() is a little bit too conservative, but this
> is nonsensical VMM behavior. So I'm not really sure what the best
> solution is. Sean, any thoughts?

Heh, deja vu.  This is essentially the same thing that was fixed by commit
fc4fad79fc3d ("KVM: VMX: Reject KVM_RUN if emulation is required with pending
exception"), just with a different WARN.

This should fix it.  Checking nested_run_pending in handle_invalid_guest_state()
is overkill, but it can't possibly do any harm, and the weirdness can be addressed
with a comment.

---
 arch/x86/kvm/vmx/vmx.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index f72835e85b6d..8c9428244cc6 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5869,11 +5869,17 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
-static bool vmx_emulation_required_with_pending_exception(struct kvm_vcpu *vcpu)
+static bool vmx_unhandleable_emulation_required(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
-	return vmx->emulation_required && !vmx->rmode.vm86_active &&
+	if (!vmx->emulation_required)
+		return false;
+
+	if (vmx->nested.nested_run_pending)
+		return true;
+
+	return !vmx->rmode.vm86_active &&
 	       (kvm_is_exception_pending(vcpu) || vcpu->arch.exception.injected);
 }
 
@@ -5896,7 +5902,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 		if (!kvm_emulate_instruction(vcpu, 0))
 			return 0;
 
-		if (vmx_emulation_required_with_pending_exception(vcpu)) {
+		if (vmx_unhandleable_emulation_required(vcpu)) {
 			kvm_prepare_emulation_failure_exit(vcpu);
 			return 0;
 		}
@@ -5920,7 +5926,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 
 int vmx_vcpu_pre_run(struct kvm_vcpu *vcpu)
 {
-	if (vmx_emulation_required_with_pending_exception(vcpu)) {
+	if (vmx_unhandleable_emulation_required(vcpu)) {
 		kvm_prepare_emulation_failure_exit(vcpu);
 		return 0;
 	}

base-commit: b1da62b213ed5f01d7ead4d14e9d51b48b6256e4
--